On Tue, 27 Feb 2024 16:02:10 +0100
Louis Chauvet <louis.chauvet@xxxxxxxxxxx> 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,
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.
>
> 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]);
> > > }
> > >
>
> [...]
>
> > > +/**
> > > + * Retrieve the correct read_pixel function for a specific format.
> > > + * The returned pointer is NULL for unsupported pixel formats. The caller must ensure that the
> > > + * pointer is valid before using it in a vkms_plane_state.
> > > + *
> > > + * @format: 4cc of the format
> >
> > Since there are many different 4cc style pixel format definition tables
> > in existence with conflicting definitions, it would not hurt to be more
> > specific that this is about DRM_FORMAT_* or drm_fourcc.h.
>
> Is this better?
>
> @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
Much better!
Thanks,
pq
> > > + */
> > > void *get_pixel_conversion_function(u32 format)
> > > {
> > > switch (format) {
> > > @@ -247,6 +280,13 @@ void *get_pixel_conversion_function(u32 format)
> > > }
> > > }
> > >
> > > +/**
> > > + * Retrieve the correct write_pixel function for a specific format.
> > > + * The returned pointer is NULL for unsupported pixel formats. The caller must ensure that the
> > > + * pointer is valid before using it in a vkms_writeback_job.
> > > + *
> > > + * @format: 4cc of the format
> >
> > This too.
>
> Ack, I will use the same as above
>
> > > + */
> > > void *get_pixel_write_function(u32 format)
> > > {
> > > switch (format) {
> > >
> >
> > I couldn't check if the docs are correct since the patch context is not
> > wide enough, but they all sound plausible to me.
>
> I checked again, I don't see other errors than your first comment.
>
> >
> > Thanks,
> > pq
>
> Kind regards,
> Louis Chauvet
>
> --
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Attachment:
pgpC_5YoAULSs.pgp
Description: OpenPGP digital signature
