On Thu, 6 Apr 2023 at 19:20, Maíra Canal <mcanal@xxxxxxxxxx> wrote: > > After commit 8ba1648567e2 ("drm: vkms: Refactor the plane composer to > accept new formats") the composition is no longer performed on top of > the primary plane, but instead on top of the CRTC, which means that > now we have a background. > > This opens to the possibility of coloring the background with a > personalized color. Therefore, create a module parameter that takes a > unsigned long number as an XRGB16161616 color and set the background > color to it. That said, the composition will be performed on top of > this background color. By default, the background color is black. > > Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx> > --- > > This patch intends to add a background color property to vkms through > a module parameter. Another option would be to implement this property > by adding a new KMS property that would indicate the background color. > It would be nice to hear other opinions on what would be the best > approach. These patches (and I think even igts for them) have been floating around for years. The hang-up is the userspace. Turns out all compositors want for background is black, thus far no one has figured out a real use-case for anything else. Maybe some time ... > Moreover, I wrote some IGT tests to ensure that the functionality is > working correctly [1]. The tests take the CRC of a colored primary > plane, offset the primary plane out of the screen, and take the CRC > of the colored background. The two CRC must be equal. > > [1] https://gitlab.freedesktop.org/mairacanal/igt-gpu-tools/-/tree/vkms/background-color I wonder whether it would be more useful to have this as a Kunit validation test to very that the vkms composing code works correctly? Still with the modparam and vkms_config to handle it all cleanly. -Daniel > > Best Regards, > - Maíra Canal > > --- > Documentation/gpu/vkms.rst | 2 -- > drivers/gpu/drm/vkms/vkms_composer.c | 20 ++++++++++++++------ > drivers/gpu/drm/vkms/vkms_drv.c | 6 ++++++ > drivers/gpu/drm/vkms/vkms_drv.h | 4 ++++ > 4 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst > index 49db221c0f52..dc01689d8c76 100644 > --- a/Documentation/gpu/vkms.rst > +++ b/Documentation/gpu/vkms.rst > @@ -121,8 +121,6 @@ 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. > > -- Add background color KMS property[Good to get started]. > - > - Full alpha blending on all planes. > > - Rotation, scaling. > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c > index 8e53fa80742b..07345faee98a 100644 > --- a/drivers/gpu/drm/vkms/vkms_composer.c > +++ b/drivers/gpu/drm/vkms/vkms_composer.c > @@ -79,7 +79,8 @@ static void fill_background(const struct pixel_argb_u16 *background_color, > * from all planes, calculates the crc32 of the output from the former step, > * and, if necessary, convert and store the output to the writeback buffer. > */ > -static void blend(struct vkms_writeback_job *wb, > +static void blend(struct vkms_device *vkms_dev, > + struct vkms_writeback_job *wb, > struct vkms_crtc_state *crtc_state, > u32 *crc32, struct line_buffer *stage_buffer, > struct line_buffer *output_buffer, size_t row_size) > @@ -87,7 +88,12 @@ static void blend(struct vkms_writeback_job *wb, > struct vkms_plane_state **plane = crtc_state->active_planes; > u32 n_active_planes = crtc_state->num_active_planes; > > - const struct pixel_argb_u16 background_color = { .a = 0xffff }; > + const struct pixel_argb_u16 background_color = { > + .a = 0xffff, > + .r = (*vkms_dev->config->background_color >> 32) & 0xffff, > + .g = (*vkms_dev->config->background_color >> 16) & 0xffff, > + .b = *vkms_dev->config->background_color & 0xffff, > + }; > > size_t crtc_y_limit = crtc_state->base.crtc->mode.vdisplay; > > @@ -139,7 +145,8 @@ static int check_iosys_map(struct vkms_crtc_state *crtc_state) > return 0; > } > > -static int compose_active_planes(struct vkms_writeback_job *active_wb, > +static int compose_active_planes(struct vkms_device *vkms_dev, > + struct vkms_writeback_job *active_wb, > struct vkms_crtc_state *crtc_state, > u32 *crc32) > { > @@ -178,7 +185,7 @@ static int compose_active_planes(struct vkms_writeback_job *active_wb, > goto free_stage_buffer; > } > > - blend(active_wb, crtc_state, crc32, &stage_buffer, > + blend(vkms_dev, active_wb, crtc_state, crc32, &stage_buffer, > &output_buffer, line_width * pixel_size); > > kvfree(output_buffer.pixels); > @@ -205,6 +212,7 @@ void vkms_composer_worker(struct work_struct *work) > struct drm_crtc *crtc = crtc_state->base.crtc; > struct vkms_writeback_job *active_wb = crtc_state->active_writeback; > struct vkms_output *out = drm_crtc_to_vkms_output(crtc); > + struct vkms_device *vkms_dev = vkms_output_to_vkms_device(out); > bool crc_pending, wb_pending; > u64 frame_start, frame_end; > u32 crc32 = 0; > @@ -228,9 +236,9 @@ void vkms_composer_worker(struct work_struct *work) > return; > > if (wb_pending) > - ret = compose_active_planes(active_wb, crtc_state, &crc32); > + ret = compose_active_planes(vkms_dev, active_wb, crtc_state, &crc32); > else > - ret = compose_active_planes(NULL, crtc_state, &crc32); > + ret = compose_active_planes(vkms_dev, NULL, crtc_state, &crc32); > > if (ret) > return; > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c > index 6d3a2d57d992..a4938dcb8c3e 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.c > +++ b/drivers/gpu/drm/vkms/vkms_drv.c > @@ -51,6 +51,10 @@ static bool enable_overlay; > module_param_named(enable_overlay, enable_overlay, bool, 0444); > MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support"); > > +static unsigned long background_color = 0x000000000000; > +module_param_named(background_color, background_color, ulong, 0644); > +MODULE_PARM_DESC(background_color, "Background color (0xRRRRGGGGBBBB)"); > + > DEFINE_DRM_GEM_FOPS(vkms_driver_fops); > > static void vkms_release(struct drm_device *dev) > @@ -99,6 +103,7 @@ static int vkms_config_show(struct seq_file *m, void *data) > seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback); > seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor); > seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay); > + seq_printf(m, "background_color=0x%lx\n", *vkmsdev->config->background_color); > > return 0; > } > @@ -226,6 +231,7 @@ static int __init vkms_init(void) > config->cursor = enable_cursor; > config->writeback = enable_writeback; > config->overlay = enable_overlay; > + config->background_color = &background_color; > > ret = vkms_create(config); > if (ret) > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index 4a248567efb2..4bc2e6a6d219 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -113,6 +113,7 @@ struct vkms_config { > bool writeback; > bool cursor; > bool overlay; > + unsigned long *background_color; > /* only set when instantiated */ > struct vkms_device *dev; > }; > @@ -127,6 +128,9 @@ struct vkms_device { > #define drm_crtc_to_vkms_output(target) \ > container_of(target, struct vkms_output, crtc) > > +#define vkms_output_to_vkms_device(target) \ > + container_of(target, struct vkms_device, output) > + > #define drm_device_to_vkms_device(target) \ > container_of(target, struct vkms_device, drm) > > -- > 2.39.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch