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

[Louis Chauvet]

[...]

> > > > > > +++ 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 did not notice this, and I agree, thanks for this feedback. For the v5 I 
will change it and update the function signature to:

static void packed_pixels_offset(const struct vkms_frame_info *frame_info, int x, int y,
				 size_t plane_index, size_t *offset, size_t *rem_x, size_t *rem_y)

where rem_x and rem_y are those reminder.
 
> 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.

Yes, none of the supported format have block_h != block_w != 1, so there 
is no need to drm_format_info_block*() helpers.

I wrote the code for DRM_FORMAT_R*. They are packed, with block_w != 1. I 
will add this patch in the next revision. I also wrote the IGT test for 
DRM_FORMAT_R1 [1]. Everything will be in the v5 (I will send it when you have the 
time to review the v4).

For information, I also have a series ready for adding more RGB variants
(I introduced a macro to make it easier and avoid copy/pasting the same
loop). I don't send them yet, because I realy want this series merged 
first. I also have the work for the writeback "line-by-line" algorithm 
ready (I just need to rebase it, but it will be fast).

[1]: https://lore.kernel.org/igt-dev/20240306-b4-kms_tests-v1-0-8fe451efd2ac@xxxxxxxxxxx

Kind regards,
Louis Chauvet

[...]

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com