| Message ID | 20220404204515.42144-8-igormtorrente@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add new formats support to vkms | expand |
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 {
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 { >
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, + 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 + }; + + 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: * ((primary <- overlay) <- cursor) */ - 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, 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); } 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; 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 {
We will break the current assumption that the primary plane has the same size and position as CRTC. 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(-)