Re: [PATCH v2 3/9] drm/vkms: write/update the documentation for pixel conversion and pixel write functions

[Pekka Paalanen <pekka.paalanen@collabora.com>]

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

On Mon, 4 Mar 2024 16:28:30 +0100
Louis Chauvet <louis.chauvet@bootlin.com> wrote:

> Le 29/02/24 - 10:48, Pekka Paalanen a écrit :
> > On Tue, 27 Feb 2024 16:02:10 +0100
> > Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> >   
> > > [...]
> > >   
> > > > > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > > > > index 172830a3936a..cb7a49b7c8e7 100644
> > > > > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > > > > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > > > > @@ -9,6 +9,17 @@
> > > > >  
> > > > >  #include "vkms_formats.h"
> > > > >  
> > > > > +/**
> > > > > + * packed_pixels_offset() - Get the offset of the block containing the pixel at coordinates x/y
> > > > > + * in the first plane
> > > > > + *
> > > > > + * @frame_info: Buffer metadata
> > > > > + * @x: The x coordinate of the wanted pixel in the buffer
> > > > > + * @y: The y coordinate of the wanted pixel in the buffer
> > > > > + *
> > > > > + * The caller must be aware that this offset is not always a pointer to a pixel. If individual
> > > > > + * pixel values are needed, they have to be extracted from the resulting block.    
> > > > 
> > > > Just wondering how the caller will be able to extract the right pixel
> > > > from the block without re-using the knowledge already used in this
> > > > function. I'd also expect the function to round down x,y to be
> > > > divisible by block dimensions, but that's not visible in this email.
> > > > Then the caller needs the remainder from the round-down, too?    
> > > 
> > > You are right, the current implementation is only working when block_h == 
> > > block_w == 1. I think I wrote the documentation for PATCHv2 5/9, but when 
> > > backporting this comment for PATCHv2 3/9 I forgot to update it.
> > > The new comment will be:
> > > 
> > >  * pixels_offset() - Get the offset of a given pixel data at coordinate 
> > >  * x/y in the first plane
> > >    [...]
> > >  * The caller must ensure that the framebuffer associated with this 
> > >  * request uses a pixel format where block_h == block_w == 1.
> > >  * If this requirement is not fulfilled, the resulting offset can be 
> > >  * completly wrong.  
> > 
> > Hi Louis,  
> 
> Hi Pekka,
> 
> > if there is no plan for how non-1x1 blocks would work yet, then I think
> > the above wording is fine. In my mind, the below wording would
> > encourage callers to seek out and try arbitrary tricks to make things
> > work for non-1x1 without rewriting the function to actually work.
> >
> > I believe something would need to change in the function signature to
> > make it properly usable for non-1x1 blocks, but I too cannot suggest
> > anything off-hand.  
> 
> I already made the change to support non-1x1 blocks in Patchv2 5/9 
> (I will extract this modification in "drm/vkms: Update pixels accessor to 
> support packed and multi-plane formats"), this function is now able 
> to extract the pointer to the start of a block. But as stated in the 
> comment, the caller must manually extract the correct pixel values (if the 
> format is 2x2, the pointer will point to the first byte of this block, the 
> caller must do some computation to access the bottom-right value).

Patchv2 5/9 is not enough.

"Manually extract the correct pixels" is the thing I have a problem
with here. The caller should not need to re-do any semantic
calculations this function already did. Most likely this function
should return the remainders from the x,y coordinate division, so that
the caller can extract the right pixels from the block, or something
else equivalent.

That same semantic division should not be done in two different places.
It is too easy for someone later to come and change one site while
missing the other.

I have a hard time finding in "[PATCH v2 6/9] drm/vkms: Add YUV
support" how you actually handle blocks bigger than 1x1. I see
get_subsampling() which returns format->{hsub,vsub}, and I see
get_subsampling_offset() which combined with remainder-division gates U
and V plane pixel pointer increments.

However, I do not see you ever using
drm_format_info_block_{width,height}() anywhere else. That makes me
think you have no code to actually handle non-1x1 block formats, which
means that you cannot get the function signature of
packed_pixels_offset() right in this series either. It would be better
to not even pretend the function works for non-1x1 blocks until you
have code handling at least one such format.

All of the YUV formats that patch 6 adds support for use 1x1 blocks all
all their planes.


Thanks,
pq

> > > 
> > > And yes, even after PATCHv2 5/9 it is not clear what is the offset. Is 
> > > this better to replace the last sentence? (I will do the same update for 
> > > the last sentence of packed_pixels_addr)
> > > 
> > >    [...]
> > >  * The returned offset correspond to the offset of the block containing the pixel at coordinates 
> > >  * x/y.
> > >  * The caller must use this offset with care, as for formats with block_h != 1 or block_w != 1 
> > >  * the requested pixel value may have to be extracted from the block, even if they are 
> > >  * individually adressable.
> > >    
> > > > > + */
> > > > >  static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
> > > > >  {
> > > > >  	struct drm_framebuffer *fb = frame_info->fb;
> > > > > @@ -17,12 +28,13 @@ static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int
> > > > >  			      + (x * fb->format->cpp[0]);
> > > > >  }
> > > > >      

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