[PATCH v5 7/9] drm: vkms: Supports to the case where primary plane doesn't match the CRTC
Igor Torrente
igormtorrente at gmail.com
Sun Apr 24 00:41:27 UTC 2022
Hi Pekka,
On 4/20/22 10:13, Pekka Paalanen wrote:
> On Mon, 4 Apr 2022 17:45:13 -0300
> Igor Torrente <igormtorrente at 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 at 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 at 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 {
>
More information about the dri-devel
mailing list