Re: [PATCH 31/32] drm: Nuke fb->bits_per_pixel

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Ville,

On Thursday 17 Nov 2016 20:35:46 Ville Syrjälä wrote:
> On Thu, Nov 17, 2016 at 08:14:16PM +0200, Laurent Pinchart wrote:
> > On Thursday 17 Nov 2016 18:14:30 ville.syrjala@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> 
> >> Replace uses of fb->bits_per_pixel with fb->format->cpp[0]*8.
> >> Less duplicated information is a good thing.
> >> 
> >> Note that I didn't put parens around the cpp*8 in the below cocci
> >> script, on account of not wanting spurious parens all over the place.
> >> Instead I did the unsafe way, and tried to look over the entire diff to
> >> spot if any dangerous expressions were produced. I didn't see any.
> >> 
> >> There are some cases where previously the code did X*bpp/8, so the
> >> division happened after the multiplication. Those are now just X*cpp
> >> so the division effectively happens before the multiplication,
> >> but that is perfectly fine since bpp is always a multiple of 8.
> > 
> > [snip]
> > 
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c        |  2 +-
> >>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c        |  2 +-
> >>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c         |  2 +-
> >>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c         |  2 +-
> >>  drivers/gpu/drm/armada/armada_crtc.c          |  4 ++--
> >>  drivers/gpu/drm/armada/armada_fbdev.c         |  2 +-
> >>  drivers/gpu/drm/ast/ast_fb.c                  |  2 +-
> >>  drivers/gpu/drm/ast/ast_mode.c                |  9 +++++----
> >>  drivers/gpu/drm/cirrus/cirrus_fbdev.c         |  2 +-
> >>  drivers/gpu/drm/cirrus/cirrus_mode.c          |  2 +-
> >>  drivers/gpu/drm/drm_fb_helper.c               |  8 ++++----
> >>  drivers/gpu/drm/drm_framebuffer.c             |  2 +-
> >>  drivers/gpu/drm/drm_modeset_helper.c          |  3 ---
> >>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  4 ++--
> >>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    |  6 +++---
> >>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c     |  4 ++--
> >>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  2 +-
> >>  drivers/gpu/drm/exynos/exynos_mixer.c         |  4 ++--
> >>  drivers/gpu/drm/gma500/framebuffer.c          |  2 +-
> >>  drivers/gpu/drm/gma500/gma_display.c          |  4 ++--
> >>  drivers/gpu/drm/gma500/mdfld_intel_display.c  |  6 +++---
> >>  drivers/gpu/drm/gma500/oaktrail_crtc.c        |  4 ++--
> >>  drivers/gpu/drm/i915/i915_debugfs.c           |  4 ++--
> >>  drivers/gpu/drm/i915/intel_display.c          | 11 ++++-------
> >>  drivers/gpu/drm/i915/intel_fbdev.c            |  6 +++---
> >>  drivers/gpu/drm/mgag200/mgag200_fb.c          |  2 +-
> >>  drivers/gpu/drm/mgag200/mgag200_mode.c        | 16 ++++++++--------
> >>  drivers/gpu/drm/nouveau/dispnv04/crtc.c       |  4 ++--
> >>  drivers/gpu/drm/nouveau/nouveau_display.c     |  2 +-
> >>  drivers/gpu/drm/qxl/qxl_draw.c                |  2 +-
> >>  drivers/gpu/drm/radeon/atombios_crtc.c        | 11 ++++++-----
> >>  drivers/gpu/drm/radeon/r100.c                 |  4 ++--
> >>  drivers/gpu/drm/radeon/radeon_display.c       |  6 +++---
> >>  drivers/gpu/drm/radeon/radeon_legacy_crtc.c   | 14 +++++++-------
> >>  drivers/gpu/drm/tegra/dc.c                    |  2 +-
> >>  drivers/gpu/drm/tegra/drm.c                   |  2 +-
> >>  drivers/gpu/drm/udl/udl_fb.c                  |  2 +-
> >>  drivers/gpu/drm/virtio/virtgpu_fb.c           |  2 +-
> >>  drivers/gpu/drm/vmwgfx/vmwgfx_fb.c            |  6 +++---
> >>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c           |  2 --
> >>  drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c           |  4 ++--
> >>  drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c          |  2 +-
> >>  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c          |  2 +-
> >>  include/drm/drm_framebuffer.h                 |  7 -------
> >>  44 files changed, 89 insertions(+), 102 deletions(-)
> > 
> > [snip]
> > 
> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c
> >> b/drivers/gpu/drm/drm_fb_helper.c index 755e3b6e9710..bf5a06b7c0c1
> >> 100644
> >> --- a/drivers/gpu/drm/drm_fb_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >> @@ -1162,7 +1162,7 @@ static int setcolreg(struct drm_crtc *crtc, u16
> >> red, u16 green,
> >> 		!fb_helper->funcs->gamma_get))
> >> 
> >>  		return -EINVAL;
> >> 
> >> -	WARN_ON(fb->bits_per_pixel != 8);
> >> +	WARN_ON(fb->format->cpp[0] * 8 != 8);
> > 
> > Maybe just WARN_ON(fb->format->cpp[0] != 1); ?
> 
> I think I had a bit of cocci to do that... Yeah, it's there, and rerunning
> it does clean this up.
> 
> spatch is rather finicky and does occasionally miss entire files when
> you run it in the -dir mode. But in this case it clearly do something to
> the file, so not sure why the cleanup part didn't get done.
> 
> >>  	fb_helper->funcs->gamma_set(crtc, red, green, blue, regno);
> > 
> > [snip]
> > 
> >> diff --git a/drivers/gpu/drm/drm_modeset_helper.c
> >> b/drivers/gpu/drm/drm_modeset_helper.c index e5d19e5fc341..3c44409244dc
> >> 100644
> >> --- a/drivers/gpu/drm/drm_modeset_helper.c
> >> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> >> @@ -82,10 +82,7 @@ void drm_helper_mode_fill_fb_struct(struct drm_device
> >> *dev, DRM_DEBUG_KMS("non-RGB pixel format %s\n",
> >>  		              drm_get_format_name(mode_cmd->pixel_format,
> >>  		                                  &format_name));
> >> -
> >> -		fb->bits_per_pixel = 0;
> >>  	} else {
> >> -		fb->bits_per_pixel = info->cpp[0] * 8;
> >>  	}
> > 
> > I think you can remove the whole check.
> 
> Not sure I can tell the tool that. Probably easier to remove it by hand
> afterwards.

Fine with me, you can create a follow-up patch that manually fixes the left-
overs.

> >>  	fb->dev = dev;
> > 
> > [snip]
> > 
> >> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c
> >> b/drivers/gpu/drm/radeon/atombios_crtc.c index
> >> 05f4ebe31ce2..8eeffc5324b2
> >> 100644
> >> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> >> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> >> @@ -1277,7 +1277,7 @@ static int dce4_crtc_do_set_base(struct drm_crtc
> >> *crtc,
> >>  				/* Calculate the macrotile mode index. */
> >>  				tile_split_bytes = 64 << tile_split;
> >> -				tileb = 8 * 8 * target_fb->bits_per_pixel / 8;
> >> +				tileb = (8 * 8) * target_fb->format->cpp[0];
> > 
> > The parentheses are not needed.
> 
> Added by cocci. I guess I can tell it to clean that up, but have to
> double check that it won't accidentally clean up something else too
> which needs extra parens.

You can also fix it manually in a subsequent patch.

> > With these fixed,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> >>  				tileb = min(tile_split_bytes, tileb);
> >>  				
> >>  				for (index = 0; tileb > 64; index++)
> > 
> > [snip]

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel