| Message ID | 20240409-yuv-v6-7-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:25 +0200 Louis Chauvet <louis.chauvet@bootlin.com> wrote: > Introduce the usage of block_h/block_w to compute the offset and the > pointer of a pixel. The previous implementation was specialized for > planes with block_h == block_w == 1. To avoid confusion and allow easier > implementation of tiled formats. It also remove the usage of the > deprecated format field `cpp`. > > Introduce the plane_index parameter to get an offset/pointer on a > different plane. > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > --- > drivers/gpu/drm/vkms/vkms_formats.c | 110 ++++++++++++++++++++++++++++-------- > 1 file changed, 87 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c > index 69cf9733fec5..9a1400ad4db6 100644 > --- a/drivers/gpu/drm/vkms/vkms_formats.c > +++ b/drivers/gpu/drm/vkms/vkms_formats.c > @@ -10,22 +10,43 @@ > #include "vkms_formats.h" > > /** > - * pixel_offset() - Get the offset of the pixel at coordinates x/y in the first plane > + * packed_pixels_offset() - Get the offset of the block containing the pixel at coordinates x/y > * > * @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 > + * @plane_index: The index of the plane to use > + * @offset: The returned offset inside the buffer of the block > + * @rem_x,@rem_y: The returned coordinate of the requested pixel in the block > * > - * 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 point to an other pixel or > - * outside of the buffer. > + * As some pixel formats store multiple pixels in a block (DRM_FORMAT_R* for example), some > + * pixels are not individually addressable. This function return 3 values: the offset of the > + * whole block, and the coordinate of the requested pixel inside this block. > + * For example, if the format is DRM_FORMAT_R1 and the requested coordinate is 13,5, the offset > + * will point to the byte 5*pitches + 13/8 (second byte of the 5th line), and the rem_x/rem_y > + * coordinates will be (13 % 8, 5 % 1) = (5, 0) > + * > + * With this function, the caller just have to extract the correct pixel from the block. > */ > -static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y) > +static void packed_pixels_offset(const struct vkms_frame_info *frame_info, int x, int y, > + int plane_index, int *offset, int *rem_x, int *rem_y) > { > struct drm_framebuffer *fb = frame_info->fb; > + const struct drm_format_info *format = frame_info->fb->format; > + /* Directly using x and y to multiply pitches and format->ccp is not sufficient because > + * in some formats a block can represent multiple pixels. > + * > + * Dividing x and y by the block size allows to extract the correct offset of the block > + * containing the pixel. > + */ > > - return fb->offsets[0] + (y * fb->pitches[0]) + (x * fb->format->cpp[0]); > + int block_x = x / drm_format_info_block_width(format, plane_index); > + int block_y = y / drm_format_info_block_height(format, plane_index); > + *rem_x = x % drm_format_info_block_width(format, plane_index); > + *rem_y = y % drm_format_info_block_height(format, plane_index); > + *offset = fb->offsets[plane_index] + > + block_y * fb->pitches[plane_index] + > + block_x * format->char_per_block[plane_index]; I started thinking... is + block_y * fb->pitches[plane_index] + correct, or should it be + y * fb->pitches[plane_index] + ? I'm looking at drm_format_info_min_pitch() which sounds like it should be the latter? Because of return DIV_ROUND_UP_ULL((u64)buffer_width * info->char_per_block[plane], drm_format_info_block_width(info, plane) * drm_format_info_block_height(info, plane)); in drm_format_info_min_pitch(). Btw. maybe this should check that the result is not negative (e.g. due to overflow)? Or does that even work since signed overflow is undefined behavior (UB) and compilers may assume UB does not happen, causing the check to be eliminated as dead code? Otherwise this patch looks ok to me. Thanks, pq > } > > /** > @@ -35,30 +56,70 @@ static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int > * @frame_info: Buffer metadata > * @x: The x (width) coordinate inside the plane > * @y: The y (height) coordinate inside the plane > + * @plane_index: The index of the plane > + * @addr: The returned pointer > + * @rem_x,@rem_y: The returned coordinate of the requested pixel in the block > * > - * Takes the information stored in the frame_info, a pair of coordinates, and > - * returns the address of the first color channel. > - * This function assumes the channels are packed together, i.e. a color channel > - * comes immediately after another in the memory. And therefore, this function > - * doesn't work for YUV with chroma subsampling (e.g. YUV420 and NV21). > + * Takes the information stored in the frame_info, a pair of coordinates, and returns the address > + * of the block containing this pixel and the pixel position inside this block. > * > - * The caller must ensure that the framebuffer associated with this request uses a pixel format > - * where block_h == block_w == 1, otherwise the returned pointer can be outside the buffer. > + * See @packed_pixel_offset for details about rem_x/rem_y behavior. > */ > -static void *packed_pixels_addr(const struct vkms_frame_info *frame_info, > - int x, int y) > +static void packed_pixels_addr(const struct vkms_frame_info *frame_info, > + int x, int y, int plane_index, u8 **addr, int *rem_x, > + int *rem_y) > { > - size_t offset = pixel_offset(frame_info, x, y); > + int offset; > > - return (u8 *)frame_info->map[0].vaddr + offset; > + packed_pixels_offset(frame_info, x, y, plane_index, &offset, rem_x, rem_y); > + *addr = (u8 *)frame_info->map[0].vaddr + offset; > } > > -static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y) > +/** > + * packed_pixels_addr_1x1() - Get the pointer to the block containing the pixel at the given > + * coordinates > + * > + * @frame_info: Buffer metadata > + * @x: The x (width) coordinate inside the plane > + * @y: The y (height) coordinate inside the plane > + * @plane_index: The index of the plane > + * @addr: The returned pointer > + * > + * This function can only be used with format where block_h == block_w == 1. > + */ > +static void packed_pixels_addr_1x1(const struct vkms_frame_info *frame_info, > + int x, int y, int plane_index, u8 **addr) > +{ > + int offset, rem_x, rem_y; > + > + WARN_ONCE(drm_format_info_block_width(frame_info->fb->format, > + plane_index) != 1, > + "%s() only support formats with block_w == 1", __func__); > + WARN_ONCE(drm_format_info_block_height(frame_info->fb->format, > + plane_index) != 1, > + "%s() only support formats with block_h == 1", __func__); > + > + packed_pixels_offset(frame_info, x, y, plane_index, &offset, &rem_x, > + &rem_y); > + *addr = (u8 *)frame_info->map[0].vaddr + offset; > +} > + > +static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y, > + int plane_index) > { > int x_src = frame_info->src.x1 >> 16; > int y_src = y - frame_info->rotated.y1 + (frame_info->src.y1 >> 16); > + u8 *addr; > + int rem_x, rem_y; > + > + WARN_ONCE(drm_format_info_block_width(frame_info->fb->format, plane_index) != 1, > + "%s() only support formats with block_w == 1", __func__); > + WARN_ONCE(drm_format_info_block_height(frame_info->fb->format, plane_index) != 1, > + "%s() only support formats with block_h == 1", __func__); > > - return packed_pixels_addr(frame_info, x_src, y_src); > + packed_pixels_addr(frame_info, x_src, y_src, plane_index, &addr, &rem_x, &rem_y); > + > + return addr; > } > > static int get_x_position(const struct vkms_frame_info *frame_info, int limit, int x) > @@ -167,14 +228,14 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state > { > struct pixel_argb_u16 *out_pixels = stage_buffer->pixels; > struct vkms_frame_info *frame_info = plane->frame_info; > - u8 *src_pixels = get_packed_src_addr(frame_info, y); > + u8 *src_pixels = get_packed_src_addr(frame_info, y, 0); > int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels); > > for (size_t x = 0; x < limit; x++, src_pixels += frame_info->fb->format->cpp[0]) { > int x_pos = get_x_position(frame_info, limit, x); > > if (drm_rotation_90_or_270(frame_info->rotation)) > - src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1) > + src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1, 0) > + frame_info->fb->format->cpp[0] * y; > > plane->pixel_read(src_pixels, &out_pixels[x_pos]); > @@ -275,7 +336,10 @@ void vkms_writeback_row(struct vkms_writeback_job *wb, > { > struct vkms_frame_info *frame_info = &wb->wb_frame_info; > int x_dst = frame_info->dst.x1; > - u8 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y); > + u8 *dst_pixels; > + int rem_x, rem_y; > + > + packed_pixels_addr(frame_info, x_dst, y, 0, &dst_pixels, &rem_x, &rem_y); > struct pixel_argb_u16 *in_pixels = src_buffer->pixels; > int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst), src_buffer->n_pixels); > >
Le 22/04/24 - 14:07, Pekka Paalanen a écrit : > On Tue, 09 Apr 2024 15:25:25 +0200 > Louis Chauvet <louis.chauvet@bootlin.com> wrote: > > > Introduce the usage of block_h/block_w to compute the offset and the > > pointer of a pixel. The previous implementation was specialized for > > planes with block_h == block_w == 1. To avoid confusion and allow easier > > implementation of tiled formats. It also remove the usage of the > > deprecated format field `cpp`. > > > > Introduce the plane_index parameter to get an offset/pointer on a > > different plane. > > > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > > --- > > drivers/gpu/drm/vkms/vkms_formats.c | 110 ++++++++++++++++++++++++++++-------- > > 1 file changed, 87 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c > > index 69cf9733fec5..9a1400ad4db6 100644 > > --- a/drivers/gpu/drm/vkms/vkms_formats.c > > +++ b/drivers/gpu/drm/vkms/vkms_formats.c > > @@ -10,22 +10,43 @@ > > #include "vkms_formats.h" > > > > /** > > - * pixel_offset() - Get the offset of the pixel at coordinates x/y in the first plane > > + * packed_pixels_offset() - Get the offset of the block containing the pixel at coordinates x/y > > * > > * @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 > > + * @plane_index: The index of the plane to use > > + * @offset: The returned offset inside the buffer of the block > > + * @rem_x,@rem_y: The returned coordinate of the requested pixel in the block > > * > > - * 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 point to an other pixel or > > - * outside of the buffer. > > + * As some pixel formats store multiple pixels in a block (DRM_FORMAT_R* for example), some > > + * pixels are not individually addressable. This function return 3 values: the offset of the > > + * whole block, and the coordinate of the requested pixel inside this block. > > + * For example, if the format is DRM_FORMAT_R1 and the requested coordinate is 13,5, the offset > > + * will point to the byte 5*pitches + 13/8 (second byte of the 5th line), and the rem_x/rem_y > > + * coordinates will be (13 % 8, 5 % 1) = (5, 0) > > + * > > + * With this function, the caller just have to extract the correct pixel from the block. > > */ > > -static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y) > > +static void packed_pixels_offset(const struct vkms_frame_info *frame_info, int x, int y, > > + int plane_index, int *offset, int *rem_x, int *rem_y) > > { > > struct drm_framebuffer *fb = frame_info->fb; > > + const struct drm_format_info *format = frame_info->fb->format; > > + /* Directly using x and y to multiply pitches and format->ccp is not sufficient because > > + * in some formats a block can represent multiple pixels. > > + * > > + * Dividing x and y by the block size allows to extract the correct offset of the block > > + * containing the pixel. > > + */ > > > > - return fb->offsets[0] + (y * fb->pitches[0]) + (x * fb->format->cpp[0]); > > + int block_x = x / drm_format_info_block_width(format, plane_index); > > + int block_y = y / drm_format_info_block_height(format, plane_index); > > + *rem_x = x % drm_format_info_block_width(format, plane_index); > > + *rem_y = y % drm_format_info_block_height(format, plane_index); > > + *offset = fb->offsets[plane_index] + > > + block_y * fb->pitches[plane_index] + > > + block_x * format->char_per_block[plane_index]; > > I started thinking... is > > + block_y * fb->pitches[plane_index] + > > correct, or should it be > > + y * fb->pitches[plane_index] + > > ? The documentation is not very clear about that: * @pitches: Line stride per buffer. For userspace created object this * is copied from drm_mode_fb_cmd2. If I look at the drm_mode_fb_cmd2, there is this documentation: /** @pitches: Pitch (aka. stride) in bytes, one per plane. */ For me, I interpret "stride" as it is used in matrix calculation, where it means "the number of bytes between two number adjacent verticaly" (&matrix[x,y] + stride == &matrix[x,y+1]). So in a graphic context, I interpret a stride as the number of byte to reach the next line of blocks (as pixels can not always be accessed individually). So, for me, buffer_size_in_byte >= stride * number_of_lines. > I'm looking at drm_format_info_min_pitch() which sounds like it should > be the latter? Because of > > return DIV_ROUND_UP_ULL((u64)buffer_width * info->char_per_block[plane], > drm_format_info_block_width(info, plane) * > drm_format_info_block_height(info, plane)); > > in drm_format_info_min_pitch(). This function doesn't make sense to me. I can't understand how it could work. If I consider the X0L2 format, with block_h == block_w == 2, char_per_block = 8, and a framebuffer of 1 * 10 pixels, the result of drm_format_info_min_pitch is 2. However, for this format and this framebuffer, I think the stride should be at least 8 bytes (the buffer is "1 block width"). If pitch equals 2 (as suggested by the test), it implies that height * pitch is not valid for calculating the minimum size of the buffer (in our case, 10 * 2 = 20 bytes, but the minimum framebuffer size should be 5 blocks * 8 bytes_per_block = 40 bytes). And I don't understand what the 2 represents in this context. Is it the number of pixels on a line (which is strange, because pitch should be in byte)? The width in byte of the first line, but by using the "byte per pixel" value (which make no sense when using block formats)? If pitch equals 8 (as I would expect), height * pitch is not optimal (but at least sufficient to contain the entire framebuffer), and height * pitch / block_h is optimal (which is logical, because height/block_h is the number of block per columns). > Btw. maybe this should check that the result is not negative (e.g. due > to overflow)? Or does that even work since signed overflow is undefined > behavior (UB) and compilers may assume UB does not happen, causing the > check to be eliminated as dead code? > > Otherwise this patch looks ok to me. > > > Thanks, > pq > [...] -- Louis Chauvet, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Mon, 13 May 2024 09:15:13 +0200 Louis Chauvet <louis.chauvet@bootlin.com> wrote: > Le 22/04/24 - 14:07, Pekka Paalanen a écrit : > > On Tue, 09 Apr 2024 15:25:25 +0200 > > Louis Chauvet <louis.chauvet@bootlin.com> wrote: > > > > > Introduce the usage of block_h/block_w to compute the offset and the > > > pointer of a pixel. The previous implementation was specialized for > > > planes with block_h == block_w == 1. To avoid confusion and allow easier > > > implementation of tiled formats. It also remove the usage of the > > > deprecated format field `cpp`. > > > > > > Introduce the plane_index parameter to get an offset/pointer on a > > > different plane. > > > > > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > > > --- > > > drivers/gpu/drm/vkms/vkms_formats.c | 110 ++++++++++++++++++++++++++++-------- > > > 1 file changed, 87 insertions(+), 23 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c > > > index 69cf9733fec5..9a1400ad4db6 100644 > > > --- a/drivers/gpu/drm/vkms/vkms_formats.c > > > +++ b/drivers/gpu/drm/vkms/vkms_formats.c > > > @@ -10,22 +10,43 @@ > > > #include "vkms_formats.h" > > > > > > /** > > > - * pixel_offset() - Get the offset of the pixel at coordinates x/y in the first plane > > > + * packed_pixels_offset() - Get the offset of the block containing the pixel at coordinates x/y > > > * > > > * @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 > > > + * @plane_index: The index of the plane to use > > > + * @offset: The returned offset inside the buffer of the block > > > + * @rem_x,@rem_y: The returned coordinate of the requested pixel in the block > > > * > > > - * 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 point to an other pixel or > > > - * outside of the buffer. > > > + * As some pixel formats store multiple pixels in a block (DRM_FORMAT_R* for example), some > > > + * pixels are not individually addressable. This function return 3 values: the offset of the > > > + * whole block, and the coordinate of the requested pixel inside this block. > > > + * For example, if the format is DRM_FORMAT_R1 and the requested coordinate is 13,5, the offset > > > + * will point to the byte 5*pitches + 13/8 (second byte of the 5th line), and the rem_x/rem_y > > > + * coordinates will be (13 % 8, 5 % 1) = (5, 0) > > > + * > > > + * With this function, the caller just have to extract the correct pixel from the block. > > > */ > > > -static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y) > > > +static void packed_pixels_offset(const struct vkms_frame_info *frame_info, int x, int y, > > > + int plane_index, int *offset, int *rem_x, int *rem_y) > > > { > > > struct drm_framebuffer *fb = frame_info->fb; > > > + const struct drm_format_info *format = frame_info->fb->format; > > > + /* Directly using x and y to multiply pitches and format->ccp is not sufficient because > > > + * in some formats a block can represent multiple pixels. > > > + * > > > + * Dividing x and y by the block size allows to extract the correct offset of the block > > > + * containing the pixel. > > > + */ > > > > > > - return fb->offsets[0] + (y * fb->pitches[0]) + (x * fb->format->cpp[0]); > > > + int block_x = x / drm_format_info_block_width(format, plane_index); > > > + int block_y = y / drm_format_info_block_height(format, plane_index); > > > + *rem_x = x % drm_format_info_block_width(format, plane_index); > > > + *rem_y = y % drm_format_info_block_height(format, plane_index); > > > + *offset = fb->offsets[plane_index] + > > > + block_y * fb->pitches[plane_index] + > > > + block_x * format->char_per_block[plane_index]; > > > > I started thinking... is > > > > + block_y * fb->pitches[plane_index] + > > > > correct, or should it be > > > > + y * fb->pitches[plane_index] + > > > > ? > > The documentation is not very clear about that: > > * @pitches: Line stride per buffer. For userspace created object this > * is copied from drm_mode_fb_cmd2. > > If I look at the drm_mode_fb_cmd2, there is this documentation: > > /** @pitches: Pitch (aka. stride) in bytes, one per plane. */ > > For me, I interpret "stride" as it is used in matrix calculation, where it > means "the number of bytes between two number adjacent verticaly" > (&matrix[x,y] + stride == &matrix[x,y+1]). > > So in a graphic context, I interpret a stride as the number of byte to > reach the next line of blocks (as pixels can not always be accessed > individually). > > So, for me, buffer_size_in_byte >= stride * number_of_lines. This is the definition, yes. Even for blocky formats, it is still number of 1-pixel-high lines, even though one cannot address a line as such. For blocky formats it is a theoretical value, and computing with it only makes sense when your number of lines is a multiple of block height. That's my recollection. This has been hashed in issues like https://gitlab.freedesktop.org/wayland/weston/-/issues/896 > > I'm looking at drm_format_info_min_pitch() which sounds like it should > > be the latter? Because of > > > > return DIV_ROUND_UP_ULL((u64)buffer_width * info->char_per_block[plane], > > drm_format_info_block_width(info, plane) * > > drm_format_info_block_height(info, plane)); > > > > in drm_format_info_min_pitch(). > > This function doesn't make sense to me. I can't understand how it could > work. > > If I consider the X0L2 format, with block_h == block_w == 2, > char_per_block = 8, and a framebuffer of 1 * 10 pixels, the result of > drm_format_info_min_pitch is 2. If buffer_width is 1, then buffer_width / block_w is 1 because of rounding up. Cannot have images with partial blocks. That is a block-row of one block. That block takes char_per_block bytes, that is, 8 bytes here. But block height is 2, and stride is computed for 1-pixel-high line, so we divide by block_h, and the result is stride = 4 bytes. However, 1 * 8 / (2 * 2) = 2. I think the code is bugged, and the round-up happens at a wrong point. The correct form would be div_round_up(div_round_up(buffer_width, block_w) * char_per_block, block_h) in my opinion. There must always be an integer number of blocks on a block-row. If a block-row has multiple blocks, doing a div_round_up(char_per_block, block_h) would over-estimate the per-block bytes and add the extra for each block rather than averaging it out. So, multiplying by char_per_block before dividing by block_h gives a stricter minimum stride. The condition buffer_size_bytes >= buffer_height * stride is necessary but not always sufficient, because the buffer must hold an integer number of block-rows. > However, for this format and this framebuffer, I think the stride should > be at least 8 bytes (the buffer is "1 block width"). I believe the stride is 4 bytes, because stride for a 1-pixel-high line on average, rounded up. Stride allows for unused blocks per block-row, but its use for buffer size checking with buffer_height is incomplete, I believe. For the question "is buffer_size enough?" it may produce false-positives, but it cannot produce false-negatives. Thanks, pq > If pitch equals 2 (as suggested by the test), it implies that > height * pitch is not valid for calculating the minimum size of the buffer > (in our case, 10 * 2 = 20 bytes, but the minimum framebuffer size should > be 5 blocks * 8 bytes_per_block = 40 bytes). And I don't understand what > the 2 represents in this context. > Is it the number of pixels on a line (which is strange, because pitch > should be in byte)? The width in byte of the first line, but by using the > "byte per pixel" value (which make no sense when using block formats)? > > If pitch equals 8 (as I would expect), height * pitch is not optimal (but > at least sufficient to contain the entire framebuffer), and height * pitch > / block_h is optimal (which is logical, because height/block_h is the > number of block per columns). > > > Btw. maybe this should check that the result is not negative (e.g. due > > to overflow)? Or does that even work since signed overflow is undefined > > behavior (UB) and compilers may assume UB does not happen, causing the > > check to be eliminated as dead code? > > > > Otherwise this patch looks ok to me. > > > > > > Thanks, > > pq > > > > [...] > > -- > Louis Chauvet, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c index 69cf9733fec5..9a1400ad4db6 100644 --- a/drivers/gpu/drm/vkms/vkms_formats.c +++ b/drivers/gpu/drm/vkms/vkms_formats.c @@ -10,22 +10,43 @@ #include "vkms_formats.h" /** - * pixel_offset() - Get the offset of the pixel at coordinates x/y in the first plane + * packed_pixels_offset() - Get the offset of the block containing the pixel at coordinates x/y * * @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 + * @plane_index: The index of the plane to use + * @offset: The returned offset inside the buffer of the block + * @rem_x,@rem_y: The returned coordinate of the requested pixel in the block * - * 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 point to an other pixel or - * outside of the buffer. + * As some pixel formats store multiple pixels in a block (DRM_FORMAT_R* for example), some + * pixels are not individually addressable. This function return 3 values: the offset of the + * whole block, and the coordinate of the requested pixel inside this block. + * For example, if the format is DRM_FORMAT_R1 and the requested coordinate is 13,5, the offset + * will point to the byte 5*pitches + 13/8 (second byte of the 5th line), and the rem_x/rem_y + * coordinates will be (13 % 8, 5 % 1) = (5, 0) + * + * With this function, the caller just have to extract the correct pixel from the block. */ -static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y) +static void packed_pixels_offset(const struct vkms_frame_info *frame_info, int x, int y, + int plane_index, int *offset, int *rem_x, int *rem_y) { struct drm_framebuffer *fb = frame_info->fb; + const struct drm_format_info *format = frame_info->fb->format; + /* Directly using x and y to multiply pitches and format->ccp is not sufficient because + * in some formats a block can represent multiple pixels. + * + * Dividing x and y by the block size allows to extract the correct offset of the block + * containing the pixel. + */ - return fb->offsets[0] + (y * fb->pitches[0]) + (x * fb->format->cpp[0]); + int block_x = x / drm_format_info_block_width(format, plane_index); + int block_y = y / drm_format_info_block_height(format, plane_index); + *rem_x = x % drm_format_info_block_width(format, plane_index); + *rem_y = y % drm_format_info_block_height(format, plane_index); + *offset = fb->offsets[plane_index] + + block_y * fb->pitches[plane_index] + + block_x * format->char_per_block[plane_index]; } /** @@ -35,30 +56,70 @@ static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int * @frame_info: Buffer metadata * @x: The x (width) coordinate inside the plane * @y: The y (height) coordinate inside the plane + * @plane_index: The index of the plane + * @addr: The returned pointer + * @rem_x,@rem_y: The returned coordinate of the requested pixel in the block * - * Takes the information stored in the frame_info, a pair of coordinates, and - * returns the address of the first color channel. - * This function assumes the channels are packed together, i.e. a color channel - * comes immediately after another in the memory. And therefore, this function - * doesn't work for YUV with chroma subsampling (e.g. YUV420 and NV21). + * Takes the information stored in the frame_info, a pair of coordinates, and returns the address + * of the block containing this pixel and the pixel position inside this block. * - * The caller must ensure that the framebuffer associated with this request uses a pixel format - * where block_h == block_w == 1, otherwise the returned pointer can be outside the buffer. + * See @packed_pixel_offset for details about rem_x/rem_y behavior. */ -static void *packed_pixels_addr(const struct vkms_frame_info *frame_info, - int x, int y) +static void packed_pixels_addr(const struct vkms_frame_info *frame_info, + int x, int y, int plane_index, u8 **addr, int *rem_x, + int *rem_y) { - size_t offset = pixel_offset(frame_info, x, y); + int offset; - return (u8 *)frame_info->map[0].vaddr + offset; + packed_pixels_offset(frame_info, x, y, plane_index, &offset, rem_x, rem_y); + *addr = (u8 *)frame_info->map[0].vaddr + offset; } -static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y) +/** + * packed_pixels_addr_1x1() - Get the pointer to the block containing the pixel at the given + * coordinates + * + * @frame_info: Buffer metadata + * @x: The x (width) coordinate inside the plane + * @y: The y (height) coordinate inside the plane + * @plane_index: The index of the plane + * @addr: The returned pointer + * + * This function can only be used with format where block_h == block_w == 1. + */ +static void packed_pixels_addr_1x1(const struct vkms_frame_info *frame_info, + int x, int y, int plane_index, u8 **addr) +{ + int offset, rem_x, rem_y; + + WARN_ONCE(drm_format_info_block_width(frame_info->fb->format, + plane_index) != 1, + "%s() only support formats with block_w == 1", __func__); + WARN_ONCE(drm_format_info_block_height(frame_info->fb->format, + plane_index) != 1, + "%s() only support formats with block_h == 1", __func__); + + packed_pixels_offset(frame_info, x, y, plane_index, &offset, &rem_x, + &rem_y); + *addr = (u8 *)frame_info->map[0].vaddr + offset; +} + +static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y, + int plane_index) { int x_src = frame_info->src.x1 >> 16; int y_src = y - frame_info->rotated.y1 + (frame_info->src.y1 >> 16); + u8 *addr; + int rem_x, rem_y; + + WARN_ONCE(drm_format_info_block_width(frame_info->fb->format, plane_index) != 1, + "%s() only support formats with block_w == 1", __func__); + WARN_ONCE(drm_format_info_block_height(frame_info->fb->format, plane_index) != 1, + "%s() only support formats with block_h == 1", __func__); - return packed_pixels_addr(frame_info, x_src, y_src); + packed_pixels_addr(frame_info, x_src, y_src, plane_index, &addr, &rem_x, &rem_y); + + return addr; } static int get_x_position(const struct vkms_frame_info *frame_info, int limit, int x) @@ -167,14 +228,14 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state { struct pixel_argb_u16 *out_pixels = stage_buffer->pixels; struct vkms_frame_info *frame_info = plane->frame_info; - u8 *src_pixels = get_packed_src_addr(frame_info, y); + u8 *src_pixels = get_packed_src_addr(frame_info, y, 0); int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels); for (size_t x = 0; x < limit; x++, src_pixels += frame_info->fb->format->cpp[0]) { int x_pos = get_x_position(frame_info, limit, x); if (drm_rotation_90_or_270(frame_info->rotation)) - src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1) + src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1, 0) + frame_info->fb->format->cpp[0] * y; plane->pixel_read(src_pixels, &out_pixels[x_pos]); @@ -275,7 +336,10 @@ void vkms_writeback_row(struct vkms_writeback_job *wb, { struct vkms_frame_info *frame_info = &wb->wb_frame_info; int x_dst = frame_info->dst.x1; - u8 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y); + u8 *dst_pixels; + int rem_x, rem_y; + + packed_pixels_addr(frame_info, x_dst, y, 0, &dst_pixels, &rem_x, &rem_y); struct pixel_argb_u16 *in_pixels = src_buffer->pixels; int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst), src_buffer->n_pixels);
Introduce the usage of block_h/block_w to compute the offset and the pointer of a pixel. The previous implementation was specialized for planes with block_h == block_w == 1. To avoid confusion and allow easier implementation of tiled formats. It also remove the usage of the deprecated format field `cpp`. Introduce the plane_index parameter to get an offset/pointer on a different plane. Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> --- drivers/gpu/drm/vkms/vkms_formats.c | 110 ++++++++++++++++++++++++++++-------- 1 file changed, 87 insertions(+), 23 deletions(-)