On 19/04/16 05:12, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> 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 at ti.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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:

Reply via email to