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

[Igor Torrente <igormtorrente@gmail.com>]

Hi Pekka,

On 4/20/22 10:13, Pekka Paalanen wrote:
> 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. :-)

Yeah, sure. :)

> 
>> 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.

Oops.

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

It fits.

> 
>> +
>> +	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.

I always forgot to update these comments. Thanks!

> 
>>   		 */
>> -		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.

I will remove this.

> 
>>   	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? 

I thought that I hadn't access to the crtc at the wb creation.

After looking more carefully at the structs, I found this is not the case.

So I will improve this.

> Can the CRTC (video mode) be smaller than the wb buffer?

AFAIK this is not possible.

> 
> 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?
> 

I don't really need it. I was just putting more easily accessible to the 
composer functions.

But np, I can change this.

> 
> 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 {
>