| Message ID | 20230410125435.128689-1-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/vkms: add module parameter to set background color | expand |
I think this should be a KMS property instead of a module parameter. Is there a reason why this patch uses a module parameter? It breaks user-space expectations.
On 04/10, Simon Ser wrote: > I think this should be a KMS property instead of a module parameter. > Is there a reason why this patch uses a module parameter? It breaks > user-space expectations. Hi Simon, a KMS property is what we have on vkms TODO [1] and the module parameter was Maíra's first step to open a discussion for this property [2]. AFAIK, we would need to create the KMS property first, but it seems there isn't an userspace case/need to support this API change. Do you know any valid use cases to support a bkg color property? BR, Melissa [1] https://dri.freedesktop.org/docs/drm/gpu/vkms.html#add-plane-features [2] https://lore.kernel.org/dri-devel/20230406172002.124456-1-mcanal@igalia.com/
Hi, On Monday, April 10th, 2023 at 19:50, Melissa Wen <mwen@igalia.com> wrote: > On 04/10, Simon Ser wrote: > > > I think this should be a KMS property instead of a module parameter. > > Is there a reason why this patch uses a module parameter? It breaks > > user-space expectations. > > a KMS property is what we have on vkms TODO [1] and the module parameter > was Maíra's first step to open a discussion for this property [2]. > AFAIK, we would need to create the KMS property first, but it seems > there isn't an userspace case/need to support this API change. > Do you know any valid use cases to support a bkg color property? There have been previous attempts for msm [1] and i915 [2]. From user-space PoV, a KMS property would be useful, for instance to render single color background images. I can type some user-space code if that helps. If this is a module parameter instead of a KMS property, what purpose does this achieve? What is the use-case? Just trying to understand the motivation here. Thanks, Simon [1]: https://lore.kernel.org/dri-devel/20221028225952.160-1-quic_jesszhan@quicinc.com/ [2]: https://lore.kernel.org/dri-devel/20190930224707.14904-1-matthew.d.roper@intel.com/
On Tue, Apr 11, 2023 at 08:58:16AM +0000, Simon Ser wrote: > Hi, > > On Monday, April 10th, 2023 at 19:50, Melissa Wen <mwen@igalia.com> wrote: > > > On 04/10, Simon Ser wrote: > > > > > I think this should be a KMS property instead of a module parameter. > > > Is there a reason why this patch uses a module parameter? It breaks > > > user-space expectations. > > > > a KMS property is what we have on vkms TODO [1] and the module parameter > > was Maíra's first step to open a discussion for this property [2]. > > AFAIK, we would need to create the KMS property first, but it seems > > there isn't an userspace case/need to support this API change. > > Do you know any valid use cases to support a bkg color property? > > There have been previous attempts for msm [1] and i915 [2]. > > From user-space PoV, a KMS property would be useful, for instance to > render single color background images. I can type some user-space code > if that helps. Yeah the hold-up thus far was that no one ever came up with an actually useful use in a compositor for this. Everyone seems happy with black, but maybe with proper color rendering this is changing :-) > If this is a module parameter instead of a KMS property, what purpose > does this achieve? What is the use-case? Just trying to understand the > motivation here. Just a step to get things going, occasionally that's needed ... -Daniel
On 4/11/23 05:58, Simon Ser wrote: > Hi, > > On Monday, April 10th, 2023 at 19:50, Melissa Wen <mwen@igalia.com> wrote: > >> On 04/10, Simon Ser wrote: >> >>> I think this should be a KMS property instead of a module parameter. >>> Is there a reason why this patch uses a module parameter? It breaks >>> user-space expectations. >> >> a KMS property is what we have on vkms TODO [1] and the module parameter >> was Maíra's first step to open a discussion for this property [2]. >> AFAIK, we would need to create the KMS property first, but it seems >> there isn't an userspace case/need to support this API change. >> Do you know any valid use cases to support a bkg color property? > > There have been previous attempts for msm [1] and i915 [2]. > > From user-space PoV, a KMS property would be useful, for instance to > render single color background images. I can type some user-space code > if that helps. I believe that, if we have some user-space code, we can push this as a KMS property. If you can provide some user-space code, I can work on the KMS property to back the use case and we can move this forward. > > If this is a module parameter instead of a KMS property, what purpose > does this achieve? What is the use-case? Just trying to understand the > motivation here. > This was a attempt to start the discussion about the background color property. Without the user-space use case, we cannot move this forward and as I didn't have a user-space use case, I decided to implement it as a module parameter. Best Regards, - Maíra Canal > Thanks, > > Simon > > [1]: https://lore.kernel.org/dri-devel/20221028225952.160-1-quic_jesszhan@quicinc.com/ > [2]: https://lore.kernel.org/dri-devel/20190930224707.14904-1-matthew.d.roper@intel.com/
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..b0056fad908e 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 >> 16) & 0xff) * 257, + .g = ((*vkms_dev->config->background_color >> 8) & 0xff) * 257, + .b = (*vkms_dev->config->background_color & 0xff) * 257, + }; 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..bb6990882608 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 int background_color; +module_param_named(background_color, background_color, uint, 0644); +MODULE_PARM_DESC(background_color, "Background color (0xRRGGBB)"); + 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%x\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..95e779ef017b 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 int *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)
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 int number as an XRGB8888 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@igalia.com> --- In order to test this functionality, I wrote some IGT tests to ensure that it 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. v1 -> v2 [2] * Use XRGB8888 as input format to avoid compilation errors on PPC (kernel test robot) [1] https://gitlab.freedesktop.org/mairacanal/igt-gpu-tools/-/tree/vkms/background-color [2] https://lore.kernel.org/dri-devel/20230406172002.124456-1-mcanal@igalia.com/T/ 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(-)