Received-SPF: pass (google.com: domain of linux-kernel+bounces-117090-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 25 Mar 2024 15:11:03 +0200
From: Pekka Paalanen <pekka.paalanen@collabora.com>
To: Louis Chauvet <louis.chauvet@bootlin.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>, Melissa Wen
 <melissa.srw@gmail.com>, =?UTF-8?B?TWHDrXJh?= Canal
 <mairacanal@riseup.net>, Haneen Mohammed <hamohammed.sa@gmail.com>, Daniel
 Vetter <daniel@ffwll.ch>, Maarten Lankhorst
 <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>,
 Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>,
 arthurgrillo@riseup.net, Jonathan Corbet <corbet@lwn.net>,
 dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
 jeremie.dautheribes@bootlin.com, miquel.raynal@bootlin.com,
 thomas.petazzoni@bootlin.com, seanpaul@google.com, marcheu@google.com,
 nicolejadeyee@google.com
Subject: Re: [PATCH v5 09/16] drm/vkms: Introduce pixel_read_direction enum
Message-ID: <20240325151103.0a5f7112.pekka.paalanen@collabora.com>
In-Reply-To: <20240313-yuv-v5-9-e610cbd03f52@bootlin.com>
References: <20240313-yuv-v5-0-e610cbd03f52@bootlin.com>
	<20240313-yuv-v5-9-e610cbd03f52@bootlin.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="Sig_/pOcUB5TKgO05bc1fdT4reYt";
 protocol="application/pgp-signature"; micalg=pgp-sha256

--Sig_/pOcUB5TKgO05bc1fdT4reYt
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable

On Wed, 13 Mar 2024 18:45:03 +0100
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.
>=20
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 36 ++++++++++++++++++++++++++++++=
++++++
>  drivers/gpu/drm/vkms/vkms_drv.h      | 11 +++++++++++
>  drivers/gpu/drm/vkms/vkms_formats.c  | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+)
>=20
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/=
vkms_composer.c
> index 9254086f23ff..989bcf59f375 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -159,6 +159,42 @@ static void apply_lut(const struct vkms_crtc_state *=
crtc_state, struct line_buff
>  	}
>  }
> =20
> +/**
> + * direction_for_rotation() - Get the correct reading direction for a gi=
ven rotation
> + *
> + * This function will use the @rotation setting of a source plane to com=
pute 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.

That is a well written description.

> + *
> + * @rotation: Rotation to analyze. It correspond the field @frame_info.r=
otation.
> + */
> +static enum pixel_read_direction direction_for_rotation(unsigned int rot=
ation)
> +{
> +	if (rotation & DRM_MODE_ROTATE_0) {
> +		if (rotation & DRM_MODE_REFLECT_X)
> +			return READ_RIGHT_TO_LEFT;
> +		else
> +			return READ_LEFT_TO_RIGHT;
> +	} else if (rotation & DRM_MODE_ROTATE_90) {
> +		if (rotation & DRM_MODE_REFLECT_Y)
> +			return READ_BOTTOM_TO_TOP;
> +		else
> +			return READ_TOP_TO_BOTTOM;
> +	} else if (rotation & DRM_MODE_ROTATE_180) {
> +		if (rotation & DRM_MODE_REFLECT_X)
> +			return READ_LEFT_TO_RIGHT;
> +		else
> +			return READ_RIGHT_TO_LEFT;
> +	} else if (rotation & DRM_MODE_ROTATE_270) {
> +		if (rotation & DRM_MODE_REFLECT_Y)
> +			return READ_TOP_TO_BOTTOM;
> +		else
> +			return READ_BOTTOM_TO_TOP;
> +	}
> +	return READ_LEFT_TO_RIGHT;

I'm a little worried seeing REFLECT_X is supported only for some
rotations, and REFLECT_Y for other rotations. Why is an analysis of all
combinations not necessary?

I hope IGT uses FB patterns instead of solid color in its tests of
rotation to be able to detect the difference.

The return values do seem correct to me, assuming I have guessed
correctly what "X" and "Y" refer to when combined with rotation. I did
not find good documentation about that.

Btw. if there are already functions that are able to transform
coordinates based on the rotation bitfield, you could alternatively use
them. Transform CRTC point (0, 0) to A, and (1, 0) to B. Now A and B
are in plane coordinate system, and vector B - A gives you the
direction. The reason I'm mentioning this is that then you don't have
to implement yet another copy of the rotation bitfield semantics from
scratch.


> +}
> +
>  /**
>   * 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 3ead8b39af4a..985e7a92b7bc 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;
>  };
> =20
> +/**
> + * 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 th=
e 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/v=
kms_formats.c
> index 649d75d05b1f..743b6fd06db5 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 =3D (u8 *)frame_info->map[0].vaddr + offset;
>  }
> =20
> +/**
> + * get_step_next_block() - Common helper to compute the correct step val=
ue between each pixel block
> + * to read in a certain direction.
> + *
> + * 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.
> + *
> + * @fb: Framebuffer to iter on
> + * @direction: Direction of the reading
> + * @plane_index: Plane to get the step from
> + */
> +static int get_step_next_block(struct drm_framebuffer *fb, enum pixel_re=
ad_direction direction,
> +			       int plane_index)
> +{

I would have called this something like get_block_step_bytes() for
example. That makes it clear it returns bytes (not e.g. pixels). "next"
implies to me that I tell the function the current block, and then it
gets me the next one. It does not do that, so I'd not use "next".

> +	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;
> +}

Looks good.


Thanks,
pq

> +
>  static void *get_packed_src_addr(const struct vkms_frame_info *frame_inf=
o, int y,
>  				 int plane_index)
>  {
>=20


--Sig_/pOcUB5TKgO05bc1fdT4reYt
Content-Type: application/pgp-signature
Content-Description: OpenPGP digital signature

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEJQjwWQChkWOYOIONI1/ltBGqqqcFAmYBd+cACgkQI1/ltBGq
qqfarg//Q+qvT9aL8AZvOG15t7LfHydp088OlP6y0I621RTKlpMf3u9o4uV6vM+n
8CW/h0xeXteJAt0/xBX/ZMziac8FLC1iQ9tKz4qoBd5EywoPwc69L6iANQvKsXuv
vvoF7SN/nv0iP/VDlrXuI1TIwGquIcs0Sf+tqhPOoYyg8LTM4RihmEeS3xgzaCO6
0fWhZbdnkTqOGXY/aPAtPC8xRvlG6rN1gLvNMaDxN/OXIJzhCSd/3Juau2+USqLL
6KzkmSoRsu/V+Uqnji4WlsPj0fLt+5BkvQcLfXkigv8x19RXAYB65HMAWuy42kwC
nxGsrvaDGKKxU0ltFb0uxzanEpi7mydfhGkTM75EeQFV77++SSsnYpZt48TkhM/7
WX8E9pOLtRfLvKfZrF5Wt2CS0LZ0M8E/UUyv97zf+HgU/0a7Tr66Ee4DPm0rvGL2
EhyPXzyhEfT/KqriaaiI54+KcQeOuOycoNikzfxj8XdyYp20b6vIGPSWVwNntKnO
SyrO3rx2oU21TdSkWHNNlGb8YY2rVtZGz0/jF93juipKcLraUnCy3pxRiB8+IO1y
x7BxEEMrozMz1/fvcIiU95CqbClMdraof5I96Avrx/Hh38iXfI1z4T3iFMMTtg3V
25KIJul1oqV8d23+PD8JMQhVm2wiCnQqjIfF9ynU1P7EwCsyIvk=
=F3pF
-----END PGP SIGNATURE-----

--Sig_/pOcUB5TKgO05bc1fdT4reYt--