Re: [PATCH 3/4] drm/omap: remove align_pitch()

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

Hi Tomi,

Thank you for the patch.

On Tuesday 19 Apr 2016 09:15:23 Tomi Valkeinen wrote:
> On 19/04/16 05:12, Laurent Pinchart wrote:
> > On Monday 18 Apr 2016 18:42:13 Tomi Valkeinen wrote:
> >> The previous commit removed aligning the pitch to SGX's pitch
> >> requirement from align_pitch(). What's left is effectively a function
> >> that returns width * bytespp.
> >> 
> >> To clean up the driver, we can remove the function and have the
> >> calculation inline in the two places which call align_pitch().
> >> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > I wonder, however, if the calculation is correct. Shouldn't the pitch be
> > calculated as DIV_ROUND_UP(width * bpp, 8) instead of width *
> > DIV_ROUND_UP(bpp, 8) ?
> 
> Well, in practice it doesn't matter. The bpps we use, 32/24/16 are all
> divisible by 8. So I don't really even know why the div round up is there in
> the first place.
> 
> There's the 12 bit RGB, in a 16 bit container, which in theory works but I
> don't think we have ever used it or really supported it. If 'bpp' is 12 in
> this case, width * DIV_ROUND_UP(bpp, 8) gives the correct pitch. But then,
> I guess 'bpp' should be 16 in this case, as the omap_gem.c part is about
> allocating memory, without knowing the format.
> 
> Then there are the 1/2/4/8 bpp CLUT modes, which we have never supported
> (and they are no longer supported by the HW in recent HW). For those,
> DIV_ROUND_UP(width * bpp, 8) would be correct.
> 
> So... I think you are right. Here:
> 
> From c435ebfc5953d4983c5772ec4fb7c5b0d01ecb9f Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Tue, 19 Apr 2016 09:06:32 +0300
> Subject: [PATCH] drm/omap: fix pitch round-up
> 
> At the moment we calculate the buffer's pitch with:
> 
>   pitch = width * DIV_ROUND_UP(bpp, 8)
> 
> For CLUT modes with bpp of 1/2/4/8 this gives wrong result, and the
> correct pitch is:
> 
>   pitch = DIV_ROUND_UP(width * bpp, 8)
> 
> In practice this doesn't change anything, as we don't support CLUT
> modes, but it's better to have the pitch calculation correct.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> b/drivers/gpu/drm/omapdrm/omap_fbdev.c index cb6af7757720..42831b8c46b9
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
> @@ -125,8 +125,8 @@ static int omap_fbdev_create(struct drm_fb_helper
> *helper, mode_cmd.width = sizes->surface_width;
>  	mode_cmd.height = sizes->surface_height;
> 
> -	mode_cmd.pitches[0] = mode_cmd.width *
> -		DIV_ROUND_UP(sizes->surface_bpp, 8);
> +	mode_cmd.pitches[0] =
> +			DIV_ROUND_UP(mode_cmd.width * sizes->surface_bpp, 8);
> 
>  	fbdev->ywrap_enabled = priv->has_dmm && ywrap_enabled;
>  	if (fbdev->ywrap_enabled) {
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c index 7ea1e00d8ca6..23af01eb1773
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -660,7 +660,7 @@ int omap_gem_dumb_create(struct drm_file *file, struct
> drm_device *dev, {
>  	union omap_gem_size gsize;
> 
> -	args->pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> +	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> 
>  	args->size = PAGE_ALIGN(args->pitch * args->height);

-- 
Regards,

Laurent Pinchart

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