| Message ID | 20240409-yuv-v6-9-de1c5728fd70@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: Reimplement line-per-line pixel conversion for plane reading | expand |
On Tue, 09 Apr 2024 15:25:27 +0200 Louis Chauvet <louis.chauvet@bootlin.com> wrote: > The pixel_read_direction enum is useful to describe the reading direction > in a plane. It avoids using the rotation property of DRM, which not > practical to know the direction of reading. > This patch also introduce two helpers, one to compute the > pixel_read_direction from the DRM rotation property, and one to compute > the step, in byte, between two successive pixel in a specific direction. > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > --- > drivers/gpu/drm/vkms/vkms_composer.c | 42 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/vkms/vkms_drv.h | 11 ++++++++++ > drivers/gpu/drm/vkms/vkms_formats.c | 30 ++++++++++++++++++++++++++ > 3 files changed, 83 insertions(+) > > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c > index 45b111c74884..7c2e328c9510 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -159,6 +159,48 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff > } > } > > +/** > + * direction_for_rotation() - Get the correct reading direction for a given rotation > + * > + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation. > + * > + * This function will use the @rotation setting of a source plane to compute the reading > + * direction in this plane which correspond to a "left to right writing" in the CRTC. > + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left > + * to be written from left to right on the CRTC. > + */ > +static enum pixel_read_direction direction_for_rotation(unsigned int rotation) > +{ > + struct drm_rect tmp_a, tmp_b; > + int x, y; > + > + /* > + * The direction is computed by rotating the vector AB (top-left to top-right) in a > + * 1x1 square. Points A and B are depicted as zero-size rectangles on the CRTC. The CRTC writing direction is from A to B. The plane reading direction is discovered by inverse-transforming A and B. (If you want, you can add that to the comment.) > + */ > + > + tmp_a = DRM_RECT_INIT(0, 0, 0, 0); > + tmp_b = DRM_RECT_INIT(1, 0, 0, 0); > + drm_rect_rotate_inv(&tmp_a, 1, 1, rotation); > + drm_rect_rotate_inv(&tmp_b, 1, 1, rotation); > + > + x = tmp_b.x1 - tmp_a.x1; > + y = tmp_b.y1 - tmp_a.y1; > + > + if (x == 1) > + return READ_LEFT_TO_RIGHT; > + else if (x == -1) > + return READ_RIGHT_TO_LEFT; > + else if (y == 1) > + return READ_TOP_TO_BOTTOM; > + else if (y == -1) > + return READ_BOTTOM_TO_TOP; I find this code practically obvious. Excellent! If you want to be more strict, each condition could also require the other component to be zero. > + > + > + WARN_ONCE(true, "The inverse of the rotation gives an incorrect direction."); > + return READ_LEFT_TO_RIGHT; > +} > + > /** > * blend - blend the pixels from all planes and compute crc > * @wb: The writeback frame buffer metadata > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index 2e1a1b824a3c..16317b063c20 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -69,6 +69,17 @@ struct vkms_writeback_job { > pixel_write_t pixel_write; > }; > > +/** > + * enum pixel_read_direction - Enum used internaly by VKMS to represent a reading direction in a > + * plane. > + */ > +enum pixel_read_direction { > + READ_BOTTOM_TO_TOP, > + READ_TOP_TO_BOTTOM, > + READ_RIGHT_TO_LEFT, > + READ_LEFT_TO_RIGHT > +}; > + > /** > * typedef pixel_read_t - These functions are used to read a pixel in the source frame, > * convert it to `struct pixel_argb_u16` and write it to @out_pixel. > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c > index 9a1400ad4db6..f76944874fe7 100644 > --- a/drivers/gpu/drm/vkms/vkms_formats.c > +++ b/drivers/gpu/drm/vkms/vkms_formats.c > @@ -75,6 +75,36 @@ static void packed_pixels_addr(const struct vkms_frame_info *frame_info, > *addr = (u8 *)frame_info->map[0].vaddr + offset; > } > > +/** > + * get_block_step_byte() - Common helper to compute the correct step value between each pixel block This should be called get_block_step_bytes(). "Byte" sounds like it returns a single byte. > + * to read in a certain direction. > + * > + * @fb: Framebuffer to iter on > + * @direction: Direction of the reading > + * @plane_index: Plane to get the step from > + * > + * As the returned offset is the number of bytes between two consecutive blocks in a direction, I'd call it "returned count" rather than "returned offset". > + * the caller may have to read multiple pixel before using the next one (for example, to read from ...multiple pixels before using the next block > + * left to right in a DRM_FORMAT_R1 plane, each block contains 8 pixels, so the step must be used > + * only every 8 pixels. Close parenthesis. > + */ > +static int get_block_step_byte(struct drm_framebuffer *fb, enum pixel_read_direction direction, > + int plane_index) > +{ > + switch (direction) { > + case READ_LEFT_TO_RIGHT: > + return fb->format->char_per_block[plane_index]; > + case READ_RIGHT_TO_LEFT: > + return -fb->format->char_per_block[plane_index]; > + case READ_TOP_TO_BOTTOM: > + return (int)fb->pitches[plane_index]; > + case READ_BOTTOM_TO_TOP: > + return -(int)fb->pitches[plane_index]; I'm not sure if this is correct for formats with block_h > 1. If a pitch is the theoretical count of bytes per line, then this should return block_h * pitch. But I'm not exactly sure what is correct here. Aside from this problem, looks good. Thanks, pq > + } > + > + return 0; > +} > + > /** > * packed_pixels_addr_1x1() - Get the pointer to the block containing the pixel at the given > * coordinates >
[...] > > +/** > > + * direction_for_rotation() - Get the correct reading direction for a given rotation > > + * > > + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation. > > + * > > + * This function will use the @rotation setting of a source plane to compute the reading > > + * direction in this plane which correspond to a "left to right writing" in the CRTC. > > + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left > > + * to be written from left to right on the CRTC. > > + */ > > +static enum pixel_read_direction direction_for_rotation(unsigned int rotation) > > +{ > > + struct drm_rect tmp_a, tmp_b; > > + int x, y; > > + > > + /* > > + * The direction is computed by rotating the vector AB (top-left to top-right) in a > > + * 1x1 square. > > Points A and B are depicted as zero-size rectangles on the CRTC. > The CRTC writing direction is from A to B. The plane reading direction > is discovered by inverse-transforming A and B. > > (If you want, you can add that to the comment.) It is better, thanks! > > + */ > > + > > + tmp_a = DRM_RECT_INIT(0, 0, 0, 0); > > + tmp_b = DRM_RECT_INIT(1, 0, 0, 0); > > + drm_rect_rotate_inv(&tmp_a, 1, 1, rotation); > > + drm_rect_rotate_inv(&tmp_b, 1, 1, rotation); > > + > > + x = tmp_b.x1 - tmp_a.x1; > > + y = tmp_b.y1 - tmp_a.y1; > > + > > + if (x == 1) > > + return READ_LEFT_TO_RIGHT; > > + else if (x == -1) > > + return READ_RIGHT_TO_LEFT; > > + else if (y == 1) > > + return READ_TOP_TO_BOTTOM; > > + else if (y == -1) > > + return READ_BOTTOM_TO_TOP; > > I find this code practically obvious. Excellent! > > If you want to be more strict, each condition could also require the > other component to be zero. I will add it. [...] > > + */ > > +static int get_block_step_byte(struct drm_framebuffer *fb, enum pixel_read_direction direction, > > + int plane_index) > > +{ > > + switch (direction) { > > + case READ_LEFT_TO_RIGHT: > > + return fb->format->char_per_block[plane_index]; > > + case READ_RIGHT_TO_LEFT: > > + return -fb->format->char_per_block[plane_index]; > > + case READ_TOP_TO_BOTTOM: > > + return (int)fb->pitches[plane_index]; > > + case READ_BOTTOM_TO_TOP: > > + return -(int)fb->pitches[plane_index]; > > I'm not sure if this is correct for formats with block_h > 1. > > If a pitch is the theoretical count of bytes per line, then this should > return block_h * pitch. But I'm not exactly sure what is correct here. I think it is related to my answer to patch 07/17. If pitch is what you describe, yes, the step should be block_h * DIV_ROUND_UP(fb_width / block_w) (or something similar). If I take X0L2 with a buffer of 9x5 pixels, the step between two blocks verticaly must be 40 bytes. Your formula and interpretation of pitch will give only 36 bytes (a row only need 18 bytes). So a new "pitch" is needed. > Aside from this problem, looks good. > > > Thanks, > pq > > > + } > > + > > + return 0; > > +} > > + > > /** > > * packed_pixels_addr_1x1() - Get the pointer to the block containing the pixel at the given > > * coordinates > > >
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c index 45b111c74884..7c2e328c9510 100644 --- a/drivers/gpu/drm/vkms/vkms_composer.c +++ b/drivers/gpu/drm/vkms/vkms_composer.c @@ -159,6 +159,48 @@ static void apply_lut(const struct vkms_crtc_state *crtc_state, struct line_buff } } +/** + * direction_for_rotation() - Get the correct reading direction for a given rotation + * + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation. + * + * This function will use the @rotation setting of a source plane to compute the reading + * direction in this plane which correspond to a "left to right writing" in the CRTC. + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left + * to be written from left to right on the CRTC. + */ +static enum pixel_read_direction direction_for_rotation(unsigned int rotation) +{ + struct drm_rect tmp_a, tmp_b; + int x, y; + + /* + * The direction is computed by rotating the vector AB (top-left to top-right) in a + * 1x1 square. + */ + + tmp_a = DRM_RECT_INIT(0, 0, 0, 0); + tmp_b = DRM_RECT_INIT(1, 0, 0, 0); + drm_rect_rotate_inv(&tmp_a, 1, 1, rotation); + drm_rect_rotate_inv(&tmp_b, 1, 1, rotation); + + x = tmp_b.x1 - tmp_a.x1; + y = tmp_b.y1 - tmp_a.y1; + + if (x == 1) + return READ_LEFT_TO_RIGHT; + else if (x == -1) + return READ_RIGHT_TO_LEFT; + else if (y == 1) + return READ_TOP_TO_BOTTOM; + else if (y == -1) + return READ_BOTTOM_TO_TOP; + + + WARN_ONCE(true, "The inverse of the rotation gives an incorrect direction."); + return READ_LEFT_TO_RIGHT; +} + /** * blend - blend the pixels from all planes and compute crc * @wb: The writeback frame buffer metadata diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 2e1a1b824a3c..16317b063c20 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -69,6 +69,17 @@ struct vkms_writeback_job { pixel_write_t pixel_write; }; +/** + * enum pixel_read_direction - Enum used internaly by VKMS to represent a reading direction in a + * plane. + */ +enum pixel_read_direction { + READ_BOTTOM_TO_TOP, + READ_TOP_TO_BOTTOM, + READ_RIGHT_TO_LEFT, + READ_LEFT_TO_RIGHT +}; + /** * typedef pixel_read_t - These functions are used to read a pixel in the source frame, * convert it to `struct pixel_argb_u16` and write it to @out_pixel. diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c index 9a1400ad4db6..f76944874fe7 100644 --- a/drivers/gpu/drm/vkms/vkms_formats.c +++ b/drivers/gpu/drm/vkms/vkms_formats.c @@ -75,6 +75,36 @@ static void packed_pixels_addr(const struct vkms_frame_info *frame_info, *addr = (u8 *)frame_info->map[0].vaddr + offset; } +/** + * get_block_step_byte() - Common helper to compute the correct step value between each pixel block + * to read in a certain direction. + * + * @fb: Framebuffer to iter on + * @direction: Direction of the reading + * @plane_index: Plane to get the step from + * + * As the returned offset is the number of bytes between two consecutive blocks in a direction, + * the caller may have to read multiple pixel before using the next one (for example, to read from + * left to right in a DRM_FORMAT_R1 plane, each block contains 8 pixels, so the step must be used + * only every 8 pixels. + */ +static int get_block_step_byte(struct drm_framebuffer *fb, enum pixel_read_direction direction, + int plane_index) +{ + switch (direction) { + case READ_LEFT_TO_RIGHT: + return fb->format->char_per_block[plane_index]; + case READ_RIGHT_TO_LEFT: + return -fb->format->char_per_block[plane_index]; + case READ_TOP_TO_BOTTOM: + return (int)fb->pitches[plane_index]; + case READ_BOTTOM_TO_TOP: + return -(int)fb->pitches[plane_index]; + } + + return 0; +} + /** * packed_pixels_addr_1x1() - Get the pointer to the block containing the pixel at the given * coordinates
The pixel_read_direction enum is useful to describe the reading direction in a plane. It avoids using the rotation property of DRM, which not practical to know the direction of reading. This patch also introduce two helpers, one to compute the pixel_read_direction from the DRM rotation property, and one to compute the step, in byte, between two successive pixel in a specific direction. Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> --- drivers/gpu/drm/vkms/vkms_composer.c | 42 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/vkms/vkms_drv.h | 11 ++++++++++ drivers/gpu/drm/vkms/vkms_formats.c | 30 ++++++++++++++++++++++++++ 3 files changed, 83 insertions(+)