Re: [PATCH v5 5/9] drm: vkms: Add fb information to `vkms_writeback_job`

[Igor Torrente <igormtorrente@gmail.com>]

On 4/26/22 04:09, Pekka Paalanen wrote:
> On Mon, 25 Apr 2022 21:56:12 -0300
> Igor Torrente <igormtorrente@gmail.com> wrote:
> 
>> Hi Pekka,
>>
>> On 4/25/22 04:56, Pekka Paalanen wrote:
>>> On Sat, 23 Apr 2022 12:12:51 -0300
>>> Igor Torrente <igormtorrente@gmail.com> wrote:
>>>    
>>>> Hi Pekka,
>>>>
>>>> On 4/20/22 08:23, Pekka Paalanen wrote:
>>>>> On Mon,  4 Apr 2022 17:45:11 -0300
>>>>> Igor Torrente <igormtorrente@gmail.com> wrote:
>>>>>       
>>>>>> This commit is the groundwork to introduce new formats to the planes and
>>>>>> writeback buffer. As part of it, a new buffer metadata field is added to
>>>>>> `vkms_writeback_job`, this metadata is represented by the `vkms_composer`
>>>>>> struct.
>>>>>
>>>>> Hi,
>>>>>
>>>>> should this be talking about vkms_frame_info struct instead?
>>>>
>>>> Yes it should. I will fix this. Thanks.
>>>>   
>>>>>       
>>>>>>
>>>>>> Also adds two new function pointers (`{wb,plane}_format_transform_func`)
>>>>>> are defined to handle format conversion to/from internal format.
>>>>>>
>>>>>> These things will allow us, in the future, to have different compositing
>>>>>> and wb format types.
>>>>>>
>>>>>> V2: Change the code to get the drm_framebuffer reference and not copy its
>>>>>>        contents(Thomas Zimmermann).
>>>>>> V3: Drop the refcount in the wb code(Thomas Zimmermann).
>>>>>> V5: Add {wb,plane}_format_transform_func to vkms_writeback_job
>>>>>>        and vkms_plane_state (Pekka Paalanen)
>>>>>>
>>>>>> Signed-off-by: Igor Torrente <igormtorrente@gmail.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/vkms/vkms_composer.c  |  4 ++--
>>>>>>     drivers/gpu/drm/vkms/vkms_drv.h       | 31 +++++++++++++++++++++------
>>>>>>     drivers/gpu/drm/vkms/vkms_plane.c     | 10 ++++-----
>>>>>>     drivers/gpu/drm/vkms/vkms_writeback.c | 20 ++++++++++++++---
>>>>>>     4 files changed, 49 insertions(+), 16 deletions(-)
> 
> ...
> 
>>>>>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
>>>>>> index 2e6342164bef..2704cfb6904b 100644
>>>>>> --- a/drivers/gpu/drm/vkms/vkms_drv.h
>>>>>> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> 
> ...
> 
>>>>>> +
>>>>>> +struct vkms_writeback_job {
>>>>>> +	struct dma_buf_map data[DRM_FORMAT_MAX_PLANES];
>>>>>> +	struct vkms_frame_info frame_info;
>>>>>
>>>>> Which frame_info is this? Should the field be called wb_frame_info?
>>>>
>>>> Considering it's already in the writeback_job struct do you think this
>>>> necessary?
>>>
>>> This struct has 'data' too, and that is not the wb buffer, right?
>>
>> AFAIU, no. Each plane has its own `iosys_map map[]`.
>>
>>>
>>> Hmm, if it's not the wb buffer, then using DRM_FORMAT_MAX_PLANES is
>>> odd...
>>
>> Yeah. I honestly don't have any clue why we have an array of `iosys_map`
>> for each plane, why we only use the map[0] and why we only call
>> `iosys_map_is_null` only to the `primary_composer`.
>>
>> Maybe @tzimmermann or @rodrigosiqueiramelo can shed some light on these
>> questions.
> 
> Yeah, those questions would be really good to figure out.
> 
> FWIW, I can tell you this: "plane" has two different meanings in the
> context of KMS:
> 
> https://gitlab.freedesktop.org/pq/color-and-hdr/-/blob/main/doc/glossary.md#plane
> 
> DRM_FORMAT_MAX_PLANES refers to the number of planes (or the number of
> memory buffers) for a single image (single framebuffer). Most often
> with RGB there is just one plane in one memory buffer. RGB buffer could
> be accompanied with e.g. a compression data buffer, so two planes,
> one buffer each. Different YUV formats have different numbers of planes
> from N=1 to 3, and those plane may be stored in 1 to N memory buffers
> (with dmabuf handles pointing to them).
> 
> The number of planes and the number of memory buffers are often
> conflated in APIs by just passing the same memory buffer multiple times
> when multiple planes are stored in the same buffer (with different
> offset).
> 
> The number of planes is determined by the pixel format and the format
> modifier. The number of memory buffers is more... vaguely defined and
> may vary sometimes.

Right. So probably this answers the first two questions. And also
probably my initial implementation of YUV420 and NV12 is wrong.

> 
>>
>>>    
>>>> In other words, what kind of misudertanding do you think can happen if
>>>> this variable stay without the `wb_` prefix?
>>>>
>>>> I spent a few minutes trying to find a case, but nothing came to my
>>>> mind.
>>>
>>> My question above is the confusion.
>>>
>>> If all these members are about the wb destination buffer only, then
>>> where do the inputs come from and how are they reference-counted to
>>> make sure they are available when needed?
>>
>> Ok. Got it.
> 
> 
> Thanks,
> pq