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

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

Hi Ville,

Thank you for the patch.

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); ?

>  	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.

> 
>  	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.

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