| Message ID | 20240223-yuv-v2-3-aa6be2827bb7@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: Reimplement line-per-line pixel conversion for plane reading | expand |
On Fri, 23 Feb 2024 12:37:23 +0100 Louis Chauvet <louis.chauvet@bootlin.com> wrote: > Add some documentation on pixel conversion functions. > Update of outdated comments for pixel_write functions. > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > --- > drivers/gpu/drm/vkms/vkms_composer.c | 4 +++ > drivers/gpu/drm/vkms/vkms_drv.h | 13 ++++++++ > drivers/gpu/drm/vkms/vkms_formats.c | 58 ++++++++++++++++++++++++++++++------ > 3 files changed, 66 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c > index c6d9b4a65809..5b341222d239 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -189,6 +189,10 @@ static void blend(struct vkms_writeback_job *wb, > > size_t crtc_y_limit = crtc_state->base.crtc->mode.vdisplay; > > + /* > + * The planes are composed line-by-line. It is a necessary complexity to avoid poor > + * blending performance. > + */ > for (size_t y = 0; y < crtc_y_limit; y++) { > fill_background(&background_color, output_buffer); > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index b4b357447292..18086423a3a7 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -25,6 +25,17 @@ > > #define VKMS_LUT_SIZE 256 > > +/** > + * struct vkms_frame_info - structure to store the state of a frame > + * > + * @fb: backing drm framebuffer > + * @src: source rectangle of this frame in the source framebuffer > + * @dst: destination rectangle in the crtc buffer > + * @map: see drm_shadow_plane_state@data > + * @rotation: rotation applied to the source. > + * > + * @src and @dst should have the same size modulo the rotation. > + */ > struct vkms_frame_info { > struct drm_framebuffer *fb; > struct drm_rect src, dst; > @@ -52,6 +63,8 @@ struct vkms_writeback_job { > * vkms_plane_state - Driver specific plane state > * @base: base plane state > * @frame_info: data required for composing computation > + * @pixel_read: function to read a pixel in this plane. The creator of a vkms_plane_state must > + * ensure that this pointer is valid > */ > struct vkms_plane_state { > struct drm_shadow_plane_state base; > 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? > + */ > 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]); > } > > -/* > - * packed_pixels_addr - Get the pointer to pixel of a given pair of coordinates > +/** > + * packed_pixels_addr() - Get the pointer to the block containing the pixel at the given > + * coordinates > * > * @frame_info: Buffer metadata > - * @x: The x(width) coordinate of the 2D buffer > - * @y: The y(Heigth) coordinate of the 2D buffer > + * @x: The x(width) coordinate inside the plane > + * @y: The y(height) coordinate inside the plane > * > * Takes the information stored in the frame_info, a pair of coordinates, and > * returns the address of the first color channel. > @@ -53,6 +65,13 @@ static int get_x_position(const struct vkms_frame_info *frame_info, int limit, i > return x; > } > > +/* > + * The following functions take pixel data from the buffer and convert them to the format > + * ARGB16161616 in out_pixel. > + * > + * They are used in the `vkms_compose_row` function to handle multiple formats. > + */ > + > static void ARGB8888_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel) > { > /* > @@ -145,12 +164,11 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state > } > > /* > - * The following functions take an line of argb_u16 pixels from the > - * src_buffer, convert them to a specific format, and store them in the > - * destination. > + * The following functions take one argb_u16 pixel and convert it to a specific format. The > + * result is stored in @dst_pixels. > * > - * They are used in the `compose_active_planes` to convert and store a line > - * from the src_buffer to the writeback buffer. > + * They are used in the `vkms_writeback_row` to convert and store a pixel from the src_buffer to > + * the writeback buffer. > */ > static void argb_u16_to_ARGB8888(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel) > { > @@ -216,6 +234,14 @@ static void argb_u16_to_RGB565(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel) > *pixels = cpu_to_le16(r << 11 | g << 5 | b); > } > > +/** > + * Generic loop for all supported writeback format. It is executed just after the blending to > + * write a line in the writeback buffer. > + * > + * @wb: Job where to insert the final image > + * @src_buffer: Line to write > + * @y: Row to write in the writeback buffer > + */ > void vkms_writeback_row(struct vkms_writeback_job *wb, > const struct line_buffer *src_buffer, int y) > { > @@ -229,6 +255,13 @@ void vkms_writeback_row(struct vkms_writeback_job *wb, > wb->pixel_write(dst_pixels, &in_pixels[x]); > } > > +/** > + * 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. > + */ > 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. > + */ > 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. Thanks, pq
[...] > > 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. 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]) > > + */ > > 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
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, 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
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). > > > > 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 [...] Kind regards, Louis Chauvet
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]); > > > > > } > > > > >
[...] > > > > > > +++ 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@bootlin.com Kind regards, Louis Chauvet [...]
On Wed, 6 Mar 2024 18:29:53 +0100 Louis Chauvet <louis.chauvet@bootlin.com> 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@bootlin.com > > Kind regards, > Louis Chauvet > > [...] >
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index c6d9b4a65809..5b341222d239 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -189,6 +189,10 @@ static void blend(struct vkms_writeback_job *wb, size_t crtc_y_limit = crtc_state->base.crtc->mode.vdisplay; + /* + * The planes are composed line-by-line. It is a necessary complexity to avoid poor + * blending performance. + */ for (size_t y = 0; y < crtc_y_limit; y++) { fill_background(&background_color, output_buffer); diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index b4b357447292..18086423a3a7 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -25,6 +25,17 @@ #define VKMS_LUT_SIZE 256 +/** + * struct vkms_frame_info - structure to store the state of a frame + * + * @fb: backing drm framebuffer + * @src: source rectangle of this frame in the source framebuffer + * @dst: destination rectangle in the crtc buffer + * @map: see drm_shadow_plane_state@data + * @rotation: rotation applied to the source. + * + * @src and @dst should have the same size modulo the rotation. + */ struct vkms_frame_info { struct drm_framebuffer *fb; struct drm_rect src, dst; @@ -52,6 +63,8 @@ struct vkms_writeback_job { * vkms_plane_state - Driver specific plane state * @base: base plane state * @frame_info: data required for composing computation + * @pixel_read: function to read a pixel in this plane. The creator of a vkms_plane_state must + * ensure that this pointer is valid */ struct vkms_plane_state { struct drm_shadow_plane_state base; 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. + */ 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]); } -/* - * packed_pixels_addr - Get the pointer to pixel of a given pair of coordinates +/** + * packed_pixels_addr() - Get the pointer to the block containing the pixel at the given + * coordinates * * @frame_info: Buffer metadata - * @x: The x(width) coordinate of the 2D buffer - * @y: The y(Heigth) coordinate of the 2D buffer + * @x: The x(width) coordinate inside the plane + * @y: The y(height) coordinate inside the plane * * Takes the information stored in the frame_info, a pair of coordinates, and * returns the address of the first color channel. @@ -53,6 +65,13 @@ static int get_x_position(const struct vkms_frame_info *frame_info, int limit, i return x; } +/* + * The following functions take pixel data from the buffer and convert them to the format + * ARGB16161616 in out_pixel. + * + * They are used in the `vkms_compose_row` function to handle multiple formats. + */ + static void ARGB8888_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel) { /* @@ -145,12 +164,11 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state } /* - * The following functions take an line of argb_u16 pixels from the - * src_buffer, convert them to a specific format, and store them in the - * destination. + * The following functions take one argb_u16 pixel and convert it to a specific format. The + * result is stored in @dst_pixels. * - * They are used in the `compose_active_planes` to convert and store a line - * from the src_buffer to the writeback buffer. + * They are used in the `vkms_writeback_row` to convert and store a pixel from the src_buffer to + * the writeback buffer. */ static void argb_u16_to_ARGB8888(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel) { @@ -216,6 +234,14 @@ static void argb_u16_to_RGB565(u8 *dst_pixels, struct pixel_argb_u16 *in_pixel) *pixels = cpu_to_le16(r << 11 | g << 5 | b); } +/** + * Generic loop for all supported writeback format. It is executed just after the blending to + * write a line in the writeback buffer. + * + * @wb: Job where to insert the final image + * @src_buffer: Line to write + * @y: Row to write in the writeback buffer + */ void vkms_writeback_row(struct vkms_writeback_job *wb, const struct line_buffer *src_buffer, int y) { @@ -229,6 +255,13 @@ void vkms_writeback_row(struct vkms_writeback_job *wb, wb->pixel_write(dst_pixels, &in_pixels[x]); } +/** + * 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 + */ 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 + */ void *get_pixel_write_function(u32 format) { switch (format) {
Add some documentation on pixel conversion functions. Update of outdated comments for pixel_write functions. Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> --- drivers/gpu/drm/vkms/vkms_composer.c | 4 +++ drivers/gpu/drm/vkms/vkms_drv.h | 13 ++++++++ drivers/gpu/drm/vkms/vkms_formats.c | 58 ++++++++++++++++++++++++++++++------ 3 files changed, 66 insertions(+), 9 deletions(-)