On 09/08/2020 01:14, Laurent Pinchart wrote:
> Hi Tomi,
>
> On Thu, Aug 06, 2020 at 12:21:39PM +0300, Tomi Valkeinen wrote:
>> On 06/08/2020 05:18, Laurent Pinchart wrote:
>>> The BO pitches are unconditionally set to the frame buffer pitch, for
>>> all planes. This is correct for semiplanar YUV formats, as they
>>> subsample chroma horizontally by two but combined U and V in a single
>>> plane, cancelling each other. For fully planar YUV formats, however, the
>>> horizontal subsampling need to be taken into account to compute the
>>> pitch. Fix it.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> ---
>>> kms++/src/dumbframebuffer.cpp | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/kms++/src/dumbframebuffer.cpp b/kms++/src/dumbframebuffer.cpp
>>> index 18f3f152943d..4c3c03164a90 100644
>>> --- a/kms++/src/dumbframebuffer.cpp
>>> +++ b/kms++/src/dumbframebuffer.cpp
>>> @@ -42,6 +42,14 @@ DumbFramebuffer::DumbFramebuffer(Card& card, uint32_t width, uint32_t height, Pi
>>> struct drm_mode_create_dumb creq = drm_mode_create_dumb();
>>> creq.width = width;
>>> creq.height = height / pi.ysub;
>>> + /*
>>> + * For fully planar YUV buffers, the chroma planes don't combine
>>> + * U and V components, their width must thus be divided by the
>>> + * horizontal subsampling factor.
>>> + */
>>> + if (format_info.type == PixelColorType::YUV &&
>>> + format_info.num_planes == 3)
>>> + creq.width /= pi.xsub;
>>
>> This feels a bit of a hack... I think we should somehow have the
>> relevant information in the PixelFormatInfo. Having the same plane
>> info, { 8, 2, 2 }, for both NV12 UV plane and YUV420 U and V planes
>> doesn't sound correct.
>>
>> Should NV12's UV plane be { 16, 2, 2 }? Subsampled formats are
>> confusing... =)
>
> I'll give it a try. I however wonder if all drivers will expect 16bpp.
> The ones based on drm_gem_cma_dumb_create() will be fine, but other
> drivers may not expect this.
Oh, right, that number is passed to DRM_IOCTL_MODE_CREATE_DUMB. I was only thinking about how kms++
handles these internally.
To be honest, I don't even quite know how subsampled formats are supposed to be handled in DRM.
Above we pass width as it is, but divide height by ysub. And then we tune the bpp adjust to the fact
that we didn't divide the width.
For e.g. YUYV, the bpp tells the container size. But for NV12's second plane, bpp is not the
container size.
Well, I think at least omapdrm doesn't care, it just allocates the number of bytes according to
w*h*bpp. But maybe it would be good to have a clear rule how to represent these in kms++, and then
if DRM wants the values in some other way, adjust the values according to the format.
Or maybe we already have all the numbers according to a clear rule, I'm just not sure what the rule
is =).
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
