Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

2015-03-02 Thread Tomi Valkeinen 
On 02/03/15 12:22, Daniel Stone wrote:

 I don't know why Rob named it like that. The bpp of the stride? Shrug.
 
 It's just the bpp of the pixel format; it's not at all associated with
 the stride?

The comment says this times width is stride, so I thought the naming comes
from that train of thought. But I agree, it's just bpp.

Here's updated patch:

From af0db526ae274d47488ec49577365909eb8f629d Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen tomi.valkei...@ti.com
Date: Thu, 25 Sep 2014 19:24:28 +
Subject: [PATCH] drm/omap: handle incompatible buffer stride and pixel size

omapdrm doesn't check if the pitch of the framebuffer and the color
format's bits-per-pixel are compatible. omapdss requires that the stride
of a buffer is an integer number of pixels

For example, when using modetest with a display that has x resolution of
1280, and using packed 24 RGB mode (3 bytes per pixel), modetest
allocates a buffer with a byte stride of 4 * 1280 = 5120. But 5120 / 3 =
1706.666... pixels, which causes wrong colors and a tilt on the screen.

Add a check into omapdrm to return an error if the user tries to use
such a combination.

Note: this is not a HW requirement at least for non-rotation use cases,
but a SW driver requirement. In the future we should study if also
rotation use cases are fine with any stride size, and if so, change the
driver to allow these strides.

Signed-off-by: Tomi Valkeinen tomi.valkei...@ti.com
---
 drivers/gpu/drm/omapdrm/omap_fb.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c 
b/drivers/gpu/drm/omapdrm/omap_fb.c
index 2975096abdf5..e123b4dee670 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct 
drm_device *dev,
goto fail;
}
 
+   if (pitch % format-planes[i].stride_bpp != 0) {
+   dev_err(dev-dev,
+   buffer pitch (%d bytes) is not a multiple of 
pixel size (%d bytes)\n,
+   pitch, format-planes[i].stride_bpp);
+   ret = -EINVAL;
+   goto fail;
+   }
+
size = pitch * mode_cmd-height / format-planes[i].sub_y;
 
if (size  (omap_gem_mmap_size(bos[i]) - mode_cmd-offsets[i])) 
{
-- 
2.3.1





signature.asc
Description: OpenPGP digital signature

Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

2015-03-02 Thread Tomi Valkeinen 
On 27/02/15 16:40, Daniel Stone wrote:
 Hi,
 
 On 27 February 2015 at 13:01, Daniel Vetter dan...@ffwll.ch wrote:
 On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote:
 omapdrm doesn't check if the width of the framebuffer and the color
 
 s/width/pitch/
 
 format's bits-per-pixel match.
 
 s/match/are compatible/
 
 For example, using a display with a width of 1280, and a buffer
 allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with
 
 Might be clearer to say 'i.e. byte stride of ...', and also s/with using/for/.

Above you said pitch, here you say stride. They are the same thing, right?

 a 24 bits per pixel color format, leads to the following mismatch:
 5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the
 
 s/bytes/pixels/
 
 screen.

 Add a check into omapdrm to return an error if the user tries to use
 such a combination.

 Signed-off-by: Tomi Valkeinen tomi.valkei...@ti.com
 ---
  drivers/gpu/drm/omapdrm/omap_fb.c | 8 
  1 file changed, 8 insertions(+)

 diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c 
 b/drivers/gpu/drm/omapdrm/omap_fb.c
 index 2975096abdf5..bf98580223d0 100644
 --- a/drivers/gpu/drm/omapdrm/omap_fb.c
 +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
 @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct 
 drm_device *dev,
   goto fail;
   }

 + if (mode_cmd-width % format-planes[i].stride_bpp != 0) {

 width is in pixels. No idea what you're trying to check here, but this
 probably isn't it.

Yep, I don't know what I was smoking when writing the patch...

 stride_bpp is very misnamed: it is the bits per pixel for that plane,
 and not stride at all. I think the check should in fact be be (pitch %

I don't know why Rob named it like that. The bpp of the stride? Shrug.

 format-planes[i].stride_bpp), which would achieve the desired result,
 i.e. that the stride can be expressed as an integer number of pixels.

Yes, that looks correct.

 Also drm checks that things fit into the specified pitch (which is in
 bytes), see the pichtes[i]  width * cpp check in framebuffer_check.
 
 This isn't that check. At some stages, OMAP IIRC requires pitch to be
 specified in pixels rather than bytes, so this makes sure that's
 possible to express.

Right, that's what this patch was trying to achieve.

However... After thinking about this and going through some of the DISPC
code, I think that's not a strict requirement. We do calculate all the
configs using pixels as units, so at the moment the stride has to be an
integer number of pixels. But the hardware actually takes the row-inc
and pix-inc as bytes.

That said, the HW supports features like rotation and whatnot, and it
was not clear with a quick study if there are corner cases where the
hardware also requires the stride to be an integer number of pixels.
Also, the HW documentation only talks about pixels in this context, even
if the final value written to the registers is in bytes. I don't know if
that's just to make the documentation simpler, or if there's some
reasoning to only use pixel units.

So I think for the time being I'll just fix this patch, and look at the
possibility of allowing any stride size in the future.

Thanks for the review!

 Tomi




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

2015-03-02 Thread Daniel Stone 
Hi,

On 2 March 2015 at 09:50, Tomi Valkeinen tomi.valkei...@ti.com wrote:
 On 27/02/15 16:40, Daniel Stone wrote:
 On 27 February 2015 at 13:01, Daniel Vetter dan...@ffwll.ch wrote:
 On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote:
 omapdrm doesn't check if the width of the framebuffer and the color

 s/width/pitch/

 format's bits-per-pixel match.

 s/match/are compatible/

 For example, using a display with a width of 1280, and a buffer
 allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with

 Might be clearer to say 'i.e. byte stride of ...', and also s/with 
 using/for/.

 Above you said pitch, here you say stride. They are the same thing, right?

Yeah, they're interchangeable. In theory, I think it's supposed to be
that pitch is in pixels and stride in bpp, but they're so often
interchanged that they've lost that distinction. Still, using one
consistently is always good - and since KMS uses pitch exclusively,
that might be a good one to settle on.

 stride_bpp is very misnamed: it is the bits per pixel for that plane,
 and not stride at all. I think the check should in fact be be (pitch %

 I don't know why Rob named it like that. The bpp of the stride? Shrug.

It's just the bpp of the pixel format; it's not at all associated with
the stride?

 This isn't that check. At some stages, OMAP IIRC requires pitch to be
 specified in pixels rather than bytes, so this makes sure that's
 possible to express.

 Right, that's what this patch was trying to achieve.

 However... After thinking about this and going through some of the DISPC
 code, I think that's not a strict requirement. We do calculate all the
 configs using pixels as units, so at the moment the stride has to be an
 integer number of pixels. But the hardware actually takes the row-inc
 and pix-inc as bytes.

 That said, the HW supports features like rotation and whatnot, and it
 was not clear with a quick study if there are corner cases where the
 hardware also requires the stride to be an integer number of pixels.
 Also, the HW documentation only talks about pixels in this context, even
 if the final value written to the registers is in bytes. I don't know if
 that's just to make the documentation simpler, or if there's some
 reasoning to only use pixel units.

Indeed that was my impression from a quick look, but my OMAP is
extremely rusty these days, so wasn't quite sure ... :) Specifying
pixel units isn't totally unheard of, but bytes is definitely more
common.

Cheers,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

2015-02-27 Thread Daniel Vetter 
On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote:
 omapdrm doesn't check if the width of the framebuffer and the color
 format's bits-per-pixel match.
 
 For example, using a display with a width of 1280, and a buffer
 allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with
 a 24 bits per pixel color format, leads to the following mismatch:
 5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the
 screen.
 
 Add a check into omapdrm to return an error if the user tries to use
 such a combination.
 
 Signed-off-by: Tomi Valkeinen tomi.valkei...@ti.com
 ---
  drivers/gpu/drm/omapdrm/omap_fb.c | 8 
  1 file changed, 8 insertions(+)
 
 diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c 
 b/drivers/gpu/drm/omapdrm/omap_fb.c
 index 2975096abdf5..bf98580223d0 100644
 --- a/drivers/gpu/drm/omapdrm/omap_fb.c
 +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
 @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct 
 drm_device *dev,
   goto fail;
   }
  
 + if (mode_cmd-width % format-planes[i].stride_bpp != 0) {

width is in pixels. No idea what you're trying to check here, but this
probably isn't it.

Also drm checks that things fit into the specified pitch (which is in
bytes), see the pichtes[i]  width * cpp check in framebuffer_check.

Cheers, Daniel
 + dev_err(dev-dev,
 + buffer width (%d) is not a multiple of pixel 
 width (%d)\n,
 + mode_cmd-width, format-planes[i].stride_bpp);
 + ret = -EINVAL;
 + goto fail;
 + }
 +
   size = pitch * mode_cmd-height / format-planes[i].sub_y;
  
   if (size  (omap_gem_mmap_size(bos[i]) - mode_cmd-offsets[i])) 
 {
 -- 
 2.3.0
 
 ___
 dri-devel mailing list
 dri-de...@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

2015-02-27 Thread Daniel Stone 
Hi,

On 27 February 2015 at 13:01, Daniel Vetter dan...@ffwll.ch wrote:
 On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote:
 omapdrm doesn't check if the width of the framebuffer and the color

s/width/pitch/

 format's bits-per-pixel match.

s/match/are compatible/

 For example, using a display with a width of 1280, and a buffer
 allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with

Might be clearer to say 'i.e. byte stride of ...', and also s/with using/for/.

 a 24 bits per pixel color format, leads to the following mismatch:
 5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the

s/bytes/pixels/

 screen.

 Add a check into omapdrm to return an error if the user tries to use
 such a combination.

 Signed-off-by: Tomi Valkeinen tomi.valkei...@ti.com
 ---
  drivers/gpu/drm/omapdrm/omap_fb.c | 8 
  1 file changed, 8 insertions(+)

 diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c 
 b/drivers/gpu/drm/omapdrm/omap_fb.c
 index 2975096abdf5..bf98580223d0 100644
 --- a/drivers/gpu/drm/omapdrm/omap_fb.c
 +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
 @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct 
 drm_device *dev,
   goto fail;
   }

 + if (mode_cmd-width % format-planes[i].stride_bpp != 0) {

 width is in pixels. No idea what you're trying to check here, but this
 probably isn't it.

stride_bpp is very misnamed: it is the bits per pixel for that plane,
and not stride at all. I think the check should in fact be be (pitch %
format-planes[i].stride_bpp), which would achieve the desired result,
i.e. that the stride can be expressed as an integer number of pixels.

 Also drm checks that things fit into the specified pitch (which is in
 bytes), see the pichtes[i]  width * cpp check in framebuffer_check.

This isn't that check. At some stages, OMAP IIRC requires pitch to be
specified in pixels rather than bytes, so this makes sure that's
possible to express.

Cheers,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

2015-02-27 Thread Daniel Vetter 
On Fri, Feb 27, 2015 at 02:40:20PM +, Daniel Stone wrote:
 On 27 February 2015 at 13:01, Daniel Vetter dan...@ffwll.ch wrote:
  On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote:
  omapdrm doesn't check if the width of the framebuffer and the color
  diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c 
  b/drivers/gpu/drm/omapdrm/omap_fb.c
  index 2975096abdf5..bf98580223d0 100644
  --- a/drivers/gpu/drm/omapdrm/omap_fb.c
  +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
  @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct 
  drm_device *dev,
goto fail;
}
 
  + if (mode_cmd-width % format-planes[i].stride_bpp != 0) {
 
  width is in pixels. No idea what you're trying to check here, but this
  probably isn't it.
 
 stride_bpp is very misnamed: it is the bits per pixel for that plane,
 and not stride at all. I think the check should in fact be be (pitch %
 format-planes[i].stride_bpp), which would achieve the desired result,
 i.e. that the stride can be expressed as an integer number of pixels.

I meant that mode_cmd-width is in pixels and so totally not what you want
to check here. It probably should be mode_cmd-pitches[i].
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html