Re: [PATCH] drm/vkms: add module parameter to set background color

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux