[PATCH 31/32] drm: Nuke fb->bits_per_pixel
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Nov 17 18:35:46 UTC 2016
On Thu, Nov 17, 2016 at 08:14:16PM +0200, Laurent Pinchart wrote:
> Hi Ville,
>
> Thank you for the patch.
>
> On Thursday 17 Nov 2016 18:14:30 ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at 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 at 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.
>
> >
> > 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.
>
> With these fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > tileb = min(tile_split_bytes, tileb);
> >
> > for (index = 0; tileb > 64; index++)
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
--
Ville Syrjälä
Intel OTC
More information about the dri-devel
mailing list