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.

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