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

[Pekka Paalanen <ppaalanen@gmail.com>]

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

On Tue, 26 Apr 2022 21:43:12 -0300
Igor Torrente <igormtorrente@gmail.com> wrote:

> 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.

If I'm reading the code right, it's using the maximum number of image
planes (four) as the maximum number of KMS planes (chosen by the
driver). IOW, it confusing the two meanings of "plane" which have
nothing to do with each other.

Assuming that there is one data[] element for each KMS plane created by
VKMS. That makes a further assumption that each KMS plane framebuffer
has only one image plane. I think. Which they do when you are limited
to RGB formats.


Thanks,
pq

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