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@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx>
>
> > tileb = min(tile_split_bytes, tileb);
> >
> > for (index = 0; tileb > 64; index++)
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
