[PATCH v2 0/4] Fix most ImgU driver compiler / checker warnings

[PATCH v2 0/4] Fix most ImgU driver compiler / checker warnings

[Sakari Ailus]

Hi Mauro, Hans,                                                                 
                                                                                
This set addresses most of the compiler / checker warnings from the ImgU        
driver. There are a few about stack usage from the compiler left but those      
were not trivial to address so I postponed that. There are also a few           
about memset but Hans suggested addressing that by increasing the maximum       
size. I guess it wouldn't benefit anyone to split those memsets into            
two...

since v2:

- Drop __attribute__((aligned())) -> __aligned() change for now

- Align awb_fr field in struct ipu3_uapi_acc_param

- Add Raj's Tested-by: tag

Sakari Ailus (4):
  staging: imgu: Address a compiler warning on alignment
  staging: imgu: Remove redundant checks
  staging: imgu: Address compiler / checker warnings in MMU code
  Revert "media: ipu3: shut up warnings produced with W=1"

 drivers/staging/media/ipu3/Makefile             |  6 ----
 drivers/staging/media/ipu3/TODO                 |  2 --
 drivers/staging/media/ipu3/include/intel-ipu3.h |  2 +-
 drivers/staging/media/ipu3/ipu3-css-fw.c        |  6 ++--
 drivers/staging/media/ipu3/ipu3-mmu.c           | 39 ++++++++++++++++++++++---
 5 files changed, 38 insertions(+), 17 deletions(-)

-- 
2.11.0

[PATCH v2 1/4] staging: imgu: Address a compiler warning on alignment

[Sakari Ailus]

Address a compiler warnings on alignment of struct ipu3_uapi_awb_fr_config_s
by adding __attribute__((aligned(32))) to a struct member of that type as
well.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Tested-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
 drivers/staging/media/ipu3/include/intel-ipu3.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
index eb6f52aca9929..4a0e97b0cfd2b 100644
--- a/drivers/staging/media/ipu3/include/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
@@ -2472,7 +2472,7 @@ struct ipu3_uapi_acc_param {
 	struct ipu3_uapi_yuvp1_yds_config yds2 __attribute__((aligned(32)));
 	struct ipu3_uapi_yuvp2_tcc_static_config tcc __attribute__((aligned(32)));
 	struct ipu3_uapi_anr_config anr;
-	struct ipu3_uapi_awb_fr_config_s awb_fr;
+	struct ipu3_uapi_awb_fr_config_s awb_fr __attribute__((aligned(32)));
 	struct ipu3_uapi_ae_config ae;
 	struct ipu3_uapi_af_config_s af;
 	struct ipu3_uapi_awb_config awb;
-- 
2.11.0

[PATCH v2 2/4] staging: imgu: Remove redundant checks

[Sakari Ailus]

Remove redundant checks for less than zero on unsigned variables.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Tested-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
 drivers/staging/media/ipu3/ipu3-css-fw.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
index 4122d4e42db6c..45aff76198e2c 100644
--- a/drivers/staging/media/ipu3/ipu3-css-fw.c
+++ b/drivers/staging/media/ipu3/ipu3-css-fw.c
@@ -200,13 +200,11 @@ int imgu_css_fw_init(struct imgu_css *css)
 			goto bad_fw;
 
 		for (j = 0; j < bi->info.isp.num_output_formats; j++)
-			if (bi->info.isp.output_formats[j] < 0 ||
-			    bi->info.isp.output_formats[j] >=
+			if (bi->info.isp.output_formats[j] >=
 			    IMGU_ABI_FRAME_FORMAT_NUM)
 				goto bad_fw;
 		for (j = 0; j < bi->info.isp.num_vf_formats; j++)
-			if (bi->info.isp.vf_formats[j] < 0 ||
-			    bi->info.isp.vf_formats[j] >=
+			if (bi->info.isp.vf_formats[j] >=
 			    IMGU_ABI_FRAME_FORMAT_NUM)
 				goto bad_fw;
 
-- 
2.11.0

[PATCH v2 3/4] staging: imgu: Address compiler / checker warnings in MMU code

[Sakari Ailus]

Address C compiler, sparse and smatch warnings and little style issues in
the IMGU MMU code.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Tested-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
 drivers/staging/media/ipu3/ipu3-mmu.c | 39 +++++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/ipu3/ipu3-mmu.c b/drivers/staging/media/ipu3/ipu3-mmu.c
index cfc2bdfb14b33..a55ff39be1882 100644
--- a/drivers/staging/media/ipu3/ipu3-mmu.c
+++ b/drivers/staging/media/ipu3/ipu3-mmu.c
@@ -275,7 +275,17 @@ static size_t imgu_mmu_pgsize(unsigned long pgsize_bitmap,
 	return pgsize;
 }
 
-/* drivers/iommu/iommu.c/iommu_map() */
+/**
+ * imgu_mmu_map - map a buffer to a physical address
+ *
+ * @info: MMU mappable range
+ * @iova: the virtual address
+ * @paddr: the physical address
+ * @size: length of the mappable area
+ *
+ * The function has been adapted from iommu_map() in
+ * drivers/iommu/iommu.c .
+ */
 int imgu_mmu_map(struct imgu_mmu_info *info, unsigned long iova,
 		 phys_addr_t paddr, size_t size)
 {
@@ -321,7 +331,17 @@ int imgu_mmu_map(struct imgu_mmu_info *info, unsigned long iova,
 	return ret;
 }
 
-/* drivers/iommu/iommu.c/default_iommu_map_sg() */
+/**
+ * imgu_mmu_map_sg - Map a scatterlist
+ *
+ * @info: MMU mappable range
+ * @iova: the virtual address
+ * @sg: the scatterlist to map
+ * @nents: number of entries in the scatterlist
+ *
+ * The function has been adapted from default_iommu_map_sg() in
+ * drivers/iommu/iommu.c .
+ */
 size_t imgu_mmu_map_sg(struct imgu_mmu_info *info, unsigned long iova,
 		       struct scatterlist *sg, unsigned int nents)
 {
@@ -394,7 +414,16 @@ static size_t __imgu_mmu_unmap(struct imgu_mmu *mmu,
 	return unmap;
 }
 
-/* drivers/iommu/iommu.c/iommu_unmap() */
+/**
+ * imgu_mmu_unmap - Unmap a buffer
+ *
+ * @info: MMU mappable range
+ * @iova: the virtual address
+ * @size: the length of the buffer
+ *
+ * The function has been adapted from iommu_unmap() in
+ * drivers/iommu/iommu.c .
+ */
 size_t imgu_mmu_unmap(struct imgu_mmu_info *info, unsigned long iova,
 		      size_t size)
 {
@@ -444,6 +473,7 @@ size_t imgu_mmu_unmap(struct imgu_mmu_info *info, unsigned long iova,
 
 /**
  * imgu_mmu_init() - initialize IPU3 MMU block
+ *
  * @parent:	struct device parent
  * @base:	IOMEM base of hardware registers.
  *
@@ -523,7 +553,8 @@ struct imgu_mmu_info *imgu_mmu_init(struct device *parent, void __iomem *base)
 
 /**
  * imgu_mmu_exit() - clean up IPU3 MMU block
- * @info: IPU3 MMU private data
+ *
+ * @info: MMU mappable range
  */
 void imgu_mmu_exit(struct imgu_mmu_info *info)
 {
-- 
2.11.0

[PATCH v2 4/4] Revert "media: ipu3: shut up warnings produced with W=1"

[Sakari Ailus]

This reverts commit 0bdfc56c13c0ffe003f28395fcde2cd9b5ea0622.

The original patch suppressed compiler and checker warnings and those
warnings have been addressed. Do not attempt to suppress these warnings
anymore.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Tested-by: Rajmohan Mani <rajmohan.mani@intel.com>
---
 drivers/staging/media/ipu3/Makefile | 6 ------
 drivers/staging/media/ipu3/TODO     | 2 --
 2 files changed, 8 deletions(-)

diff --git a/drivers/staging/media/ipu3/Makefile b/drivers/staging/media/ipu3/Makefile
index fa7fa3372bcbe..fb146d178bd4b 100644
--- a/drivers/staging/media/ipu3/Makefile
+++ b/drivers/staging/media/ipu3/Makefile
@@ -9,9 +9,3 @@ ipu3-imgu-objs += \
 		ipu3-css.o ipu3-v4l2.o ipu3.o
 
 obj-$(CONFIG_VIDEO_IPU3_IMGU) += ipu3-imgu.o
-
-# HACK! While this driver is in bad shape, don't enable several warnings
-#       that would be otherwise enabled with W=1
-ccflags-y += $(call cc-disable-warning, packed-not-aligned)
-ccflags-y += $(call cc-disable-warning, type-limits)
-ccflags-y += $(call cc-disable-warning, unused-const-variable)
diff --git a/drivers/staging/media/ipu3/TODO b/drivers/staging/media/ipu3/TODO
index 5e55baeaea1a3..8b95e74e43a0f 100644
--- a/drivers/staging/media/ipu3/TODO
+++ b/drivers/staging/media/ipu3/TODO
@@ -27,5 +27,3 @@ staging directory.
 - Document different operation modes, and which buffer queues are relevant
   in each mode. To process an image, which queues require a buffer an in
   which ones is it optional?
-
-- Make sure it builds fine with no warnings with W=1
-- 
2.11.0

Re: [PATCH v2 1/4] staging: imgu: Address a compiler warning on alignment

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

Hi Sakari,

On Fri, Mar 01, 2019 at 01:23:57PM +0200, Sakari Ailus wrote:
> Address a compiler warnings on alignment of struct ipu3_uapi_awb_fr_config_s
> by adding __attribute__((aligned(32))) to a struct member of that type as
> well.
>

Sorry, just noticed there was a v2.

This is how we momentary worked it around in libcamera, so my tag on
v1 also applies here too :)

Could I know why this is preferred compared to v1? Has the field to be
actually 32 bytes aligned?

Thanks
  j
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Tested-by: Rajmohan Mani <rajmohan.mani@intel.com>
> ---
>  drivers/staging/media/ipu3/include/intel-ipu3.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> index eb6f52aca9929..4a0e97b0cfd2b 100644
> --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> @@ -2472,7 +2472,7 @@ struct ipu3_uapi_acc_param {
>  	struct ipu3_uapi_yuvp1_yds_config yds2 __attribute__((aligned(32)));
>  	struct ipu3_uapi_yuvp2_tcc_static_config tcc __attribute__((aligned(32)));
>  	struct ipu3_uapi_anr_config anr;
> -	struct ipu3_uapi_awb_fr_config_s awb_fr;
> +	struct ipu3_uapi_awb_fr_config_s awb_fr __attribute__((aligned(32)));
>  	struct ipu3_uapi_ae_config ae;
>  	struct ipu3_uapi_af_config_s af;
>  	struct ipu3_uapi_awb_config awb;
> --
> 2.11.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] staging: imgu: Address a compiler warning on alignment

[Sakari Ailus]

Hi Jacopo,

On Mon, May 27, 2019 at 06:44:41PM +0200, Jacopo Mondi wrote:
> Hi Sakari,
> 
> On Fri, Mar 01, 2019 at 01:23:57PM +0200, Sakari Ailus wrote:
> > Address a compiler warnings on alignment of struct ipu3_uapi_awb_fr_config_s
> > by adding __attribute__((aligned(32))) to a struct member of that type as
> > well.
> >
> 
> Sorry, just noticed there was a v2.
> 
> This is how we momentary worked it around in libcamera, so my tag on
> v1 also applies here too :)
> 
> Could I know why this is preferred compared to v1? Has the field to be
> actually 32 bytes aligned?

Not because of IPU3 DMA, but to maintain ABI compatibility. Technically
there's no strict need to do that as this is a staging driver. This patch
simply fixes the warning. Later on it'd be good to remove the alignment
where they are unnecessary.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 1/4] staging: imgu: Address a compiler warning on alignment

[Tomasz Figa]

Hi Sakari,

On Fri, Mar 1, 2019 at 12:24 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Address a compiler warnings on alignment of struct ipu3_uapi_awb_fr_config_s
> by adding __attribute__((aligned(32))) to a struct member of that type as
> well.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Tested-by: Rajmohan Mani <rajmohan.mani@intel.com>
> ---
>  drivers/staging/media/ipu3/include/intel-ipu3.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> index eb6f52aca9929..4a0e97b0cfd2b 100644
> --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> @@ -2472,7 +2472,7 @@ struct ipu3_uapi_acc_param {
>         struct ipu3_uapi_yuvp1_yds_config yds2 __attribute__((aligned(32)));
>         struct ipu3_uapi_yuvp2_tcc_static_config tcc __attribute__((aligned(32)));
>         struct ipu3_uapi_anr_config anr;
> -       struct ipu3_uapi_awb_fr_config_s awb_fr;
> +       struct ipu3_uapi_awb_fr_config_s awb_fr __attribute__((aligned(32)));
>         struct ipu3_uapi_ae_config ae;
>         struct ipu3_uapi_af_config_s af;
>         struct ipu3_uapi_awb_config awb;
> --
> 2.11.0
>

I don't know how this patch was tested, but for me it breaks the
layout of the ipu3_uapi_acc_param struct. The result is that awb_fr is
moved from offset 37656 to 36768 and so everything else is shifted
too. The end result is "set parameters failed" when the userspace
tries to queue a parameter buffer.

To be honest, I don't like how alignments are used to define the ABI
layout. Since these are definitions specified by the IMGU firmware,
I'd suggest removing any compiler alignments, making all the structs
packed and adding explicit padding in terms of u8 arrays, which is the
proper way to enforce a specific binary layout.

Following is the relevant snippet from a diff of pahole output before
and after this patch:

--- pahole.good      2020-04-11 16:40:18.706133867 +0000
+++ pahole.bad      2020-04-11 16:29:48.513110117 +0000
@@ -7353,29 +7349,27 @@ // struct ipu3_uapi_acc_param {
        struct ipu3_uapi_anr_config anr;                 /* 36020   736 */

        /* XXX last struct has 24 bytes of padding */
+       /* XXX 12 bytes hole, try to pack */

-       /* --- cacheline 574 boundary (36736 bytes) was 20 bytes ago --- */
-       struct ipu3_uapi_awb_fr_config_s awb_fr
__attribute__((__aligned__(32))); /* 36756    32 */
-       struct ipu3_uapi_ae_config ae;                   /* 36788   480 */
+       /* --- cacheline 574 boundary (36736 bytes) was 32 bytes ago --- */
+       struct ipu3_uapi_awb_fr_config_s awb_fr
__attribute__((__aligned__(32))); /* 36768    32 */
+       /* --- cacheline 575 boundary (36800 bytes) --- */
+       struct ipu3_uapi_ae_config ae;                   /* 36800   480 */

        /* XXX last struct has 24 bytes of padding */

-       /* --- cacheline 582 boundary (37248 bytes) was 20 bytes ago --- */
-       struct ipu3_uapi_af_config_s af;                 /* 37268    96 */
+       /* --- cacheline 582 boundary (37248 bytes) was 32 bytes ago --- */
+       struct ipu3_uapi_af_config_s af;                 /* 37280    96 */

        /* XXX last struct has 20 bytes of padding */

-       /* --- cacheline 583 boundary (37312 bytes) was 52 bytes ago --- */
-       struct ipu3_uapi_awb_config awb;                 /* 37364    32 */
-
-       /* Force padding: */
-       struct ipu3_uapi_awb_config :256;
+       /* --- cacheline 584 boundary (37376 bytes) --- */
+       struct ipu3_uapi_awb_config awb;                 /* 37376    32 */

        /* size: 37408, cachelines: 585, members: 21 */
-       /* sum members: 37152, holes: 13, sum holes: 244 */
-       /* padding: 12 */
+       /* sum members: 37152, holes: 14, sum holes: 256 */
        /* paddings: 3, sum paddings: 68 */
-       /* forced alignments: 16, forced holes: 13, sum forced holes: 244 */
+       /* forced alignments: 16, forced holes: 14, sum forced holes: 256 */
        /* last cacheline: 32 bytes */
 };

Best regards,
Tomasz

Re: [PATCH v2 1/4] staging: imgu: Address a compiler warning on alignment

[Sakari Ailus]

Hi Tomasz,

On Sat, Apr 11, 2020 at 06:50:55PM +0200, Tomasz Figa wrote:
> Hi Sakari,
> 
> On Fri, Mar 1, 2019 at 12:24 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Address a compiler warnings on alignment of struct ipu3_uapi_awb_fr_config_s
> > by adding __attribute__((aligned(32))) to a struct member of that type as
> > well.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Tested-by: Rajmohan Mani <rajmohan.mani@intel.com>
> > ---
> >  drivers/staging/media/ipu3/include/intel-ipu3.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > index eb6f52aca9929..4a0e97b0cfd2b 100644
> > --- a/drivers/staging/media/ipu3/include/intel-ipu3.h
> > +++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
> > @@ -2472,7 +2472,7 @@ struct ipu3_uapi_acc_param {
> >         struct ipu3_uapi_yuvp1_yds_config yds2 __attribute__((aligned(32)));
> >         struct ipu3_uapi_yuvp2_tcc_static_config tcc __attribute__((aligned(32)));
> >         struct ipu3_uapi_anr_config anr;
> > -       struct ipu3_uapi_awb_fr_config_s awb_fr;
> > +       struct ipu3_uapi_awb_fr_config_s awb_fr __attribute__((aligned(32)));
> >         struct ipu3_uapi_ae_config ae;
> >         struct ipu3_uapi_af_config_s af;
> >         struct ipu3_uapi_awb_config awb;
> > --
> > 2.11.0
> >
> 
> I don't know how this patch was tested, but for me it breaks the
> layout of the ipu3_uapi_acc_param struct. The result is that awb_fr is
> moved from offset 37656 to 36768 and so everything else is shifted
> too. The end result is "set parameters failed" when the userspace
> tries to queue a parameter buffer.

Thanks for reporting this.

Unlike recent patches touching the struct, I didn't test (obviously) this
one with pahole.

> 
> To be honest, I don't like how alignments are used to define the ABI
> layout. Since these are definitions specified by the IMGU firmware,
> I'd suggest removing any compiler alignments, making all the structs
> packed and adding explicit padding in terms of u8 arrays, which is the
> proper way to enforce a specific binary layout.

The current use of __aligned(32) with __packed especially makes this
cumbersome and risky. __packed alone is necessary.

I'll send patches to address the alignment issue; converting to use
reserved fields would be quite a bit more work.

Similar changes should be done to ipu3-abi.h as well, but it appears to
have been less problematic so far.

> 
> Following is the relevant snippet from a diff of pahole output before
> and after this patch:
> 
> --- pahole.good      2020-04-11 16:40:18.706133867 +0000
> +++ pahole.bad      2020-04-11 16:29:48.513110117 +0000
> @@ -7353,29 +7349,27 @@ // struct ipu3_uapi_acc_param {
>         struct ipu3_uapi_anr_config anr;                 /* 36020   736 */
> 
>         /* XXX last struct has 24 bytes of padding */
> +       /* XXX 12 bytes hole, try to pack */
> 
> -       /* --- cacheline 574 boundary (36736 bytes) was 20 bytes ago --- */
> -       struct ipu3_uapi_awb_fr_config_s awb_fr
> __attribute__((__aligned__(32))); /* 36756    32 */
> -       struct ipu3_uapi_ae_config ae;                   /* 36788   480 */
> +       /* --- cacheline 574 boundary (36736 bytes) was 32 bytes ago --- */
> +       struct ipu3_uapi_awb_fr_config_s awb_fr
> __attribute__((__aligned__(32))); /* 36768    32 */
> +       /* --- cacheline 575 boundary (36800 bytes) --- */
> +       struct ipu3_uapi_ae_config ae;                   /* 36800   480 */
> 
>         /* XXX last struct has 24 bytes of padding */
> 
> -       /* --- cacheline 582 boundary (37248 bytes) was 20 bytes ago --- */
> -       struct ipu3_uapi_af_config_s af;                 /* 37268    96 */
> +       /* --- cacheline 582 boundary (37248 bytes) was 32 bytes ago --- */
> +       struct ipu3_uapi_af_config_s af;                 /* 37280    96 */
> 
>         /* XXX last struct has 20 bytes of padding */
> 
> -       /* --- cacheline 583 boundary (37312 bytes) was 52 bytes ago --- */
> -       struct ipu3_uapi_awb_config awb;                 /* 37364    32 */
> -
> -       /* Force padding: */
> -       struct ipu3_uapi_awb_config :256;
> +       /* --- cacheline 584 boundary (37376 bytes) --- */
> +       struct ipu3_uapi_awb_config awb;                 /* 37376    32 */
> 
>         /* size: 37408, cachelines: 585, members: 21 */
> -       /* sum members: 37152, holes: 13, sum holes: 244 */
> -       /* padding: 12 */
> +       /* sum members: 37152, holes: 14, sum holes: 256 */
>         /* paddings: 3, sum paddings: 68 */
> -       /* forced alignments: 16, forced holes: 13, sum forced holes: 244 */
> +       /* forced alignments: 16, forced holes: 14, sum forced holes: 256 */
>         /* last cacheline: 32 bytes */
>  };
> 

-- 
Regards,

Sakari Ailus