| Message ID | 5715CCFB.8090307@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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);
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);