On Wed, 6 Mar 2024 18:29:53 +0100
Louis Chauvet <louis.chauvet@xxxxxxxxxxx> wrote:
> [...]
>
> > > > > > > +++ 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.
Ok, that's a start.
Why size_t? It's unsigned. You'll probably be mixing signed and
unsigned variables in computations again.
> > 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].
Excellent!
> Everything will be in the v5 (I will send it when you have the
> time to review the v4).
I'm too busy this week, I think. Maybe next.
Why should I review v4 when I already know you will be changing things
again? I'd probably flag the same things I've already said.
Thanks,
pq
> 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
>
> [...]
>
Attachment:
pgpU5Lbnb1B52.pgp
Description: OpenPGP digital signature
