Re: [PATCH v3.1 09/15] drm: tilcdc: Replace drm_fb_get_bpp_depth() with drm_format_plane_cpp()

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3106 bytes --]

On 10/06/16 15:08, Laurent Pinchart wrote:
> The driver needs the number of bytes per pixel, not the bpp and depth
> info meant for fbdev compatibility. Use the right API.
> 
> In the tilcdc_crtc_mode_set() function compute the hardware register
> value directly from the pixel format instead of computing the number of
> bits per pixels first.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 79027b1c64d3..1f3f3a1c7b5f 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -65,15 +65,13 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
>  	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_gem_cma_object *gem;
> -	unsigned int depth, bpp;
>  	dma_addr_t start, end;
>  
> -	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
>  	gem = drm_fb_cma_get_gem_obj(fb, 0);
>  
>  	start = gem->paddr + fb->offsets[0] +
>  		crtc->y * fb->pitches[0] +
> -		crtc->x * bpp / 8;
> +		crtc->x * drm_format_plane_cpp(fb->pixel_format, 0);
>  
>  	end = start + (crtc->mode.vdisplay * fb->pitches[0]);
>  
> @@ -132,11 +130,12 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
>  static int tilcdc_verify_fb(struct drm_crtc *crtc, struct drm_framebuffer *fb)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	unsigned int depth, bpp;
> +	unsigned int pitch;
>  
> -	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
> +	pitch = crtc->mode.hdisplay
> +	      * drm_format_plane_cpp(fb->pixel_format, 0);

I see you're using this form of having + or * at the start of the
continuation line in some patches... I don't like it =). And it's not
what's being used elsewhere in the driver, afaics.

>  
> -	if (fb->pitches[0] != crtc->mode.hdisplay * bpp / 8) {
> +	if (fb->pitches[0] != pitch) {
>  		dev_err(dev->dev,
>  			"Invalid pitch: fb and crtc widths must be the same");
>  		return -EINVAL;
> @@ -401,16 +400,14 @@ static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
>  	if (info->tft_alt_mode)
>  		reg |= LCDC_TFT_ALT_ENABLE;
>  	if (priv->rev == 2) {
> -		unsigned int depth, bpp;
> -
> -		drm_fb_get_bpp_depth(crtc->primary->fb->pixel_format, &depth, &bpp);
> -		switch (bpp) {
> -		case 16:
> +		switch (crtc->primary->fb->pixel_format) {
> +		case DRM_FORMAT_RGB565:
>  			break;
> -		case 32:
> +		case DRM_FORMAT_XRGB8888:
> +		case DRM_FORMAT_ARGB8888:
>  			reg |= LCDC_V2_TFT_24BPP_UNPACK;
>  			/* fallthrough */
> -		case 24:
> +		case DRM_FORMAT_RGB888:
>  			reg |= LCDC_V2_TFT_24BPP_MODE;
>  			break;
>  		default:
> 

I think it's better to keep ARGB allowed for now. If we want to disallow
it, it's better to be done in a separate patch.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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