Message ID | d8f8e86a4e89a48150fae5cc4ee4bb977a13c196.1525354194.git-series.kieran.bingham+renesas@ideasonboard.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Laurent Pinchart |
Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1fEEPX-0005L8-AP; Thu, 03 May 2018 13:36:39 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751575AbeECNgf (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 3 May 2018 09:36:35 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:34888 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751380AbeECNg3 (ORCPT <rfc822; linux-media@vger.kernel.org>); Thu, 3 May 2018 09:36:29 -0400 Received: from localhost.localdomain (cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id EE69E6179; Thu, 3 May 2018 15:36:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1525354587; bh=Pcer3MMyKc+vRes1IjBM6Ak2cR+1EIB+gmxT7le3SDU=; h=From:To:Cc:Subject:Date:In-Reply-To:References:In-Reply-To: References:From; b=dBCSuwSAX48TyfhJtgO5MxBUMYHE+KQ1e2wqrbfDVeib/6XGa3wmv3wJTHDORbi12 QIUxwN1HkIvbop6WbeIpugRyiArn1TiLyw+5pSbuR0k3icQv6otZTU5938+HrG+6er gbW2XNcUcQVZPmZpSeEDh4TiDW1bFIdbhCsGKxEM= From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, linux-renesas-soc@vger.kernel.org, linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org, Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Subject: [PATCH v4 02/11] media: vsp1: Remove packed attributes from aligned structures Date: Thu, 3 May 2018 14:36:13 +0100 Message-Id: <d8f8e86a4e89a48150fae5cc4ee4bb977a13c196.1525354194.git-series.kieran.bingham+renesas@ideasonboard.com> X-Mailer: git-send-email 2.17.0 In-Reply-To: <cover.bd2eb66d11f8094114941107dbc78dc02c9c7fdd.1525354194.git-series.kieran.bingham+renesas@ideasonboard.com> References: <cover.bd2eb66d11f8094114941107dbc78dc02c9c7fdd.1525354194.git-series.kieran.bingham+renesas@ideasonboard.com> In-Reply-To: <cover.bd2eb66d11f8094114941107dbc78dc02c9c7fdd.1525354194.git-series.kieran.bingham+renesas@ideasonboard.com> References: <cover.bd2eb66d11f8094114941107dbc78dc02c9c7fdd.1525354194.git-series.kieran.bingham+renesas@ideasonboard.com> Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Kieran Bingham
May 3, 2018, 1:36 p.m. UTC
The use of the packed attribute can cause a performance penalty for
all accesses to the struct members, as the compiler will assume that the
structure has the potential to have an unaligned base.
These structures are all correctly aligned and contain no holes, thus
the attribute is redundant and negatively impacts performance, so we
remove the attributes entirely.
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
v2
- Remove attributes entirely
---
drivers/media/platform/vsp1/vsp1_dl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Comments
Hi Kieran, Thank you for the patch. On Thursday, 3 May 2018 16:36:13 EEST Kieran Bingham wrote: > The use of the packed attribute can cause a performance penalty for > all accesses to the struct members, as the compiler will assume that the > structure has the potential to have an unaligned base. > > These structures are all correctly aligned and contain no holes, thus > the attribute is redundant and negatively impacts performance, so we > remove the attributes entirely. With gcc 6.4.0 this patch makes no difference on the generated object. Is it worth it ? > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> You forget to pick Geert's review tag. > --- > v2 > - Remove attributes entirely > --- > drivers/media/platform/vsp1/vsp1_dl.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/vsp1/vsp1_dl.c > b/drivers/media/platform/vsp1/vsp1_dl.c index c7fa1cb088cd..f4cede9b9b43 > 100644 > --- a/drivers/media/platform/vsp1/vsp1_dl.c > +++ b/drivers/media/platform/vsp1/vsp1_dl.c > @@ -25,19 +25,19 @@ > struct vsp1_dl_header_list { > u32 num_bytes; > u32 addr; > -} __attribute__((__packed__)); > +}; > > struct vsp1_dl_header { > u32 num_lists; > struct vsp1_dl_header_list lists[8]; > u32 next_header; > u32 flags; > -} __attribute__((__packed__)); > +}; > > struct vsp1_dl_entry { > u32 addr; > u32 data; > -} __attribute__((__packed__)); > +}; > > /** > * struct vsp1_dl_body - Display list body
Hi Laurent, On 24/05/18 11:47, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thursday, 3 May 2018 16:36:13 EEST Kieran Bingham wrote: >> The use of the packed attribute can cause a performance penalty for >> all accesses to the struct members, as the compiler will assume that the >> structure has the potential to have an unaligned base. >> >> These structures are all correctly aligned and contain no holes, thus >> the attribute is redundant and negatively impacts performance, so we >> remove the attributes entirely. > > With gcc 6.4.0 this patch makes no difference on the generated object. Is it > worth it ? This patch has certainly either enlightened me, or confused me about this topic - as I had in the past used the packed attribute as here to define that we don't want holes. (just as this existing code does) As the documentation seems to determine that this isn't the effect of this attribute, and removing it has no effect - I suspect removing it is the right thing to do, as otherwise we are simply mis-using the construct for no purpose. It's up to though. Drop or accept as you feel is right. > >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > You forget to pick Geert's review tag. Ah yes, Collected thanks -- Regards Kieran > >> --- >> v2 >> - Remove attributes entirely >> --- >> drivers/media/platform/vsp1/vsp1_dl.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c >> b/drivers/media/platform/vsp1/vsp1_dl.c index c7fa1cb088cd..f4cede9b9b43 >> 100644 >> --- a/drivers/media/platform/vsp1/vsp1_dl.c >> +++ b/drivers/media/platform/vsp1/vsp1_dl.c >> @@ -25,19 +25,19 @@ >> struct vsp1_dl_header_list { >> u32 num_bytes; >> u32 addr; >> -} __attribute__((__packed__)); >> +}; >> >> struct vsp1_dl_header { >> u32 num_lists; >> struct vsp1_dl_header_list lists[8]; >> u32 next_header; >> u32 flags; >> -} __attribute__((__packed__)); >> +}; >> >> struct vsp1_dl_entry { >> u32 addr; >> u32 data; >> -} __attribute__((__packed__)); >> +}; >> >> /** >> * struct vsp1_dl_body - Display list body >
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c index c7fa1cb088cd..f4cede9b9b43 100644 --- a/drivers/media/platform/vsp1/vsp1_dl.c +++ b/drivers/media/platform/vsp1/vsp1_dl.c @@ -25,19 +25,19 @@ struct vsp1_dl_header_list { u32 num_bytes; u32 addr; -} __attribute__((__packed__)); +}; struct vsp1_dl_header { u32 num_lists; struct vsp1_dl_header_list lists[8]; u32 next_header; u32 flags; -} __attribute__((__packed__)); +}; struct vsp1_dl_entry { u32 addr; u32 data; -} __attribute__((__packed__)); +}; /** * struct vsp1_dl_body - Display list body