Cleanup of -Wunused-const-variable in drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c

Cleanup of -Wunused-const-variable in drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c

[Nathan Huckleberry]

Hey all,

I'm looking into cleaning up ignored warnings in the kernel so we can
remove compiler flags to ignore warnings.

There are several unused variables in dpu_formats.c
('dpu_format_map_tile', 'dpu_format_map_p010',
'dpu_format_map_p010_ubwc', 'dpu_format_map_tp10_ubwc').
They look like modifiers that were never implemented. I'd like to
remove these variables if there are no plans moving forward to
implement them. Otherwise I'll just leave them.

https://github.com/ClangBuiltLinux/linux/issues/528

Thanks,
Nathan Huckleberry

Re: Cleanup of -Wunused-const-variable in drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c

[Sean Paul]

On Thu, Jun 13, 2019 at 4:13 PM Nathan Huckleberry <nhuck@google.com> wrote:
>
> Hey all,
>
> I'm looking into cleaning up ignored warnings in the kernel so we can
> remove compiler flags to ignore warnings.
>
> There are several unused variables in dpu_formats.c
> ('dpu_format_map_tile', 'dpu_format_map_p010',
> 'dpu_format_map_p010_ubwc', 'dpu_format_map_tp10_ubwc').
> They look like modifiers that were never implemented. I'd like to
> remove these variables if there are no plans moving forward to
> implement them. Otherwise I'll just leave them.

We can probably remove them for now and if someone wants to add
support, they can dredge them back up.

Sean


>
> https://github.com/ClangBuiltLinux/linux/issues/528
>
> Thanks,
> Nathan Huckleberry

Re: Cleanup of -Wunused-const-variable in drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c

[Rob Clark]

so, for dpu_format_map_tile, I'd like to define a fourcc modifier for
tiled formats (we currently have a workaround in userspace w/ a
private modifier in the gallium driver).. I think the problem is
defining the layout of the tiled format(s) (there are at least two per
generation and I can't guarantee they are the same across adreno
generations).  We've mostly avoided needing to know the exact layout
by using gpu blits to go from tiled<->linear so far.

For the others, those look like formats we haven't wired up yet.

I'd say they are all things we want to support eventually, although
not sure what the timeline will be..  but I'd ask if you remove them
then split into at least a separate patch for dpu_format_map_tile vs
others, so we can more easily revert/amend to bring them back.

BR,
-R

On Thu, Jun 13, 2019 at 1:13 PM Nathan Huckleberry <nhuck@google.com> wrote:
>
> Hey all,
>
> I'm looking into cleaning up ignored warnings in the kernel so we can
> remove compiler flags to ignore warnings.
>
> There are several unused variables in dpu_formats.c
> ('dpu_format_map_tile', 'dpu_format_map_p010',
> 'dpu_format_map_p010_ubwc', 'dpu_format_map_tp10_ubwc').
> They look like modifiers that were never implemented. I'd like to
> remove these variables if there are no plans moving forward to
> implement them. Otherwise I'll just leave them.
>
> https://github.com/ClangBuiltLinux/linux/issues/528
>
> Thanks,
> Nathan Huckleberry

Re: Cleanup of -Wunused-const-variable in drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c

[Daniel Vetter]

On Thu, Jun 13, 2019 at 01:52:02PM -0700, Rob Clark wrote:
> so, for dpu_format_map_tile, I'd like to define a fourcc modifier for
> tiled formats (we currently have a workaround in userspace w/ a
> private modifier in the gallium driver).. I think the problem is
> defining the layout of the tiled format(s) (there are at least two per
> generation and I can't guarantee they are the same across adreno
> generations).  We've mostly avoided needing to know the exact layout
> by using gpu blits to go from tiled<->linear so far.
> 
> For the others, those look like formats we haven't wired up yet.
> 
> I'd say they are all things we want to support eventually, although
> not sure what the timeline will be..  but I'd ask if you remove them
> then split into at least a separate patch for dpu_format_map_tile vs
> others, so we can more easily revert/amend to bring them back.

We've been kinda cheating on this with i915 modifiers too, X/Y tiled
depend upon the chip you're running on :-) But on all modern chips it's a
lot more well-defined, so probably not a huge problem.

I think there's little chance right now for adreno to become a discrete
gpu, so could do the same tricky. If it ever becomes discrete or we want
to share more, then we'd need to bake in the layout properly I think.

I guess the question is: Is the format shared with e.g. camera blocks, or
purely a freedreno internal thing.
-Daniel

> 
> BR,
> -R
> 
> On Thu, Jun 13, 2019 at 1:13 PM Nathan Huckleberry <nhuck@google.com> wrote:
> >
> > Hey all,
> >
> > I'm looking into cleaning up ignored warnings in the kernel so we can
> > remove compiler flags to ignore warnings.
> >
> > There are several unused variables in dpu_formats.c
> > ('dpu_format_map_tile', 'dpu_format_map_p010',
> > 'dpu_format_map_p010_ubwc', 'dpu_format_map_tp10_ubwc').
> > They look like modifiers that were never implemented. I'd like to
> > remove these variables if there are no plans moving forward to
> > implement them. Otherwise I'll just leave them.
> >
> > https://github.com/ClangBuiltLinux/linux/issues/528
> >
> > Thanks,
> > Nathan Huckleberry

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: Cleanup of -Wunused-const-variable in drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c

[Jani Nikula]

On Thu, 13 Jun 2019, Nathan Huckleberry <nhuck@google.com> wrote:
> Hey all,
>
> I'm looking into cleaning up ignored warnings in the kernel so we can
> remove compiler flags to ignore warnings.

Wholeheartedly agreed on the goal.

> There are several unused variables in dpu_formats.c
> ('dpu_format_map_tile', 'dpu_format_map_p010',
> 'dpu_format_map_p010_ubwc', 'dpu_format_map_tp10_ubwc').
> They look like modifiers that were never implemented. I'd like to
> remove these variables if there are no plans moving forward to
> implement them. Otherwise I'll just leave them.
>
> https://github.com/ClangBuiltLinux/linux/issues/528

No opinion on the said variables above, but, FWIW, personally I think
it's fine to add the cflags to supress warnings locally where needed in
order to be able to achieve the greater goal of removing the cflags
globally.

In drivers/gpu/drm/i915/Makefile we actually go for much stricter
warnings than the kernel defaults, and disable a more limited and
tailored set of warnings.

You can do this both on a subdir and file level with subdir-ccflags-y
and CFLAGS_filename.o, respectively.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: Cleanup of -Wunused-const-variable in drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c

[Nick Desaulniers]

On Thu, Jun 13, 2019 at 1:50 PM Sean Paul <sean@poorly.run> wrote:
>
> On Thu, Jun 13, 2019 at 4:13 PM Nathan Huckleberry <nhuck@google.com> wrote:
> >
> > Hey all,
> >
> > I'm looking into cleaning up ignored warnings in the kernel so we can
> > remove compiler flags to ignore warnings.
> >
> > There are several unused variables in dpu_formats.c
> > ('dpu_format_map_tile', 'dpu_format_map_p010',
> > 'dpu_format_map_p010_ubwc', 'dpu_format_map_tp10_ubwc').
> > They look like modifiers that were never implemented. I'd like to
> > remove these variables if there are no plans moving forward to
> > implement them. Otherwise I'll just leave them.
>
> We can probably remove them for now and if someone wants to add
> support, they can dredge them back up.

Yep, this has been the feedback for other patches for this warning
when the code was dead or not obviously some kind of
bug/typo/copy-pasta.

Nathan, please submit a patch removing the dead code; it may be
reverted later when it's actually wired up.  Nothing is truly lost w/
git*.
-- 
Thanks,
~Nick Desaulniers

Re: Cleanup of -Wunused-const-variable in drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c

[Nick Desaulniers]

On Fri, Jun 14, 2019 at 2:43 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> No opinion on the said variables above, but, FWIW, personally I think
> it's fine to add the cflags to supress warnings locally where needed in
> order to be able to achieve the greater goal of removing the cflags
> globally.

I think there's on the order of ~10 of these:
https://github.com/ClangBuiltLinux/linux/issues?q=is%3Aissue+is%3Aopen+label%3A-Wunused-const-variable
Nathan's got a pretty good handle on just fixing them.

> In drivers/gpu/drm/i915/Makefile we actually go for much stricter
> warnings than the kernel defaults, and disable a more limited and
> tailored set of warnings.

I like this.

>
> You can do this both on a subdir and file level with subdir-ccflags-y
> and CFLAGS_filename.o, respectively.

That said, I have used this trick before, but I feel like the fewer
people that know about this trick, the less it can be [ab]used.
-- 
Thanks,
~Nick Desaulniers

[PATCH] drm/msm/dpu: Fix Wunused-const-variable

[Nathan Huckleberry]

Clang produces the following warning

drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c:477:32: warning: unused
variable 'dpu_format_map_tile' [-Wunused-const-variable] static const
struct dpu_format dpu_format_map_tile[] = { ^

drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c:602:32: warning: unused
variable 'dpu_format_map_p010' [-Wunused-const-variable] static const
struct dpu_format dpu_format_map_p010[] = { ^

drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c:610:32: warning: unused
variable 'dpu_format_map_p010_ubwc' [-Wunused-const-variable] static
const struct dpu_format dpu_format_map_p010_ubwc[] = { ^

drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c:619:32: warning: unused
variable 'dpu_format_map_tp10_ubwc' [-Wunused-const-variable] static
const struct dpu_format dpu_format_map_tp10_ubwc[] = { ^

Removing the unimplemented modifiers that cause the warning.

Cc: clang-built-linux@googlegroups.com
Link: https://github.com/ClangBuiltLinux/linux/issues/528
Signed-off-by: Nathan Huckleberry <nhuck@google.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 110 --------------------
 1 file changed, 110 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
index 0440696b5bad..d28520faf157 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
@@ -470,90 +470,6 @@ static const struct dpu_format dpu_format_map[] = {
 		DPU_FETCH_LINEAR, 3),
 };
 
-/*
- * A5x tile formats tables:
- * These tables hold the A5x tile formats supported.
- */
-static const struct dpu_format dpu_format_map_tile[] = {
-	INTERLEAVED_RGB_FMT_TILED(BGR565,
-		0, COLOR_5BIT, COLOR_6BIT, COLOR_5BIT,
-		C2_R_Cr, C0_G_Y, C1_B_Cb, 0, 3,
-		false, 2, 0,
-		DPU_FETCH_UBWC, 1, DPU_TILE_HEIGHT_TILED),
-
-	INTERLEAVED_RGB_FMT_TILED(ARGB8888,
-		COLOR_8BIT, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C3_ALPHA, C2_R_Cr, C0_G_Y, C1_B_Cb, 4,
-		true, 4, 0,
-		DPU_FETCH_UBWC, 1, DPU_TILE_HEIGHT_TILED),
-
-	INTERLEAVED_RGB_FMT_TILED(ABGR8888,
-		COLOR_8BIT, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C3_ALPHA, C1_B_Cb, C0_G_Y, C2_R_Cr, 4,
-		true, 4, 0,
-		DPU_FETCH_UBWC, 1, DPU_TILE_HEIGHT_TILED),
-
-	INTERLEAVED_RGB_FMT_TILED(XBGR8888,
-		COLOR_8BIT, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C2_R_Cr, C0_G_Y, C1_B_Cb, C3_ALPHA, 4,
-		false, 4, 0,
-		DPU_FETCH_UBWC, 1, DPU_TILE_HEIGHT_TILED),
-
-	INTERLEAVED_RGB_FMT_TILED(RGBA8888,
-		COLOR_8BIT, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C2_R_Cr, C0_G_Y, C1_B_Cb, C3_ALPHA, 4,
-		true, 4, 0,
-		DPU_FETCH_UBWC, 1, DPU_TILE_HEIGHT_TILED),
-
-	INTERLEAVED_RGB_FMT_TILED(BGRA8888,
-		COLOR_8BIT, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C1_B_Cb, C0_G_Y, C2_R_Cr, C3_ALPHA, 4,
-		true, 4, 0,
-		DPU_FETCH_UBWC, 1, DPU_TILE_HEIGHT_TILED),
-
-	INTERLEAVED_RGB_FMT_TILED(BGRX8888,
-		COLOR_8BIT, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C1_B_Cb, C0_G_Y, C2_R_Cr, C3_ALPHA, 4,
-		false, 4, 0,
-		DPU_FETCH_UBWC, 1, DPU_TILE_HEIGHT_TILED),
-
-	INTERLEAVED_RGB_FMT_TILED(XRGB8888,
-		COLOR_8BIT, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C3_ALPHA, C2_R_Cr, C0_G_Y, C1_B_Cb, 4,
-		false, 4, 0,
-		DPU_FETCH_UBWC, 1, DPU_TILE_HEIGHT_TILED),
-
-	INTERLEAVED_RGB_FMT_TILED(RGBX8888,
-		COLOR_8BIT, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C2_R_Cr, C0_G_Y, C1_B_Cb, C3_ALPHA, 4,
-		false, 4, 0,
-		DPU_FETCH_UBWC, 1, DPU_TILE_HEIGHT_TILED),
-
-	INTERLEAVED_RGB_FMT_TILED(ABGR2101010,
-		COLOR_8BIT, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C2_R_Cr, C0_G_Y, C1_B_Cb, C3_ALPHA, 4,
-		true, 4, DPU_FORMAT_FLAG_DX,
-		DPU_FETCH_UBWC, 1, DPU_TILE_HEIGHT_TILED),
-
-	INTERLEAVED_RGB_FMT_TILED(XBGR2101010,
-		COLOR_8BIT, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C2_R_Cr, C0_G_Y, C1_B_Cb, C3_ALPHA, 4,
-		true, 4, DPU_FORMAT_FLAG_DX,
-		DPU_FETCH_UBWC, 1, DPU_TILE_HEIGHT_TILED),
-
-	PSEUDO_YUV_FMT_TILED(NV12,
-		0, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C1_B_Cb, C2_R_Cr,
-		DPU_CHROMA_420, DPU_FORMAT_FLAG_YUV,
-		DPU_FETCH_UBWC, 2, DPU_TILE_HEIGHT_NV12),
-
-	PSEUDO_YUV_FMT_TILED(NV21,
-		0, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C2_R_Cr, C1_B_Cb,
-		DPU_CHROMA_420, DPU_FORMAT_FLAG_YUV,
-		DPU_FETCH_UBWC, 2, DPU_TILE_HEIGHT_NV12),
-};
-
 /*
  * UBWC formats table:
  * This table holds the UBWC formats supported.
@@ -599,32 +515,6 @@ static const struct dpu_format dpu_format_map_ubwc[] = {
 		DPU_FETCH_UBWC, 4, DPU_TILE_HEIGHT_NV12),
 };
 
-static const struct dpu_format dpu_format_map_p010[] = {
-	PSEUDO_YUV_FMT_LOOSE(NV12,
-		0, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C1_B_Cb, C2_R_Cr,
-		DPU_CHROMA_420, (DPU_FORMAT_FLAG_YUV | DPU_FORMAT_FLAG_DX),
-		DPU_FETCH_LINEAR, 2),
-};
-
-static const struct dpu_format dpu_format_map_p010_ubwc[] = {
-	PSEUDO_YUV_FMT_LOOSE_TILED(NV12,
-		0, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C1_B_Cb, C2_R_Cr,
-		DPU_CHROMA_420, (DPU_FORMAT_FLAG_YUV | DPU_FORMAT_FLAG_DX |
-				DPU_FORMAT_FLAG_COMPRESSED),
-		DPU_FETCH_UBWC, 4, DPU_TILE_HEIGHT_NV12),
-};
-
-static const struct dpu_format dpu_format_map_tp10_ubwc[] = {
-	PSEUDO_YUV_FMT_TILED(NV12,
-		0, COLOR_8BIT, COLOR_8BIT, COLOR_8BIT,
-		C1_B_Cb, C2_R_Cr,
-		DPU_CHROMA_420, (DPU_FORMAT_FLAG_YUV | DPU_FORMAT_FLAG_DX |
-				DPU_FORMAT_FLAG_COMPRESSED),
-		DPU_FETCH_UBWC, 4, DPU_TILE_HEIGHT_NV12),
-};
-
 /* _dpu_get_v_h_subsample_rate - Get subsample rates for all formats we support
  *   Note: Not using the drm_format_*_subsampling since we have formats
  */
-- 
2.22.0.410.gd8fdbe21b5-goog