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

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

[-- Attachment #1: Type: text/plain, Size: 3486 bytes --]

On 27/02/15 16:40, Daniel Stone wrote:
> Hi,
> 
> On 27 February 2015 at 13:01, Daniel Vetter <daniel@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.valkeinen@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



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