Re: [PATCH v5 7/9] drm: vkms: Supports to the case where primary plane doesn't match the CRTC

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7509 bytes --]

On Mon,  4 Apr 2022 17:45:13 -0300
Igor Torrente <igormtorrente@gmail.com> wrote:

> We will break the current assumption that the primary plane has the

Hi,

I'd say "remove" rather than "break". Breaking sounds bad but this is
good. :-)

> same size and position as CRTC.

...and that the primary plane is the bottom-most in zpos order, or is
even enabled. At least as far as the blending machinery is concerned.

> 
> For that we will add CRTC dimension information to `vkms_crtc_state`
> and add a opaque black backgound color.
> 
> Because now we need to fill the background, we had a loss in
> performance with this change. Results running the IGT[1] test
> `igt@kms_cursor_crc@pipe-a-cursor-512x512-onscreen` ten times:
> 
> |                  Frametime                   |
> |:--------------------------------------------:|
> |  Implementation |  Previous |   This commit  |
> |:---------------:|:---------:|:--------------:|
> | frametime range |  5~18 ms  |     10~22 ms   |
> |     Average     |  8.47 ms  |     12.32 ms   |
> 
> [1] IGT commit id: bc3f6833a12221a46659535dac06ebb312490eb4
> 
> Signed-off-by: Igor Torrente <igormtorrente@gmail.com>
> ---
>  Documentation/gpu/vkms.rst           |  3 +--
>  drivers/gpu/drm/vkms/vkms_composer.c | 32 +++++++++++++++++++---------
>  drivers/gpu/drm/vkms/vkms_crtc.c     |  4 ++++
>  drivers/gpu/drm/vkms/vkms_drv.h      |  2 ++
>  4 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> index a49e4ae92653..49db221c0f52 100644
> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -121,8 +121,7 @@ There's lots of plane features we could add support for:
>  - ARGB format on primary plane: blend the primary plane into background with
>    translucent alpha.
>  
> -- Support when the primary plane isn't exactly matching the output size: blend
> -  the primary plane into the black background.
> +- Add background color KMS property[Good to get started].
>  
>  - Full alpha blending on all planes.
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index cf24015bf90f..f80842227669 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -61,6 +61,15 @@ static bool check_y_limit(struct vkms_frame_info *frame_info, int y)
>  	return false;
>  }
>  
> +static void fill_background(struct pixel_argb_u16 *backgroud_color,

Hi,

this could be const struct pixel_argb_u16 *. Also a typo: missing n in
backgroud_color.

> +			    struct line_buffer *output_buffer)
> +{
> +	int i;
> +
> +	for (i = 0; i < output_buffer->n_pixels; i++)
> +		output_buffer->pixels[i] = *backgroud_color;
> +}
> +
>  /**
>   * @wb_frame_info: The writeback frame buffer metadata
>   * @crtc_state: The crtc state
> @@ -78,22 +87,23 @@ static void blend(struct vkms_writeback_job *wb,
>  		  struct line_buffer *output_buffer, s64 row_size)
>  {
>  	struct vkms_plane_state **plane = crtc_state->active_planes;
> -	struct vkms_frame_info *primary_plane_info = plane[0]->frame_info;
>  	u32 n_active_planes = crtc_state->num_active_planes;
>  
> -	int y_dst = primary_plane_info->dst.y1;
> -	int h_dst = drm_rect_height(&primary_plane_info->dst);
> -	int y_limit = y_dst + h_dst;
> +	struct pixel_argb_u16 background_color = (struct pixel_argb_u16) {
> +		.a = 0xffff
> +	};

Could be const and shorter, if that fits the kernel style:

	const struct pixel_arb_u16 background_color = { .a = 0xffff };

> +
> +	int crtc_y_limit = crtc_state->crtc_height;
>  	int y, i;
>  
> -	for (y = y_dst; y < y_limit; y++) {
> -		plane[0]->format_func(output_buffer, primary_plane_info, y);
> +	for (y = 0; y < crtc_y_limit; y++) {
> +		fill_background(&background_color, output_buffer);
>  
>  		/* If there are other planes besides primary, we consider the active
>  		 * planes should be in z-order and compose them associatively:

Is "associatively" the right word here?

>  		 * ((primary <- overlay) <- cursor)

The example (primary <- overlay) is not generally true with real hardware.

Maybe what you are trying to say is: The active planes are composed in
z-order.

>  		 */
> -		for (i = 1; i < n_active_planes; i++) {
> +		for (i = 0; i < n_active_planes; i++) {
>  			if (!check_y_limit(plane[i]->frame_info, y))
>  				continue;
>  
> @@ -154,7 +164,7 @@ static int compose_active_planes(struct vkms_writeback_job *active_wb,

As I mentioned on the previous patch, I think the finding of primary
plane (which was generally incorrect) should be removed here.

>  	if (WARN_ON(check_format_funcs(crtc_state, active_wb)))
>  		return -EINVAL;
>  
> -	line_width = drm_rect_width(&primary_plane_info->dst);
> +	line_width = crtc_state->crtc_width;
>  	stage_buffer.n_pixels = line_width;
>  	output_buffer.n_pixels = line_width;
>  
> @@ -175,8 +185,10 @@ static int compose_active_planes(struct vkms_writeback_job *active_wb,
>  		struct vkms_frame_info *wb_frame_info = &active_wb->frame_info;
>  
>  		wb_format = wb_frame_info->fb->format->format;
> -		wb_frame_info->src = primary_plane_info->src;
> -		wb_frame_info->dst = primary_plane_info->dst;
> +		drm_rect_init(&wb_frame_info->src, 0, 0, crtc_state->crtc_width,
> +			      crtc_state->crtc_height);
> +		drm_rect_init(&wb_frame_info->dst, 0, 0, crtc_state->crtc_width,
> +			      crtc_state->crtc_height);

Why are these not set when the active_wb->frame_info is created? Can
the CRTC (video mode) be smaller than the wb buffer?

Somewhere you must have a check that wb buffer size can fit the crtc
size, or maybe they must be exactly the same size. At least setting
destination rectangle bigger than the buffer dimensions must be
impossible.

>  	}
>  
>  	blend(active_wb, crtc_state, crc32, &stage_buffer,
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 57bbd32e9beb..4a37e243c2d7 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -248,7 +248,9 @@ static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
>  static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
>  				   struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>  	struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> +	struct drm_display_mode *mode = &crtc_state->mode;
>  
>  	if (crtc->state->event) {
>  		spin_lock(&crtc->dev->event_lock);
> @@ -264,6 +266,8 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
>  	}
>  
>  	vkms_output->composer_state = to_vkms_crtc_state(crtc->state);
> +	vkms_output->composer_state->crtc_width = mode->hdisplay;
> +	vkms_output->composer_state->crtc_height = mode->vdisplay;

Is the crtc not keeping track of the current mode, do you really need
your own crtc_width and crtc_height?


Thanks,
pq

>  
>  	spin_unlock_irq(&vkms_output->lock);
>  }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 2704cfb6904b..ab92d9f7b701 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -90,6 +90,8 @@ struct vkms_crtc_state {
>  	bool wb_pending;
>  	u64 frame_start;
>  	u64 frame_end;
> +	u16 crtc_width;
> +	u16 crtc_height;
>  };
>  
>  struct vkms_output {


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]