Message-ID: <20240422140757.576e363b.pekka.paalanen@collabora.com>
Date: Mon, 22 Apr 2024 14:07:57 +0300
From: Pekka Paalanen <pekka.paalanen@...labora.com>
To: Louis Chauvet <louis.chauvet@...tlin.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>, Melissa Wen
 <melissa.srw@...il.com>, Maíra Canal
 <mairacanal@...eup.net>, Haneen Mohammed <hamohammed.sa@...il.com>, Daniel
 Vetter <daniel@...ll.ch>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 rdunlap@...radead.org, arthurgrillo@...eup.net, Jonathan Corbet
 <corbet@....net>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, jeremie.dautheribes@...tlin.com,
 miquel.raynal@...tlin.com, thomas.petazzoni@...tlin.com,
 seanpaul@...gle.com, marcheu@...gle.com, nicolejadeyee@...gle.com
Subject: Re: [PATCH v6 07/17] drm/vkms: Update pixels accessor to support
 packed and multi-plane formats.

On Tue, 09 Apr 2024 15:25:25 +0200
Louis Chauvet <louis.chauvet@...tlin.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@...tlin.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);
>  
> 


Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists