[...]
> > > > > > +++ 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
