* [RFC PATCH 0/3] Support for Solid Fill Planes
@ 2022-10-28 22:59 Jessica Zhang
2022-10-28 22:59 ` [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane Jessica Zhang
` (3 more replies)
0 siblings, 4 replies; 48+ messages in thread
From: Jessica Zhang @ 2022-10-28 22:59 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart, dmitry.baryshkov, Jessica Zhang
Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
properties. When the color fill value is set, and the framebuffer is set
to NULL, memory fetch will be disabled.
In addition, loosen the NULL FB checks within the atomic commit callstack
to allow a NULL FB when color_fill is nonzero and add FB checks in
methods where the FB was previously assumed to be non-NULL.
Finally, have the DPU driver use drm_plane_state.color_fill and
drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
and add extra checks in the DPU atomic commit callstack to account for a
NULL FB in cases where color_fill is set.
Some drivers support hardware that have optimizations for solid fill
planes. This series aims to expose these capabilities to userspace as
some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
hardware composer HAL) that can be set by apps like the Android Gears
app.
Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
DRM format, setting COLOR_FILL to a color fill value, and setting the
framebuffer to NULL.
Jessica Zhang (3):
drm: Introduce color fill properties for drm plane
drm: Adjust atomic checks for solid fill color
drm/msm/dpu: Use color_fill property for DPU planes
drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++-----------
drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
drivers/gpu/drm/drm_plane.c | 8 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
include/drm/drm_atomic_helper.h | 5 +-
include/drm/drm_blend.h | 2 +
include/drm/drm_plane.h | 28 ++++++++++
10 files changed, 188 insertions(+), 76 deletions(-)
--
2.38.0
^ permalink raw reply [flat|nested] 48+ messages in thread
* [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-10-28 22:59 [RFC PATCH 0/3] Support for Solid Fill Planes Jessica Zhang
@ 2022-10-28 22:59 ` Jessica Zhang
2022-10-29 11:23 ` Dmitry Baryshkov
2022-10-29 12:08 ` Dmitry Baryshkov
2022-10-28 22:59 ` [RFC PATCH 2/3] drm: Adjust atomic checks for solid fill color Jessica Zhang
` (2 subsequent siblings)
3 siblings, 2 replies; 48+ messages in thread
From: Jessica Zhang @ 2022-10-28 22:59 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart, dmitry.baryshkov, Jessica Zhang
Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
drm_plane. In addition, add support for setting and getting the values
of these properties.
COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
represents the format of the color fill. Userspace can set enable solid
fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
framebuffer to NULL.
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/drm_atomic_uapi.c | 8 +++++++
drivers/gpu/drm/drm_blend.c | 38 +++++++++++++++++++++++++++++++
include/drm/drm_blend.h | 2 ++
include/drm/drm_plane.h | 28 +++++++++++++++++++++++
4 files changed, 76 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 79730fa1dd8e..e1664463fca4 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -544,6 +544,10 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
state->src_w = val;
} else if (property == config->prop_src_h) {
state->src_h = val;
+ } else if (property == plane->color_fill_format_property) {
+ state->color_fill_format = val;
+ } else if (property == plane->color_fill_property) {
+ state->color_fill = val;
} else if (property == plane->alpha_property) {
state->alpha = val;
} else if (property == plane->blend_mode_property) {
@@ -616,6 +620,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->src_w;
} else if (property == config->prop_src_h) {
*val = state->src_h;
+ } else if (property == plane->color_fill_format_property) {
+ *val = state->color_fill_format;
+ } else if (property == plane->color_fill_property) {
+ *val = state->color_fill;
} else if (property == plane->alpha_property) {
*val = state->alpha;
} else if (property == plane->blend_mode_property) {
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index b4c8cab7158c..b8c2b263fa51 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -616,3 +616,41 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
return 0;
}
EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
+
+int drm_plane_create_color_fill_property(struct drm_plane *plane)
+{
+ struct drm_property *prop;
+
+ prop = drm_property_create_range(plane->dev, 0, "color_fill",
+ 0, 0xffffffff);
+ if (!prop)
+ return -ENOMEM;
+
+ drm_object_attach_property(&plane->base, prop, 0);
+ plane->color_fill_property = prop;
+
+ if (plane->state)
+ plane->state->color_fill = 0;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_color_fill_property);
+
+int drm_plane_create_color_fill_format_property(struct drm_plane *plane)
+{
+ struct drm_property *prop;
+
+ prop = drm_property_create_range(plane->dev, 0, "color_fill_format",
+ 0, 0xffffffff);
+ if (!prop)
+ return -ENOMEM;
+
+ drm_object_attach_property(&plane->base, prop, 0);
+ plane->color_fill_format_property = prop;
+
+ if (plane->state)
+ plane->state->color_fill_format = 0;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_color_fill_format_property);
diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
index 88bdfec3bd88..3e96f5e83cce 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -58,4 +58,6 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
struct drm_atomic_state *state);
int drm_plane_create_blend_mode_property(struct drm_plane *plane,
unsigned int supported_modes);
+int drm_plane_create_color_fill_property(struct drm_plane *plane);
+int drm_plane_create_color_fill_format_property(struct drm_plane *plane);
#endif
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 89ea54652e87..dcbfdb0e1f71 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -116,6 +116,20 @@ struct drm_plane_state {
/** @src_h: height of visible portion of plane (in 16.16) */
uint32_t src_h, src_w;
+ /**
+ * @color_fill_format:
+ * Format of the color fill value.
+ */
+ uint32_t color_fill_format;
+
+ /**
+ * @color_fill:
+ * Fill color of the plane with 0 as black and 0xffffffff as white.
+ * Can be set by user by setting the COLOR_FILL property. See
+ * drm_plane_create_color_fill_property() for more details.
+ */
+ uint32_t color_fill;
+
/**
* @alpha:
* Opacity of the plane with 0 as completely transparent and 0xffff as
@@ -699,6 +713,20 @@ struct drm_plane {
*/
struct drm_plane_state *state;
+ /*
+ * @color_fill_format_property:
+ * Optional color fill format property for this plane. See
+ * drm_plane_create_color_fill_format_property().
+ */
+ struct drm_property *color_fill_format_property;
+
+ /*
+ * @color_fill_property:
+ * Optional color fill property for this plane. See
+ * drm_plane_create_color_fill_property().
+ */
+ struct drm_property *color_fill_property;
+
/**
* @alpha_property:
* Optional alpha property for this plane. See
--
2.38.0
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [RFC PATCH 2/3] drm: Adjust atomic checks for solid fill color
2022-10-28 22:59 [RFC PATCH 0/3] Support for Solid Fill Planes Jessica Zhang
2022-10-28 22:59 ` [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane Jessica Zhang
@ 2022-10-28 22:59 ` Jessica Zhang
2022-10-29 11:38 ` Dmitry Baryshkov
2022-10-28 22:59 ` [RFC PATCH 3/3] drm/msm/dpu: Use color_fill property for DPU planes Jessica Zhang
2022-11-07 19:37 ` [RFC PATCH 0/3] Support for Solid Fill Planes Ville Syrjälä
3 siblings, 1 reply; 48+ messages in thread
From: Jessica Zhang @ 2022-10-28 22:59 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart, dmitry.baryshkov, Jessica Zhang
Loosen the requirements for atomic and legacy commit so that, in cases
where solid fill planes is enabled (and FB_ID is NULL), the commit can
still go through.
In addition, add framebuffer NULL checks in other areas to account for
FB being NULL when solid fill is enabled.
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++++++-------------
drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++++------
drivers/gpu/drm/drm_plane.c | 8 ++--
include/drm/drm_atomic_helper.h | 5 ++-
4 files changed, 64 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f197f59f6d99..b576ed221431 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -601,8 +601,10 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
uint32_t num_clips;
int ret;
- /* either *both* CRTC and FB must be set, or neither */
- if (crtc && !fb) {
+ /* When color_fill is disabled,
+ * either *both* CRTC and FB must be set, or neither
+ */
+ if (crtc && !fb && !new_plane_state->color_fill) {
drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n",
plane->base.id, plane->name);
return -EINVAL;
@@ -626,14 +628,16 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
}
/* Check whether this plane supports the fb pixel format. */
- ret = drm_plane_check_pixel_format(plane, fb->format->format,
- fb->modifier);
- if (ret) {
- drm_dbg_atomic(plane->dev,
- "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n",
- plane->base.id, plane->name,
- &fb->format->format, fb->modifier);
- return ret;
+ if (fb) {
+ ret = drm_plane_check_pixel_format(plane, fb->format->format,
+ fb->modifier);
+
+ if (ret)
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n",
+ plane->base.id, plane->name,
+ &fb->format->format, fb->modifier);
+ return ret;
}
/* Give drivers some help against integer overflows */
@@ -649,28 +653,30 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
return -ERANGE;
}
- fb_width = fb->width << 16;
- fb_height = fb->height << 16;
+ if (fb) {
+ fb_width = fb->width << 16;
+ fb_height = fb->height << 16;
- /* Make sure source coordinates are inside the fb. */
- if (new_plane_state->src_w > fb_width ||
- new_plane_state->src_x > fb_width - new_plane_state->src_w ||
- new_plane_state->src_h > fb_height ||
- new_plane_state->src_y > fb_height - new_plane_state->src_h) {
- drm_dbg_atomic(plane->dev,
- "[PLANE:%d:%s] invalid source coordinates "
- "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
- plane->base.id, plane->name,
- new_plane_state->src_w >> 16,
- ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
- new_plane_state->src_h >> 16,
- ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
- new_plane_state->src_x >> 16,
- ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
- new_plane_state->src_y >> 16,
- ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
- fb->width, fb->height);
- return -ENOSPC;
+ /* Make sure source coordinates are inside the fb. */
+ if (new_plane_state->src_w > fb_width ||
+ new_plane_state->src_x > fb_width - new_plane_state->src_w ||
+ new_plane_state->src_h > fb_height ||
+ new_plane_state->src_y > fb_height - new_plane_state->src_h) {
+ drm_dbg_atomic(plane->dev,
+ "[PLANE:%d:%s] invalid source coordinates "
+ "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
+ plane->base.id, plane->name,
+ new_plane_state->src_w >> 16,
+ ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
+ new_plane_state->src_h >> 16,
+ ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
+ new_plane_state->src_x >> 16,
+ ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
+ new_plane_state->src_y >> 16,
+ ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
+ fb->width, fb->height);
+ return -ENOSPC;
+ }
}
clips = __drm_plane_get_damage_clips(new_plane_state);
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8bf41aa24068..5a5ffa06b8bd 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -818,7 +818,7 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
*src = drm_plane_state_src(plane_state);
*dst = drm_plane_state_dest(plane_state);
- if (!fb) {
+ if (!fb && !plane_state->color_fill) {
plane_state->visible = false;
return 0;
}
@@ -835,25 +835,31 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
return -EINVAL;
}
- drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
+ if (fb) {
+ drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
- /* Check scaling */
- hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
- vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
- if (hscale < 0 || vscale < 0) {
- drm_dbg_kms(plane_state->plane->dev,
- "Invalid scaling of plane\n");
- drm_rect_debug_print("src: ", &plane_state->src, true);
- drm_rect_debug_print("dst: ", &plane_state->dst, false);
- return -ERANGE;
+ /* Check scaling */
+ hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
+ vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
+
+ if (hscale < 0 || vscale < 0) {
+ drm_dbg_kms(plane_state->plane->dev,
+ "Invalid scaling of plane\n");
+ drm_rect_debug_print("src: ", &plane_state->src, true);
+ drm_rect_debug_print("dst: ", &plane_state->dst, false);
+ return -ERANGE;
+ }
}
if (crtc_state->enable)
drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2);
- plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
-
- drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
+ if (fb) {
+ plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
+ drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
+ } else if (plane_state->color_fill) {
+ plane_state->visible = true;
+ }
if (!plane_state->visible)
/*
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 726f2f163c26..223b5ed93d3a 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -850,8 +850,8 @@ static int __setplane_internal(struct drm_plane *plane,
WARN_ON(drm_drv_uses_atomic_modeset(plane->dev));
- /* No fb means shut it down */
- if (!fb) {
+ /* No fb and no color fill means shut it down */
+ if (!fb && (plane->state && !plane->state->color_fill)) {
plane->old_fb = plane->fb;
ret = plane->funcs->disable_plane(plane, ctx);
if (!ret) {
@@ -902,8 +902,8 @@ static int __setplane_atomic(struct drm_plane *plane,
WARN_ON(!drm_drv_uses_atomic_modeset(plane->dev));
- /* No fb means shut it down */
- if (!fb)
+ /* No fb and no color fill means shut it down */
+ if (!fb && (plane->state && !plane->state->color_fill))
return plane->funcs->disable_plane(plane, ctx);
/*
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 2a0b17842402..aa7576f0879d 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -219,8 +219,9 @@ drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state,
* Anything else should be considered a bug in the atomic core, so we
* gently warn about it.
*/
- WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) ||
- (new_plane_state->crtc != NULL && new_plane_state->fb == NULL));
+ WARN_ON(!new_plane_state->color_fill &&
+ ((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) ||
+ (new_plane_state->crtc != NULL && new_plane_state->fb == NULL)));
return old_plane_state->crtc && !new_plane_state->crtc;
}
--
2.38.0
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [RFC PATCH 3/3] drm/msm/dpu: Use color_fill property for DPU planes
2022-10-28 22:59 [RFC PATCH 0/3] Support for Solid Fill Planes Jessica Zhang
2022-10-28 22:59 ` [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane Jessica Zhang
2022-10-28 22:59 ` [RFC PATCH 2/3] drm: Adjust atomic checks for solid fill color Jessica Zhang
@ 2022-10-28 22:59 ` Jessica Zhang
2022-10-29 11:40 ` Dmitry Baryshkov
2022-11-07 19:37 ` [RFC PATCH 0/3] Support for Solid Fill Planes Ville Syrjälä
3 siblings, 1 reply; 48+ messages in thread
From: Jessica Zhang @ 2022-10-28 22:59 UTC (permalink / raw)
To: freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart, dmitry.baryshkov, Jessica Zhang
Initialize and use the color_fill properties for planes in DPU driver. In
addition, relax framebuffer requirements within atomic commit path and
add checks for NULL framebuffers.
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++---------
2 files changed, 48 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 13ce321283ff..157698b4f234 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -441,7 +441,12 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
sspp_idx - SSPP_VIG0,
state->fb ? state->fb->base.id : -1);
- format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
+ if (pstate->base.fb)
+ format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
+ else if (state->color_fill && !state->color_fill_format)
+ format = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
+ else
+ format = dpu_get_dpu_format(state->color_fill_format);
if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
bg_alpha_enable = true;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 658005f609f4..f3be37e97b64 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -103,7 +103,6 @@ struct dpu_plane {
enum dpu_sspp pipe;
struct dpu_hw_pipe *pipe_hw;
- uint32_t color_fill;
bool is_error;
bool is_rt_pipe;
const struct dpu_mdss_cfg *catalog;
@@ -697,7 +696,10 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
* select fill format to match user property expectation,
* h/w only supports RGB variants
*/
- fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
+ if (plane->state->color_fill && !plane->state->color_fill_format)
+ fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
+ else
+ fmt = dpu_get_dpu_format(plane->state->color_fill_format);
/* update sspp */
if (fmt && pdpu->pipe_hw->ops.setup_solidfill) {
@@ -720,6 +722,10 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
fmt, DPU_SSPP_SOLID_FILL,
pstate->multirect_index);
+ /* skip remaining processing on color fill */
+ if (!plane->state->fb)
+ return 0;
+
if (pdpu->pipe_hw->ops.setup_rects)
pdpu->pipe_hw->ops.setup_rects(pdpu->pipe_hw,
&pipe_cfg,
@@ -999,12 +1005,21 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
dst = drm_plane_state_dest(new_plane_state);
- fb_rect.x2 = new_plane_state->fb->width;
- fb_rect.y2 = new_plane_state->fb->height;
+ if (new_plane_state->fb) {
+ fb_rect.x2 = new_plane_state->fb->width;
+ fb_rect.y2 = new_plane_state->fb->height;
+ }
max_linewidth = pdpu->catalog->caps->max_linewidth;
- fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
+ if (new_plane_state->fb) {
+ fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
+ } else if (new_plane_state->color_fill) {
+ if (new_plane_state->color_fill_format)
+ fmt = dpu_get_dpu_format(new_plane_state->color_fill_format);
+ else
+ fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
+ }
min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
@@ -1016,7 +1031,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
return -EINVAL;
/* check src bounds */
- } else if (!dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
+ } else if (new_plane_state->fb && !dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
DRM_RECT_ARG(&src));
return -E2BIG;
@@ -1084,9 +1099,9 @@ void dpu_plane_flush(struct drm_plane *plane)
if (pdpu->is_error)
/* force white frame with 100% alpha pipe output on error */
_dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
- else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
+ else if (!(plane->state->fb) && plane->state->color_fill)
/* force 100% alpha */
- _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
+ _dpu_plane_color_fill(pdpu, plane->state->color_fill, 0xFF);
else if (pdpu->pipe_hw && pdpu->pipe_hw->ops.setup_csc) {
const struct dpu_format *fmt = to_dpu_format(msm_framebuffer_format(plane->state->fb));
const struct dpu_csc_cfg *csc_ptr = _dpu_plane_get_csc(pdpu, fmt);
@@ -1125,23 +1140,30 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
struct drm_crtc *crtc = state->crtc;
struct drm_framebuffer *fb = state->fb;
bool is_rt_pipe, update_qos_remap;
- const struct dpu_format *fmt =
- to_dpu_format(msm_framebuffer_format(fb));
+ const struct dpu_format *fmt;
struct dpu_hw_pipe_cfg pipe_cfg;
- memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
-
- _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
-
pstate->pending = true;
is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
_dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
- DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
- ", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),
- crtc->base.id, DRM_RECT_ARG(&state->dst),
- (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));
+ /* override for color fill */
+ if (!fb && plane->state->color_fill) {
+ /* skip remaining processing on color fill */
+ return;
+ }
+
+ memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
+
+ fmt = to_dpu_format(msm_framebuffer_format(fb));
+ _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
+
+ if (fb)
+ DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
+ ", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),
+ crtc->base.id, DRM_RECT_ARG(&state->dst),
+ (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));
pipe_cfg.src_rect = state->src;
@@ -1153,12 +1175,6 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
pipe_cfg.dst_rect = state->dst;
- /* override for color fill */
- if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
- /* skip remaining processing on color fill */
- return;
- }
-
if (pdpu->pipe_hw->ops.setup_rects) {
pdpu->pipe_hw->ops.setup_rects(pdpu->pipe_hw,
&pipe_cfg,
@@ -1509,6 +1525,8 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
drm_plane_create_alpha_property(plane);
+ drm_plane_create_color_fill_property(plane);
+ drm_plane_create_color_fill_format_property(plane);
drm_plane_create_blend_mode_property(plane,
BIT(DRM_MODE_BLEND_PIXEL_NONE) |
BIT(DRM_MODE_BLEND_PREMULTI) |
--
2.38.0
^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-10-28 22:59 ` [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane Jessica Zhang
@ 2022-10-29 11:23 ` Dmitry Baryshkov
2022-10-31 21:58 ` Jessica Zhang
2022-11-08 18:25 ` Simon Ser
2022-10-29 12:08 ` Dmitry Baryshkov
1 sibling, 2 replies; 48+ messages in thread
From: Dmitry Baryshkov @ 2022-10-29 11:23 UTC (permalink / raw)
To: Jessica Zhang, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart
On 29/10/2022 01:59, Jessica Zhang wrote:
> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> drm_plane. In addition, add support for setting and getting the values
> of these properties.
>
> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> represents the format of the color fill. Userspace can set enable solid
> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> framebuffer to NULL.
I suppose that COLOR_FILL should override framebuffer rather than
requiring that FB is set to NULL. In other words, if color_filL_format
is non-zero, it would make sense to ignore the FB. Then one can use the
color_fill_format property to quickly switch between filled plane and
FB-backed one.
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++++++
> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++++++++++++++++++++
> include/drm/drm_blend.h | 2 ++
> include/drm/drm_plane.h | 28 +++++++++++++++++++++++
> 4 files changed, 76 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 79730fa1dd8e..e1664463fca4 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -544,6 +544,10 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> state->src_w = val;
> } else if (property == config->prop_src_h) {
> state->src_h = val;
> + } else if (property == plane->color_fill_format_property) {
> + state->color_fill_format = val;
> + } else if (property == plane->color_fill_property) {
> + state->color_fill = val;
> } else if (property == plane->alpha_property) {
> state->alpha = val;
> } else if (property == plane->blend_mode_property) {
> @@ -616,6 +620,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> *val = state->src_w;
> } else if (property == config->prop_src_h) {
> *val = state->src_h;
> + } else if (property == plane->color_fill_format_property) {
> + *val = state->color_fill_format;
> + } else if (property == plane->color_fill_property) {
> + *val = state->color_fill;
> } else if (property == plane->alpha_property) {
> *val = state->alpha;
> } else if (property == plane->blend_mode_property) {
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index b4c8cab7158c..b8c2b263fa51 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -616,3 +616,41 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
> return 0;
> }
> EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
> +
> +int drm_plane_create_color_fill_property(struct drm_plane *plane)
> +{
> + struct drm_property *prop;
> +
> + prop = drm_property_create_range(plane->dev, 0, "color_fill",
> + 0, 0xffffffff);
> + if (!prop)
> + return -ENOMEM;
> +
> + drm_object_attach_property(&plane->base, prop, 0);
> + plane->color_fill_property = prop;
> +
> + if (plane->state)
> + plane->state->color_fill = 0;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_color_fill_property);
> +
> +int drm_plane_create_color_fill_format_property(struct drm_plane *plane)
> +{
> + struct drm_property *prop;
> +
> + prop = drm_property_create_range(plane->dev, 0, "color_fill_format",
> + 0, 0xffffffff);
> + if (!prop)
> + return -ENOMEM;
> +
> + drm_object_attach_property(&plane->base, prop, 0);
> + plane->color_fill_format_property = prop;
> +
> + if (plane->state)
> + plane->state->color_fill_format = 0;
Don't you also need to reset these properties in
__drm_atomic_helper_plane_state_reset() ?
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_color_fill_format_property);
> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
> index 88bdfec3bd88..3e96f5e83cce 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -58,4 +58,6 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
> struct drm_atomic_state *state);
> int drm_plane_create_blend_mode_property(struct drm_plane *plane,
> unsigned int supported_modes);
> +int drm_plane_create_color_fill_property(struct drm_plane *plane);
> +int drm_plane_create_color_fill_format_property(struct drm_plane *plane);
> #endif
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 89ea54652e87..dcbfdb0e1f71 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -116,6 +116,20 @@ struct drm_plane_state {
> /** @src_h: height of visible portion of plane (in 16.16) */
> uint32_t src_h, src_w;
>
> + /**
> + * @color_fill_format:
> + * Format of the color fill value.
> + */
> + uint32_t color_fill_format;
> +
> + /**
> + * @color_fill:
> + * Fill color of the plane with 0 as black and 0xffffffff as white.
> + * Can be set by user by setting the COLOR_FILL property. See
> + * drm_plane_create_color_fill_property() for more details.
> + */
> + uint32_t color_fill;
> +
> /**
> * @alpha:
> * Opacity of the plane with 0 as completely transparent and 0xffff as
> @@ -699,6 +713,20 @@ struct drm_plane {
> */
> struct drm_plane_state *state;
>
> + /*
> + * @color_fill_format_property:
> + * Optional color fill format property for this plane. See
> + * drm_plane_create_color_fill_format_property().
> + */
> + struct drm_property *color_fill_format_property;
> +
> + /*
> + * @color_fill_property:
> + * Optional color fill property for this plane. See
> + * drm_plane_create_color_fill_property().
> + */
> + struct drm_property *color_fill_property;
> +
> /**
> * @alpha_property:
> * Optional alpha property for this plane. See
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 2/3] drm: Adjust atomic checks for solid fill color
2022-10-28 22:59 ` [RFC PATCH 2/3] drm: Adjust atomic checks for solid fill color Jessica Zhang
@ 2022-10-29 11:38 ` Dmitry Baryshkov
2022-10-31 20:41 ` Jessica Zhang
0 siblings, 1 reply; 48+ messages in thread
From: Dmitry Baryshkov @ 2022-10-29 11:38 UTC (permalink / raw)
To: Jessica Zhang, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart
On 29/10/2022 01:59, Jessica Zhang wrote:
> Loosen the requirements for atomic and legacy commit so that, in cases
> where solid fill planes is enabled (and FB_ID is NULL), the commit can
> still go through.
>
> In addition, add framebuffer NULL checks in other areas to account for
> FB being NULL when solid fill is enabled.
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++++++-------------
> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++++------
> drivers/gpu/drm/drm_plane.c | 8 ++--
> include/drm/drm_atomic_helper.h | 5 ++-
> 4 files changed, 64 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f197f59f6d99..b576ed221431 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -601,8 +601,10 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
> uint32_t num_clips;
> int ret;
>
> - /* either *both* CRTC and FB must be set, or neither */
> - if (crtc && !fb) {
> + /* When color_fill is disabled,
> + * either *both* CRTC and FB must be set, or neither
> + */
> + if (crtc && !fb && !new_plane_state->color_fill) {
Using !color_fill sounds bad. It would disallow using black as the plane
fill color. I think, you need to check color_fill_format instead.
> drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n",
> plane->base.id, plane->name);
> return -EINVAL;
> @@ -626,14 +628,16 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
> }
Don't we need to check that the color_fill_format is supported too?
>
> /* Check whether this plane supports the fb pixel format. */
> - ret = drm_plane_check_pixel_format(plane, fb->format->format,
> - fb->modifier);
> - if (ret) {
> - drm_dbg_atomic(plane->dev,
> - "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n",
> - plane->base.id, plane->name,
> - &fb->format->format, fb->modifier);
> - return ret;
> + if (fb) {
> + ret = drm_plane_check_pixel_format(plane, fb->format->format,
> + fb->modifier);
> +
> + if (ret)
> + drm_dbg_atomic(plane->dev,
> + "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n",
> + plane->base.id, plane->name,
> + &fb->format->format, fb->modifier);
> + return ret;
> }
>
> /* Give drivers some help against integer overflows */
> @@ -649,28 +653,30 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
> return -ERANGE;
> }
>
> - fb_width = fb->width << 16;
> - fb_height = fb->height << 16;
> + if (fb) {
> + fb_width = fb->width << 16;
> + fb_height = fb->height << 16;
>
> - /* Make sure source coordinates are inside the fb. */
> - if (new_plane_state->src_w > fb_width ||
> - new_plane_state->src_x > fb_width - new_plane_state->src_w ||
> - new_plane_state->src_h > fb_height ||
> - new_plane_state->src_y > fb_height - new_plane_state->src_h) {
> - drm_dbg_atomic(plane->dev,
> - "[PLANE:%d:%s] invalid source coordinates "
> - "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
> - plane->base.id, plane->name,
> - new_plane_state->src_w >> 16,
> - ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
> - new_plane_state->src_h >> 16,
> - ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
> - new_plane_state->src_x >> 16,
> - ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
> - new_plane_state->src_y >> 16,
> - ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
> - fb->width, fb->height);
> - return -ENOSPC;
> + /* Make sure source coordinates are inside the fb. */
> + if (new_plane_state->src_w > fb_width ||
> + new_plane_state->src_x > fb_width - new_plane_state->src_w ||
> + new_plane_state->src_h > fb_height ||
> + new_plane_state->src_y > fb_height - new_plane_state->src_h) {
> + drm_dbg_atomic(plane->dev,
> + "[PLANE:%d:%s] invalid source coordinates "
> + "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
> + plane->base.id, plane->name,
> + new_plane_state->src_w >> 16,
> + ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
> + new_plane_state->src_h >> 16,
> + ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
> + new_plane_state->src_x >> 16,
> + ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
> + new_plane_state->src_y >> 16,
> + ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
> + fb->width, fb->height);
> + return -ENOSPC;
> + }
> }
>
> clips = __drm_plane_get_damage_clips(new_plane_state);
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 8bf41aa24068..5a5ffa06b8bd 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -818,7 +818,7 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> *src = drm_plane_state_src(plane_state);
> *dst = drm_plane_state_dest(plane_state);
>
> - if (!fb) {
> + if (!fb && !plane_state->color_fill) {
> plane_state->visible = false;
> return 0;
> }
> @@ -835,25 +835,31 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> return -EINVAL;
> }
>
> - drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
> + if (fb) {
> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
>
> - /* Check scaling */
> - hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
> - vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
> - if (hscale < 0 || vscale < 0) {
> - drm_dbg_kms(plane_state->plane->dev,
> - "Invalid scaling of plane\n");
> - drm_rect_debug_print("src: ", &plane_state->src, true);
> - drm_rect_debug_print("dst: ", &plane_state->dst, false);
> - return -ERANGE;
> + /* Check scaling */
> + hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
> + vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
> +
> + if (hscale < 0 || vscale < 0) {
> + drm_dbg_kms(plane_state->plane->dev,
> + "Invalid scaling of plane\n");
> + drm_rect_debug_print("src: ", &plane_state->src, true);
> + drm_rect_debug_print("dst: ", &plane_state->dst, false);
> + return -ERANGE;
> + }
> }
>
> if (crtc_state->enable)
> drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2);
>
> - plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
> -
> - drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
> + if (fb) {
> + plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
> + drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation);
> + } else if (plane_state->color_fill) {
> + plane_state->visible = true;
> + }
>
> if (!plane_state->visible)
> /*
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 726f2f163c26..223b5ed93d3a 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -850,8 +850,8 @@ static int __setplane_internal(struct drm_plane *plane,
>
> WARN_ON(drm_drv_uses_atomic_modeset(plane->dev));
>
> - /* No fb means shut it down */
> - if (!fb) {
> + /* No fb and no color fill means shut it down */
> + if (!fb && (plane->state && !plane->state->color_fill)) {
I'd suggest abstracting this to a helper function, which can then be
used in drm_atomic_plane_check() too.
> plane->old_fb = plane->fb;
> ret = plane->funcs->disable_plane(plane, ctx);
> if (!ret) {
> @@ -902,8 +902,8 @@ static int __setplane_atomic(struct drm_plane *plane,
>
> WARN_ON(!drm_drv_uses_atomic_modeset(plane->dev));
>
> - /* No fb means shut it down */
> - if (!fb)
> + /* No fb and no color fill means shut it down */
> + if (!fb && (plane->state && !plane->state->color_fill))
> return plane->funcs->disable_plane(plane, ctx);
>
> /*
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 2a0b17842402..aa7576f0879d 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -219,8 +219,9 @@ drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state,
> * Anything else should be considered a bug in the atomic core, so we
> * gently warn about it.
> */
> - WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) ||
> - (new_plane_state->crtc != NULL && new_plane_state->fb == NULL));
> + WARN_ON(!new_plane_state->color_fill &&
> + ((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) ||
> + (new_plane_state->crtc != NULL && new_plane_state->fb == NULL)));
Please fix the indentation.
>
> return old_plane_state->crtc && !new_plane_state->crtc;
> }
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 3/3] drm/msm/dpu: Use color_fill property for DPU planes
2022-10-28 22:59 ` [RFC PATCH 3/3] drm/msm/dpu: Use color_fill property for DPU planes Jessica Zhang
@ 2022-10-29 11:40 ` Dmitry Baryshkov
2022-10-31 22:14 ` Jessica Zhang
0 siblings, 1 reply; 48+ messages in thread
From: Dmitry Baryshkov @ 2022-10-29 11:40 UTC (permalink / raw)
To: Jessica Zhang, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart
On 29/10/2022 01:59, Jessica Zhang wrote:
> Initialize and use the color_fill properties for planes in DPU driver. In
> addition, relax framebuffer requirements within atomic commit path and
> add checks for NULL framebuffers.
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++---------
> 2 files changed, 48 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 13ce321283ff..157698b4f234 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -441,7 +441,12 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
> sspp_idx - SSPP_VIG0,
> state->fb ? state->fb->base.id : -1);
>
> - format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
> + if (pstate->base.fb)
> + format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
> + else if (state->color_fill && !state->color_fill_format)
> + format = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
As I wrote in the review of the earlier patch, this disallows using
black as the plane fill colour. Not to mention that using ABGR should be
explicit rather than implicit.
> + else
> + format = dpu_get_dpu_format(state->color_fill_format);
>
> if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
> bg_alpha_enable = true;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 658005f609f4..f3be37e97b64 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -103,7 +103,6 @@ struct dpu_plane {
> enum dpu_sspp pipe;
>
> struct dpu_hw_pipe *pipe_hw;
> - uint32_t color_fill;
> bool is_error;
> bool is_rt_pipe;
> const struct dpu_mdss_cfg *catalog;
> @@ -697,7 +696,10 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
> * select fill format to match user property expectation,
> * h/w only supports RGB variants
> */
> - fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
> + if (plane->state->color_fill && !plane->state->color_fill_format)
> + fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
> + else
> + fmt = dpu_get_dpu_format(plane->state->color_fill_format);
>
> /* update sspp */
> if (fmt && pdpu->pipe_hw->ops.setup_solidfill) {
> @@ -720,6 +722,10 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
> fmt, DPU_SSPP_SOLID_FILL,
> pstate->multirect_index);
>
> + /* skip remaining processing on color fill */
> + if (!plane->state->fb)
> + return 0;
> +
> if (pdpu->pipe_hw->ops.setup_rects)
> pdpu->pipe_hw->ops.setup_rects(pdpu->pipe_hw,
> &pipe_cfg,
> @@ -999,12 +1005,21 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>
> dst = drm_plane_state_dest(new_plane_state);
>
> - fb_rect.x2 = new_plane_state->fb->width;
> - fb_rect.y2 = new_plane_state->fb->height;
> + if (new_plane_state->fb) {
> + fb_rect.x2 = new_plane_state->fb->width;
> + fb_rect.y2 = new_plane_state->fb->height;
> + }
>
> max_linewidth = pdpu->catalog->caps->max_linewidth;
>
> - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> + if (new_plane_state->fb) {
> + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> + } else if (new_plane_state->color_fill) {
> + if (new_plane_state->color_fill_format)
> + fmt = dpu_get_dpu_format(new_plane_state->color_fill_format);
> + else
> + fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
> + }
>
> min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>
> @@ -1016,7 +1031,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> return -EINVAL;
>
> /* check src bounds */
> - } else if (!dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
> + } else if (new_plane_state->fb && !dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
> DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
> DRM_RECT_ARG(&src));
> return -E2BIG;
> @@ -1084,9 +1099,9 @@ void dpu_plane_flush(struct drm_plane *plane)
> if (pdpu->is_error)
> /* force white frame with 100% alpha pipe output on error */
> _dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
> - else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
> + else if (!(plane->state->fb) && plane->state->color_fill)
> /* force 100% alpha */
> - _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
> + _dpu_plane_color_fill(pdpu, plane->state->color_fill, 0xFF);
> else if (pdpu->pipe_hw && pdpu->pipe_hw->ops.setup_csc) {
> const struct dpu_format *fmt = to_dpu_format(msm_framebuffer_format(plane->state->fb));
> const struct dpu_csc_cfg *csc_ptr = _dpu_plane_get_csc(pdpu, fmt);
> @@ -1125,23 +1140,30 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
> struct drm_crtc *crtc = state->crtc;
> struct drm_framebuffer *fb = state->fb;
> bool is_rt_pipe, update_qos_remap;
> - const struct dpu_format *fmt =
> - to_dpu_format(msm_framebuffer_format(fb));
> + const struct dpu_format *fmt;
> struct dpu_hw_pipe_cfg pipe_cfg;
>
> - memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
> -
> - _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
> -
> pstate->pending = true;
>
> is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
> _dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
>
> - DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
> - ", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),
> - crtc->base.id, DRM_RECT_ARG(&state->dst),
> - (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));
> + /* override for color fill */
> + if (!fb && plane->state->color_fill) {
> + /* skip remaining processing on color fill */
> + return;
> + }
> +
> + memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
> +
> + fmt = to_dpu_format(msm_framebuffer_format(fb));
> + _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
> +
> + if (fb)
> + DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
> + ", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),
> + crtc->base.id, DRM_RECT_ARG(&state->dst),
> + (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));
>
> pipe_cfg.src_rect = state->src;
>
> @@ -1153,12 +1175,6 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>
> pipe_cfg.dst_rect = state->dst;
>
> - /* override for color fill */
> - if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
> - /* skip remaining processing on color fill */
> - return;
> - }
> -
> if (pdpu->pipe_hw->ops.setup_rects) {
> pdpu->pipe_hw->ops.setup_rects(pdpu->pipe_hw,
> &pipe_cfg,
> @@ -1509,6 +1525,8 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
> DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
>
> drm_plane_create_alpha_property(plane);
> + drm_plane_create_color_fill_property(plane);
> + drm_plane_create_color_fill_format_property(plane);
> drm_plane_create_blend_mode_property(plane,
> BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> BIT(DRM_MODE_BLEND_PREMULTI) |
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-10-28 22:59 ` [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane Jessica Zhang
2022-10-29 11:23 ` Dmitry Baryshkov
@ 2022-10-29 12:08 ` Dmitry Baryshkov
2022-10-31 22:24 ` Jessica Zhang
2022-11-08 18:50 ` Simon Ser
1 sibling, 2 replies; 48+ messages in thread
From: Dmitry Baryshkov @ 2022-10-29 12:08 UTC (permalink / raw)
To: Jessica Zhang, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart
On 29/10/2022 01:59, Jessica Zhang wrote:
> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> drm_plane. In addition, add support for setting and getting the values
> of these properties.
>
> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> represents the format of the color fill. Userspace can set enable solid
> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> framebuffer to NULL.
>
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
Planes report supported formats using the drm_mode_getplane(). You'd
also need to tell userspace, which formats are supported for color fill.
I don't think one supports e.g. YV12.
A bit of generic comment for the discussion (this is an RFC anyway).
Using color_fill/color_fill_format properties sounds simple, but this
might be not generic enough. Limiting color_fill to 32 bits would
prevent anybody from using floating point formats (e.g.
DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
e.g. using 64-bit for the color_fill value, but then this doesn't sound
extensible too much.
So, a question for other hardware maintainers. Do we have hardware that
supports such 'color filled' planes? Do we want to support format
modifiers for filling color/data? Because what I have in mind is closer
to the blob structure, which can then be used for filling the plane:
struct color_fill_blob {
u32 pixel_format;
u64 modifiers4];
u32 pixel_data_size; // fixme: is this necessary?
u8 pixel_data[];
};
And then... This sounds a lot like a custom framebuffer.
So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
flag to the framebuffers, which would e.g. mean that the FB gets stamped
all over the plane. This would also save us from changing if (!fb)
checks all over the drm core.
Another approach might be using a format modifier instead of the FB flag.
What do you think?
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 2/3] drm: Adjust atomic checks for solid fill color
2022-10-29 11:38 ` Dmitry Baryshkov
@ 2022-10-31 20:41 ` Jessica Zhang
0 siblings, 0 replies; 48+ messages in thread
From: Jessica Zhang @ 2022-10-31 20:41 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart
On 10/29/2022 4:38 AM, Dmitry Baryshkov wrote:
> On 29/10/2022 01:59, Jessica Zhang wrote:
>> Loosen the requirements for atomic and legacy commit so that, in cases
>> where solid fill planes is enabled (and FB_ID is NULL), the commit can
>> still go through.
>>
>> In addition, add framebuffer NULL checks in other areas to account for
>> FB being NULL when solid fill is enabled.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>> drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++++++-------------
>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++++------
>> drivers/gpu/drm/drm_plane.c | 8 ++--
>> include/drm/drm_atomic_helper.h | 5 ++-
>> 4 files changed, 64 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index f197f59f6d99..b576ed221431 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -601,8 +601,10 @@ static int drm_atomic_plane_check(const struct
>> drm_plane_state *old_plane_state,
>> uint32_t num_clips;
>> int ret;
>> - /* either *both* CRTC and FB must be set, or neither */
>> - if (crtc && !fb) {
>> + /* When color_fill is disabled,
>> + * either *both* CRTC and FB must be set, or neither
>> + */
>> + if (crtc && !fb && !new_plane_state->color_fill) {
>
> Using !color_fill sounds bad. It would disallow using black as the plane
> fill color. I think, you need to check color_fill_format instead.
Hey Dmitry,
Good point -- acked.
>
>> drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no
>> FB\n",
>> plane->base.id, plane->name);
>> return -EINVAL;
>> @@ -626,14 +628,16 @@ static int drm_atomic_plane_check(const struct
>> drm_plane_state *old_plane_state,
>> }
>
>
> Don't we need to check that the color_fill_format is supported too?
Acked.
>
>> /* Check whether this plane supports the fb pixel format. */
>> - ret = drm_plane_check_pixel_format(plane, fb->format->format,
>> - fb->modifier);
>> - if (ret) {
>> - drm_dbg_atomic(plane->dev,
>> - "[PLANE:%d:%s] invalid pixel format %p4cc,
>> modifier 0x%llx\n",
>> - plane->base.id, plane->name,
>> - &fb->format->format, fb->modifier);
>> - return ret;
>> + if (fb) {
>> + ret = drm_plane_check_pixel_format(plane, fb->format->format,
>> + fb->modifier);
>> +
>> + if (ret)
>> + drm_dbg_atomic(plane->dev,
>> + "[PLANE:%d:%s] invalid pixel format %p4cc,
>> modifier 0x%llx\n",
>> + plane->base.id, plane->name,
>> + &fb->format->format, fb->modifier);
>> + return ret;
>> }
>> /* Give drivers some help against integer overflows */
>> @@ -649,28 +653,30 @@ static int drm_atomic_plane_check(const struct
>> drm_plane_state *old_plane_state,
>> return -ERANGE;
>> }
>> - fb_width = fb->width << 16;
>> - fb_height = fb->height << 16;
>> + if (fb) {
>> + fb_width = fb->width << 16;
>> + fb_height = fb->height << 16;
>> - /* Make sure source coordinates are inside the fb. */
>> - if (new_plane_state->src_w > fb_width ||
>> - new_plane_state->src_x > fb_width - new_plane_state->src_w ||
>> - new_plane_state->src_h > fb_height ||
>> - new_plane_state->src_y > fb_height - new_plane_state->src_h) {
>> - drm_dbg_atomic(plane->dev,
>> - "[PLANE:%d:%s] invalid source coordinates "
>> - "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
>> - plane->base.id, plane->name,
>> - new_plane_state->src_w >> 16,
>> - ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
>> - new_plane_state->src_h >> 16,
>> - ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
>> - new_plane_state->src_x >> 16,
>> - ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
>> - new_plane_state->src_y >> 16,
>> - ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
>> - fb->width, fb->height);
>> - return -ENOSPC;
>> + /* Make sure source coordinates are inside the fb. */
>> + if (new_plane_state->src_w > fb_width ||
>> + new_plane_state->src_x > fb_width -
>> new_plane_state->src_w ||
>> + new_plane_state->src_h > fb_height ||
>> + new_plane_state->src_y > fb_height -
>> new_plane_state->src_h) {
>> + drm_dbg_atomic(plane->dev,
>> + "[PLANE:%d:%s] invalid source coordinates "
>> + "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
>> + plane->base.id, plane->name,
>> + new_plane_state->src_w >> 16,
>> + ((new_plane_state->src_w & 0xffff) * 15625) >>
>> 10,
>> + new_plane_state->src_h >> 16,
>> + ((new_plane_state->src_h & 0xffff) * 15625) >>
>> 10,
>> + new_plane_state->src_x >> 16,
>> + ((new_plane_state->src_x & 0xffff) * 15625) >>
>> 10,
>> + new_plane_state->src_y >> 16,
>> + ((new_plane_state->src_y & 0xffff) * 15625) >>
>> 10,
>> + fb->width, fb->height);
>> + return -ENOSPC;
>> + }
>> }
>> clips = __drm_plane_get_damage_clips(new_plane_state);
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index 8bf41aa24068..5a5ffa06b8bd 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -818,7 +818,7 @@ int drm_atomic_helper_check_plane_state(struct
>> drm_plane_state *plane_state,
>> *src = drm_plane_state_src(plane_state);
>> *dst = drm_plane_state_dest(plane_state);
>> - if (!fb) {
>> + if (!fb && !plane_state->color_fill) {
>> plane_state->visible = false;
>> return 0;
>> }
>> @@ -835,25 +835,31 @@ int drm_atomic_helper_check_plane_state(struct
>> drm_plane_state *plane_state,
>> return -EINVAL;
>> }
>> - drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation);
>> + if (fb) {
>> + drm_rect_rotate(src, fb->width << 16, fb->height << 16,
>> rotation);
>> - /* Check scaling */
>> - hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
>> - vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
>> - if (hscale < 0 || vscale < 0) {
>> - drm_dbg_kms(plane_state->plane->dev,
>> - "Invalid scaling of plane\n");
>> - drm_rect_debug_print("src: ", &plane_state->src, true);
>> - drm_rect_debug_print("dst: ", &plane_state->dst, false);
>> - return -ERANGE;
>> + /* Check scaling */
>> + hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
>> + vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale);
>> +
>> + if (hscale < 0 || vscale < 0) {
>> + drm_dbg_kms(plane_state->plane->dev,
>> + "Invalid scaling of plane\n");
>> + drm_rect_debug_print("src: ", &plane_state->src, true);
>> + drm_rect_debug_print("dst: ", &plane_state->dst, false);
>> + return -ERANGE;
>> + }
>> }
>> if (crtc_state->enable)
>> drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2);
>> - plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
>> -
>> - drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
>> rotation);
>> + if (fb) {
>> + plane_state->visible = drm_rect_clip_scaled(src, dst, &clip);
>> + drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
>> rotation);
>> + } else if (plane_state->color_fill) {
>> + plane_state->visible = true;
>> + }
>> if (!plane_state->visible)
>> /*
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index 726f2f163c26..223b5ed93d3a 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -850,8 +850,8 @@ static int __setplane_internal(struct drm_plane
>> *plane,
>> WARN_ON(drm_drv_uses_atomic_modeset(plane->dev));
>> - /* No fb means shut it down */
>> - if (!fb) {
>> + /* No fb and no color fill means shut it down */
>> + if (!fb && (plane->state && !plane->state->color_fill)) {
>
> I'd suggest abstracting this to a helper function, which can then be
> used in drm_atomic_plane_check() too.
Agreed.
>
>> plane->old_fb = plane->fb;
>> ret = plane->funcs->disable_plane(plane, ctx);
>> if (!ret) {
>> @@ -902,8 +902,8 @@ static int __setplane_atomic(struct drm_plane *plane,
>> WARN_ON(!drm_drv_uses_atomic_modeset(plane->dev));
>> - /* No fb means shut it down */
>> - if (!fb)
>> + /* No fb and no color fill means shut it down */
>> + if (!fb && (plane->state && !plane->state->color_fill))
>> return plane->funcs->disable_plane(plane, ctx);
>> /*
>> diff --git a/include/drm/drm_atomic_helper.h
>> b/include/drm/drm_atomic_helper.h
>> index 2a0b17842402..aa7576f0879d 100644
>> --- a/include/drm/drm_atomic_helper.h
>> +++ b/include/drm/drm_atomic_helper.h
>> @@ -219,8 +219,9 @@ drm_atomic_plane_disabling(struct drm_plane_state
>> *old_plane_state,
>> * Anything else should be considered a bug in the atomic core,
>> so we
>> * gently warn about it.
>> */
>> - WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb !=
>> NULL) ||
>> - (new_plane_state->crtc != NULL && new_plane_state->fb == NULL));
>> + WARN_ON(!new_plane_state->color_fill &&
>> + ((new_plane_state->crtc == NULL && new_plane_state->fb !=
>> NULL) ||
>> + (new_plane_state->crtc != NULL && new_plane_state->fb ==
>> NULL)));
>
> Please fix the indentation.
Acked.
Thanks,
Jessica Zhang
>
>> return old_plane_state->crtc && !new_plane_state->crtc;
>> }
>
> --
> With best wishes
> Dmitry
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-10-29 11:23 ` Dmitry Baryshkov
@ 2022-10-31 21:58 ` Jessica Zhang
2022-11-08 18:25 ` Simon Ser
1 sibling, 0 replies; 48+ messages in thread
From: Jessica Zhang @ 2022-10-31 21:58 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart
On 10/29/2022 4:23 AM, Dmitry Baryshkov wrote:
> On 29/10/2022 01:59, Jessica Zhang wrote:
>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
>> drm_plane. In addition, add support for setting and getting the values
>> of these properties.
>>
>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
>> represents the format of the color fill. Userspace can set enable solid
>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
>> framebuffer to NULL.
>
> I suppose that COLOR_FILL should override framebuffer rather than
> requiring that FB is set to NULL. In other words, if color_filL_format
> is non-zero, it would make sense to ignore the FB. Then one can use the
> color_fill_format property to quickly switch between filled plane and
> FB-backed one.
Hey Dmitry,
I think this is a good idea -- acked.
>
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++++++
>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++++++++++++++++++++
>> include/drm/drm_blend.h | 2 ++
>> include/drm/drm_plane.h | 28 +++++++++++++++++++++++
>> 4 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>> b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 79730fa1dd8e..e1664463fca4 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -544,6 +544,10 @@ static int drm_atomic_plane_set_property(struct
>> drm_plane *plane,
>> state->src_w = val;
>> } else if (property == config->prop_src_h) {
>> state->src_h = val;
>> + } else if (property == plane->color_fill_format_property) {
>> + state->color_fill_format = val;
>> + } else if (property == plane->color_fill_property) {
>> + state->color_fill = val;
>> } else if (property == plane->alpha_property) {
>> state->alpha = val;
>> } else if (property == plane->blend_mode_property) {
>> @@ -616,6 +620,10 @@ drm_atomic_plane_get_property(struct drm_plane
>> *plane,
>> *val = state->src_w;
>> } else if (property == config->prop_src_h) {
>> *val = state->src_h;
>> + } else if (property == plane->color_fill_format_property) {
>> + *val = state->color_fill_format;
>> + } else if (property == plane->color_fill_property) {
>> + *val = state->color_fill;
>> } else if (property == plane->alpha_property) {
>> *val = state->alpha;
>> } else if (property == plane->blend_mode_property) {
>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>> index b4c8cab7158c..b8c2b263fa51 100644
>> --- a/drivers/gpu/drm/drm_blend.c
>> +++ b/drivers/gpu/drm/drm_blend.c
>> @@ -616,3 +616,41 @@ int drm_plane_create_blend_mode_property(struct
>> drm_plane *plane,
>> return 0;
>> }
>> EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
>> +
>> +int drm_plane_create_color_fill_property(struct drm_plane *plane)
>> +{
>> + struct drm_property *prop;
>> +
>> + prop = drm_property_create_range(plane->dev, 0, "color_fill",
>> + 0, 0xffffffff);
>> + if (!prop)
>> + return -ENOMEM;
>> +
>> + drm_object_attach_property(&plane->base, prop, 0);
>> + plane->color_fill_property = prop;
>> +
>> + if (plane->state)
>> + plane->state->color_fill = 0;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_plane_create_color_fill_property);
>> +
>> +int drm_plane_create_color_fill_format_property(struct drm_plane *plane)
>> +{
>> + struct drm_property *prop;
>> +
>> + prop = drm_property_create_range(plane->dev, 0, "color_fill_format",
>> + 0, 0xffffffff);
>> + if (!prop)
>> + return -ENOMEM;
>> +
>> + drm_object_attach_property(&plane->base, prop, 0);
>> + plane->color_fill_format_property = prop;
>> +
>> + if (plane->state)
>> + plane->state->color_fill_format = 0;
>
> Don't you also need to reset these properties in
> __drm_atomic_helper_plane_state_reset() ?
Ah, yes I believe so -- acked.
Thanks,
Jessica Zhang
>
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_plane_create_color_fill_format_property);
>> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
>> index 88bdfec3bd88..3e96f5e83cce 100644
>> --- a/include/drm/drm_blend.h
>> +++ b/include/drm/drm_blend.h
>> @@ -58,4 +58,6 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>> struct drm_atomic_state *state);
>> int drm_plane_create_blend_mode_property(struct drm_plane *plane,
>> unsigned int supported_modes);
>> +int drm_plane_create_color_fill_property(struct drm_plane *plane);
>> +int drm_plane_create_color_fill_format_property(struct drm_plane
>> *plane);
>> #endif
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 89ea54652e87..dcbfdb0e1f71 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -116,6 +116,20 @@ struct drm_plane_state {
>> /** @src_h: height of visible portion of plane (in 16.16) */
>> uint32_t src_h, src_w;
>> + /**
>> + * @color_fill_format:
>> + * Format of the color fill value.
>> + */
>> + uint32_t color_fill_format;
>> +
>> + /**
>> + * @color_fill:
>> + * Fill color of the plane with 0 as black and 0xffffffff as white.
>> + * Can be set by user by setting the COLOR_FILL property. See
>> + * drm_plane_create_color_fill_property() for more details.
>> + */
>> + uint32_t color_fill;
>> +
>> /**
>> * @alpha:
>> * Opacity of the plane with 0 as completely transparent and
>> 0xffff as
>> @@ -699,6 +713,20 @@ struct drm_plane {
>> */
>> struct drm_plane_state *state;
>> + /*
>> + * @color_fill_format_property:
>> + * Optional color fill format property for this plane. See
>> + * drm_plane_create_color_fill_format_property().
>> + */
>> + struct drm_property *color_fill_format_property;
>> +
>> + /*
>> + * @color_fill_property:
>> + * Optional color fill property for this plane. See
>> + * drm_plane_create_color_fill_property().
>> + */
>> + struct drm_property *color_fill_property;
>> +
>> /**
>> * @alpha_property:
>> * Optional alpha property for this plane. See
>
> --
> With best wishes
> Dmitry
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 3/3] drm/msm/dpu: Use color_fill property for DPU planes
2022-10-29 11:40 ` Dmitry Baryshkov
@ 2022-10-31 22:14 ` Jessica Zhang
0 siblings, 0 replies; 48+ messages in thread
From: Jessica Zhang @ 2022-10-31 22:14 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart
On 10/29/2022 4:40 AM, Dmitry Baryshkov wrote:
> On 29/10/2022 01:59, Jessica Zhang wrote:
>> Initialize and use the color_fill properties for planes in DPU driver. In
>> addition, relax framebuffer requirements within atomic commit path and
>> add checks for NULL framebuffers.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++---------
>> 2 files changed, 48 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 13ce321283ff..157698b4f234 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -441,7 +441,12 @@ static void _dpu_crtc_blend_setup_mixer(struct
>> drm_crtc *crtc,
>> sspp_idx - SSPP_VIG0,
>> state->fb ? state->fb->base.id : -1);
>> - format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
>> + if (pstate->base.fb)
>> + format =
>> to_dpu_format(msm_framebuffer_format(pstate->base.fb));
>> + else if (state->color_fill && !state->color_fill_format)
>> + format = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
>
> As I wrote in the review of the earlier patch, this disallows using
> black as the plane fill colour. Not to mention that using ABGR should be
> explicit rather than implicit.
Hey Dmitry,
Acked.
Thanks,
Jessica Zhang
>
>> + else
>> + format = dpu_get_dpu_format(state->color_fill_format);
>> if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
>> bg_alpha_enable = true;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> index 658005f609f4..f3be37e97b64 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> @@ -103,7 +103,6 @@ struct dpu_plane {
>> enum dpu_sspp pipe;
>> struct dpu_hw_pipe *pipe_hw;
>> - uint32_t color_fill;
>> bool is_error;
>> bool is_rt_pipe;
>> const struct dpu_mdss_cfg *catalog;
>> @@ -697,7 +696,10 @@ static int _dpu_plane_color_fill(struct dpu_plane
>> *pdpu,
>> * select fill format to match user property expectation,
>> * h/w only supports RGB variants
>> */
>> - fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
>> + if (plane->state->color_fill && !plane->state->color_fill_format)
>> + fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
>> + else
>> + fmt = dpu_get_dpu_format(plane->state->color_fill_format);
>> /* update sspp */
>> if (fmt && pdpu->pipe_hw->ops.setup_solidfill) {
>> @@ -720,6 +722,10 @@ static int _dpu_plane_color_fill(struct dpu_plane
>> *pdpu,
>> fmt, DPU_SSPP_SOLID_FILL,
>> pstate->multirect_index);
>> + /* skip remaining processing on color fill */
>> + if (!plane->state->fb)
>> + return 0;
>> +
>> if (pdpu->pipe_hw->ops.setup_rects)
>> pdpu->pipe_hw->ops.setup_rects(pdpu->pipe_hw,
>> &pipe_cfg,
>> @@ -999,12 +1005,21 @@ static int dpu_plane_atomic_check(struct
>> drm_plane *plane,
>> dst = drm_plane_state_dest(new_plane_state);
>> - fb_rect.x2 = new_plane_state->fb->width;
>> - fb_rect.y2 = new_plane_state->fb->height;
>> + if (new_plane_state->fb) {
>> + fb_rect.x2 = new_plane_state->fb->width;
>> + fb_rect.y2 = new_plane_state->fb->height;
>> + }
>> max_linewidth = pdpu->catalog->caps->max_linewidth;
>> - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
>> + if (new_plane_state->fb) {
>> + fmt =
>> to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
>> + } else if (new_plane_state->color_fill) {
>> + if (new_plane_state->color_fill_format)
>> + fmt =
>> dpu_get_dpu_format(new_plane_state->color_fill_format);
>> + else
>> + fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
>> + }
>> min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>> @@ -1016,7 +1031,7 @@ static int dpu_plane_atomic_check(struct
>> drm_plane *plane,
>> return -EINVAL;
>> /* check src bounds */
>> - } else if (!dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
>> + } else if (new_plane_state->fb && !dpu_plane_validate_src(&src,
>> &fb_rect, min_src_size)) {
>> DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
>> DRM_RECT_ARG(&src));
>> return -E2BIG;
>> @@ -1084,9 +1099,9 @@ void dpu_plane_flush(struct drm_plane *plane)
>> if (pdpu->is_error)
>> /* force white frame with 100% alpha pipe output on error */
>> _dpu_plane_color_fill(pdpu, 0xFFFFFF, 0xFF);
>> - else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
>> + else if (!(plane->state->fb) && plane->state->color_fill)
>> /* force 100% alpha */
>> - _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
>> + _dpu_plane_color_fill(pdpu, plane->state->color_fill, 0xFF);
>> else if (pdpu->pipe_hw && pdpu->pipe_hw->ops.setup_csc) {
>> const struct dpu_format *fmt =
>> to_dpu_format(msm_framebuffer_format(plane->state->fb));
>> const struct dpu_csc_cfg *csc_ptr = _dpu_plane_get_csc(pdpu,
>> fmt);
>> @@ -1125,23 +1140,30 @@ static void
>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>> struct drm_crtc *crtc = state->crtc;
>> struct drm_framebuffer *fb = state->fb;
>> bool is_rt_pipe, update_qos_remap;
>> - const struct dpu_format *fmt =
>> - to_dpu_format(msm_framebuffer_format(fb));
>> + const struct dpu_format *fmt;
>> struct dpu_hw_pipe_cfg pipe_cfg;
>> - memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
>> -
>> - _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
>> -
>> pstate->pending = true;
>> is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
>> _dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
>> - DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u "
>> DRM_RECT_FMT
>> - ", %4.4s ubwc %d\n", fb->base.id,
>> DRM_RECT_FP_ARG(&state->src),
>> - crtc->base.id, DRM_RECT_ARG(&state->dst),
>> - (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt));
>> + /* override for color fill */
>> + if (!fb && plane->state->color_fill) {
>> + /* skip remaining processing on color fill */
>> + return;
>> + }
>> +
>> + memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
>> +
>> + fmt = to_dpu_format(msm_framebuffer_format(fb));
>> + _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
>> +
>> + if (fb)
>> + DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u "
>> DRM_RECT_FMT
>> + ", %4.4s ubwc %d\n", fb->base.id,
>> DRM_RECT_FP_ARG(&state->src),
>> + crtc->base.id, DRM_RECT_ARG(&state->dst),
>> + (char *)&fmt->base.pixel_format,
>> DPU_FORMAT_IS_UBWC(fmt));
>> pipe_cfg.src_rect = state->src;
>> @@ -1153,12 +1175,6 @@ static void dpu_plane_sspp_atomic_update(struct
>> drm_plane *plane)
>> pipe_cfg.dst_rect = state->dst;
>> - /* override for color fill */
>> - if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
>> - /* skip remaining processing on color fill */
>> - return;
>> - }
>> -
>> if (pdpu->pipe_hw->ops.setup_rects) {
>> pdpu->pipe_hw->ops.setup_rects(pdpu->pipe_hw,
>> &pipe_cfg,
>> @@ -1509,6 +1525,8 @@ struct drm_plane *dpu_plane_init(struct
>> drm_device *dev,
>> DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
>> drm_plane_create_alpha_property(plane);
>> + drm_plane_create_color_fill_property(plane);
>> + drm_plane_create_color_fill_format_property(plane);
>> drm_plane_create_blend_mode_property(plane,
>> BIT(DRM_MODE_BLEND_PIXEL_NONE) |
>> BIT(DRM_MODE_BLEND_PREMULTI) |
>
> --
> With best wishes
> Dmitry
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-10-29 12:08 ` Dmitry Baryshkov
@ 2022-10-31 22:24 ` Jessica Zhang
2022-11-01 0:11 ` Dmitry Baryshkov
2022-11-08 18:50 ` Simon Ser
1 sibling, 1 reply; 48+ messages in thread
From: Jessica Zhang @ 2022-10-31 22:24 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart
On 10/29/2022 5:08 AM, Dmitry Baryshkov wrote:
> On 29/10/2022 01:59, Jessica Zhang wrote:
>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
>> drm_plane. In addition, add support for setting and getting the values
>> of these properties.
>>
>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
>> represents the format of the color fill. Userspace can set enable solid
>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
>> framebuffer to NULL.
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >
> Planes report supported formats using the drm_mode_getplane(). You'd
> also need to tell userspace, which formats are supported for color fill.
> I don't think one supports e.g. YV12.
Hey Dmitry,
Good point. Forgot to add this in the patch [3/3] commit message, but
FWIW MSM DPU devices only support the RGB format variants [1].
[1]
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697
>
> A bit of generic comment for the discussion (this is an RFC anyway).
> Using color_fill/color_fill_format properties sounds simple, but this
> might be not generic enough. Limiting color_fill to 32 bits would
> prevent anybody from using floating point formats (e.g.
> DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
> e.g. using 64-bit for the color_fill value, but then this doesn't sound
> extensible too much.
Hm... I can definitely change color_fill to use u64 instead. As for
making it more extensible, do you have any suggestions?
>
> So, a question for other hardware maintainers. Do we have hardware that
> supports such 'color filled' planes? Do we want to support format
> modifiers for filling color/data? Because what I have in mind is closer
> to the blob structure, which can then be used for filling the plane:
>
> struct color_fill_blob {
> u32 pixel_format;
> u64 modifiers4];
> u32 pixel_data_size; // fixme: is this necessary?
> u8 pixel_data[];
> };
Just a question about this blob struct -- what is the purpose of pixel_data?
At least for devices using the DPU driver, the only data needed to
enable solid fill is color_fill and color_fill_format. I'd also be
interested in knowing if there are other drivers support a similar
feature and what is needed for them.
>
> And then... This sounds a lot like a custom framebuffer.
>
> So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
> flag to the framebuffers, which would e.g. mean that the FB gets stamped
> all over the plane. This would also save us from changing if (!fb)
> checks all over the drm core.
JFYI we did originally consider using a custom 1x1 FB to for color fill
[1], but decided to go with a plane property instead. IIRC the
conclusion was that having color fill as a plane property is a cleaner
solution.
As for creating a new blob struct to hold all color fill info, I'm open
to implementing that over having 2 separate properties.
[1] https://oftc.irclog.whitequark.org/dri-devel/2022-09-23#31409842
>
> Another approach might be using a format modifier instead of the FB flag.
I like the idea of having a format modifier denoting if the driver
supports color_fill for that specific format. This would allow userspace
to know which formats are supported by solid fill planes.
Thanks,
Jessica Zhang
>
> What do you think?
>
> --
> With best wishes
> Dmitry
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-10-31 22:24 ` Jessica Zhang
@ 2022-11-01 0:11 ` Dmitry Baryshkov
2022-11-01 17:35 ` Jessica Zhang
0 siblings, 1 reply; 48+ messages in thread
From: Dmitry Baryshkov @ 2022-11-01 0:11 UTC (permalink / raw)
To: Jessica Zhang, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart
Hi,
On 01/11/2022 01:24, Jessica Zhang wrote:
>
>
> On 10/29/2022 5:08 AM, Dmitry Baryshkov wrote:
>> On 29/10/2022 01:59, Jessica Zhang wrote:
>>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
>>> drm_plane. In addition, add support for setting and getting the values
>>> of these properties.
>>>
>>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
>>> represents the format of the color fill. Userspace can set enable solid
>>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
>>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
>>> framebuffer to NULL.
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >
>> Planes report supported formats using the drm_mode_getplane(). You'd
>> also need to tell userspace, which formats are supported for color
>> fill. I don't think one supports e.g. YV12.
>
> Hey Dmitry,
>
> Good point. Forgot to add this in the patch [3/3] commit message, but
> FWIW MSM DPU devices only support the RGB format variants [1].
>
> [1]
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697
Ack. So you'd need to tell this to userspace.
>
>>
>> A bit of generic comment for the discussion (this is an RFC anyway).
>> Using color_fill/color_fill_format properties sounds simple, but this
>> might be not generic enough. Limiting color_fill to 32 bits would
>> prevent anybody from using floating point formats (e.g.
>> DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
>> e.g. using 64-bit for the color_fill value, but then this doesn't
>> sound extensible too much.
>
> Hm... I can definitely change color_fill to use u64 instead. As for
> making it more extensible, do you have any suggestions?
No. Not u64. It is a blob. Basically because when designing API you can
not guarantee that all fill values would fit into u64. Also see below.
>
>>
>> So, a question for other hardware maintainers. Do we have hardware
>> that supports such 'color filled' planes? Do we want to support format
>> modifiers for filling color/data? Because what I have in mind is
>> closer to the blob structure, which can then be used for filling the
>> plane:
>>
>> struct color_fill_blob {
>> u32 pixel_format;
>> u64 modifiers4];
>> u32 pixel_data_size; // fixme: is this necessary?
>> u8 pixel_data[];
>> };
>
> Just a question about this blob struct -- what is the purpose of
> pixel_data?
>
> At least for devices using the DPU driver, the only data needed to
> enable solid fill is color_fill and color_fill_format. I'd also be
> interested in knowing if there are other drivers support a similar
> feature and what is needed for them.
Yes. You are thinking from the DPU point of view. ARGB only. However as
we are adding generic API, we should not limit ourselves to it. Other
deivces might support other formats of fill data. For example using
YUY2/UYVY for filling the plane. And such YUV data is not a colour
anymore. It is a pixel data, just as I named it.
Another hardware might support some fill patterns. Or e.g. passing a
compressed texels/macrotiles. So, pixel data. Note, I've added format
modifiers. Maybe `u64 modifiers[4]` is an overkill, as we have just a
single data plane. Maybe just `u64 modifier` would be enough.
>
>>
>> And then... This sounds a lot like a custom framebuffer.
>>
>> So, maybe what should we do instead is to add new
>> DRM_MODE_FB_COLOR_FILL flag to the framebuffers, which would e.g. mean
>> that the FB gets stamped all over the plane. This would also save us
>> from changing if (!fb) checks all over the drm core.
>
> JFYI we did originally consider using a custom 1x1 FB to for color fill
> [1], but decided to go with a plane property instead. IIRC the
> conclusion was that having color fill as a plane property is a cleaner
> solution.
>
> As for creating a new blob struct to hold all color fill info, I'm open
> to implementing that over having 2 separate properties.
>
> [1] https://oftc.irclog.whitequark.org/dri-devel/2022-09-23#31409842
Let me cite the conclusion form the IRC chat: `22:20 <robclark>
abhinav__: kinda.. the proposal was that userspace creates a blob
property with the solid fill color, and then attaches the blob-prop id
to the plane's FB_ID`.
It's not a pair of properties. It is a blob, because it is not that
limited as the pair of range properties is.
I will reread the log later, but just my 2c. Attaching the blob property
as the FB_ID might confuse userspace. I'd be slightly biased to being
more conservative here. However as the final proposal was to attach the
blob ID, let's do it this way.
>
>>
>> Another approach might be using a format modifier instead of the FB flag.
> I like the idea of having a format modifier denoting if the driver
> supports color_fill for that specific format. This would allow userspace
> to know which formats are supported by solid fill planes.
Yes, exactly. It would come in a natural way.
[Rumbling: and then it's natural to have the fill data in FB. Dull mode
off.]
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-01 0:11 ` Dmitry Baryshkov
@ 2022-11-01 17:35 ` Jessica Zhang
0 siblings, 0 replies; 48+ messages in thread
From: Jessica Zhang @ 2022-11-01 17:35 UTC (permalink / raw)
To: Dmitry Baryshkov, freedreno
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart
On 10/31/2022 5:11 PM, Dmitry Baryshkov wrote:
> Hi,
>
> On 01/11/2022 01:24, Jessica Zhang wrote:
>>
>>
>> On 10/29/2022 5:08 AM, Dmitry Baryshkov wrote:
>>> On 29/10/2022 01:59, Jessica Zhang wrote:
>>>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
>>>> drm_plane. In addition, add support for setting and getting the values
>>>> of these properties.
>>>>
>>>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
>>>> represents the format of the color fill. Userspace can set enable solid
>>>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
>>>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
>>>> framebuffer to NULL.
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >
>>> Planes report supported formats using the drm_mode_getplane(). You'd
>>> also need to tell userspace, which formats are supported for color
>>> fill. I don't think one supports e.g. YV12.
>>
>> Hey Dmitry,
>>
>> Good point. Forgot to add this in the patch [3/3] commit message, but
>> FWIW MSM DPU devices only support the RGB format variants [1].
>>
>> [1]
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697
>
> Ack. So you'd need to tell this to userspace.
>
>>
>>>
>>> A bit of generic comment for the discussion (this is an RFC anyway).
>>> Using color_fill/color_fill_format properties sounds simple, but this
>>> might be not generic enough. Limiting color_fill to 32 bits would
>>> prevent anybody from using floating point formats (e.g.
>>> DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
>>> e.g. using 64-bit for the color_fill value, but then this doesn't
>>> sound extensible too much.
>>
>> Hm... I can definitely change color_fill to use u64 instead. As for
>> making it more extensible, do you have any suggestions?
>
> No. Not u64. It is a blob. Basically because when designing API you can
> not guarantee that all fill values would fit into u64. Also see below.
>
>>
>>>
>>> So, a question for other hardware maintainers. Do we have hardware
>>> that supports such 'color filled' planes? Do we want to support
>>> format modifiers for filling color/data? Because what I have in mind
>>> is closer to the blob structure, which can then be used for filling
>>> the plane:
>>>
>>> struct color_fill_blob {
>>> u32 pixel_format;
>>> u64 modifiers4];
>>> u32 pixel_data_size; // fixme: is this necessary?
>>> u8 pixel_data[];
>>> };
>>
>> Just a question about this blob struct -- what is the purpose of
>> pixel_data?
>>
>> At least for devices using the DPU driver, the only data needed to
>> enable solid fill is color_fill and color_fill_format. I'd also be
>> interested in knowing if there are other drivers support a similar
>> feature and what is needed for them.
>
> Yes. You are thinking from the DPU point of view. ARGB only. However as
> we are adding generic API, we should not limit ourselves to it. Other
> deivces might support other formats of fill data. For example using
> YUY2/UYVY for filling the plane. And such YUV data is not a colour
> anymore. It is a pixel data, just as I named it.
>
> Another hardware might support some fill patterns. Or e.g. passing a
> compressed texels/macrotiles. So, pixel data. Note, I've added format
> modifiers. Maybe `u64 modifiers[4]` is an overkill, as we have just a
> single data plane. Maybe just `u64 modifier` would be enough.
Got it, I think that's reasonable then.
>
>>
>>>
>>> And then... This sounds a lot like a custom framebuffer.
>>>
>>> So, maybe what should we do instead is to add new
>>> DRM_MODE_FB_COLOR_FILL flag to the framebuffers, which would e.g.
>>> mean that the FB gets stamped all over the plane. This would also
>>> save us from changing if (!fb) checks all over the drm core.
>>
>> JFYI we did originally consider using a custom 1x1 FB to for color
>> fill [1], but decided to go with a plane property instead. IIRC the
>> conclusion was that having color fill as a plane property is a cleaner
>> solution.
>>
>> As for creating a new blob struct to hold all color fill info, I'm
>> open to implementing that over having 2 separate properties.
>>
>> [1] https://oftc.irclog.whitequark.org/dri-devel/2022-09-23#31409842
>
> Let me cite the conclusion form the IRC chat: `22:20 <robclark>
> abhinav__: kinda.. the proposal was that userspace creates a blob
> property with the solid fill color, and then attaches the blob-prop id
> to the plane's FB_ID`.
>
> It's not a pair of properties. It is a blob, because it is not that
> limited as the pair of range properties is.
>
> I will reread the log later, but just my 2c. Attaching the blob property
> as the FB_ID might confuse userspace. I'd be slightly biased to being
> more conservative here. However as the final proposal was to attach the
> blob ID, let's do it this way.
Sounds good. Will spin up a v2 with the prop_blob implementation.
Thanks,
Jessica Zhang
>
>>
>>>
>>> Another approach might be using a format modifier instead of the FB
>>> flag.
>> I like the idea of having a format modifier denoting if the driver
>> supports color_fill for that specific format. This would allow
>> userspace to know which formats are supported by solid fill planes.
>
> Yes, exactly. It would come in a natural way.
>
> [Rumbling: and then it's natural to have the fill data in FB. Dull mode
> off.]
>
> --
> With best wishes
> Dmitry
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2022-10-28 22:59 [RFC PATCH 0/3] Support for Solid Fill Planes Jessica Zhang
` (2 preceding siblings ...)
2022-10-28 22:59 ` [RFC PATCH 3/3] drm/msm/dpu: Use color_fill property for DPU planes Jessica Zhang
@ 2022-11-07 19:37 ` Ville Syrjälä
2022-11-07 21:32 ` Jessica Zhang
` (2 more replies)
3 siblings, 3 replies; 48+ messages in thread
From: Ville Syrjälä @ 2022-11-07 19:37 UTC (permalink / raw)
To: Jessica Zhang
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, dmitry.baryshkov, freedreno
On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> properties. When the color fill value is set, and the framebuffer is set
> to NULL, memory fetch will be disabled.
Thinking a bit more universally I wonder if there should be
some kind of enum property:
enum plane_pixel_source {
FB,
COLOR,
LIVE_FOO,
LIVE_BAR,
...
}
> In addition, loosen the NULL FB checks within the atomic commit callstack
> to allow a NULL FB when color_fill is nonzero and add FB checks in
> methods where the FB was previously assumed to be non-NULL.
>
> Finally, have the DPU driver use drm_plane_state.color_fill and
> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
> and add extra checks in the DPU atomic commit callstack to account for a
> NULL FB in cases where color_fill is set.
>
> Some drivers support hardware that have optimizations for solid fill
> planes. This series aims to expose these capabilities to userspace as
> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> hardware composer HAL) that can be set by apps like the Android Gears
> app.
>
> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
> DRM format, setting COLOR_FILL to a color fill value, and setting the
> framebuffer to NULL.
Is there some real reason for the format property? Ie. why not
just do what was the plan for the crttc background color and
specify the color in full 16bpc format and just pick as many
msbs from that as the hw can use?
>
> Jessica Zhang (3):
> drm: Introduce color fill properties for drm plane
> drm: Adjust atomic checks for solid fill color
> drm/msm/dpu: Use color_fill property for DPU planes
>
> drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++-----------
> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
> drivers/gpu/drm/drm_plane.c | 8 +--
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
> include/drm/drm_atomic_helper.h | 5 +-
> include/drm/drm_blend.h | 2 +
> include/drm/drm_plane.h | 28 ++++++++++
> 10 files changed, 188 insertions(+), 76 deletions(-)
>
> --
> 2.38.0
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2022-11-07 19:37 ` [RFC PATCH 0/3] Support for Solid Fill Planes Ville Syrjälä
@ 2022-11-07 21:32 ` Jessica Zhang
2022-11-07 22:09 ` [Freedreno] " Rob Clark
2022-11-07 23:06 ` Laurent Pinchart
2023-06-26 23:02 ` Jessica Zhang
2 siblings, 1 reply; 48+ messages in thread
From: Jessica Zhang @ 2022-11-07 21:32 UTC (permalink / raw)
To: Ville Syrjälä
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, dmitry.baryshkov, freedreno
On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>> properties. When the color fill value is set, and the framebuffer is set
>> to NULL, memory fetch will be disabled.
>
> Thinking a bit more universally I wonder if there should be
> some kind of enum property:
>
> enum plane_pixel_source {
> FB,
> COLOR,
> LIVE_FOO,
> LIVE_BAR,
> ...
> }
Hi Ville,
Makes sense -- this way, we'd also lay some groundwork for cases where
drivers want to use other non-FB sources.
>
>> In addition, loosen the NULL FB checks within the atomic commit callstack
>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>> methods where the FB was previously assumed to be non-NULL.
>>
>> Finally, have the DPU driver use drm_plane_state.color_fill and
>> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
>> and add extra checks in the DPU atomic commit callstack to account for a
>> NULL FB in cases where color_fill is set.
>>
>> Some drivers support hardware that have optimizations for solid fill
>> planes. This series aims to expose these capabilities to userspace as
>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
>> hardware composer HAL) that can be set by apps like the Android Gears
>> app.
>>
>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
>> DRM format, setting COLOR_FILL to a color fill value, and setting the
>> framebuffer to NULL.
>
> Is there some real reason for the format property? Ie. why not
> just do what was the plan for the crttc background color and
> specify the color in full 16bpc format and just pick as many
> msbs from that as the hw can use?
The format property was added because we can't assume that all hardware
will support/use the same color format for solid fill planes. Even for
just MSM devices, the hardware supports different variations of RGB
formats [1].
Thanks,
Jessica Zhang
[1]
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697
>
>>
>> Jessica Zhang (3):
>> drm: Introduce color fill properties for drm plane
>> drm: Adjust atomic checks for solid fill color
>> drm/msm/dpu: Use color_fill property for DPU planes
>>
>> drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++-----------
>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
>> drivers/gpu/drm/drm_plane.c | 8 +--
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
>> include/drm/drm_atomic_helper.h | 5 +-
>> include/drm/drm_blend.h | 2 +
>> include/drm/drm_plane.h | 28 ++++++++++
>> 10 files changed, 188 insertions(+), 76 deletions(-)
>>
>> --
>> 2.38.0
>
> --
> Ville Syrjälä
> Intel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
2022-11-07 21:32 ` Jessica Zhang
@ 2022-11-07 22:09 ` Rob Clark
2022-11-08 0:22 ` Jessica Zhang
0 siblings, 1 reply; 48+ messages in thread
From: Rob Clark @ 2022-11-07 22:09 UTC (permalink / raw)
To: Jessica Zhang
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, dmitry.baryshkov, freedreno
On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>
> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> > On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> >> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> >> properties. When the color fill value is set, and the framebuffer is set
> >> to NULL, memory fetch will be disabled.
> >
> > Thinking a bit more universally I wonder if there should be
> > some kind of enum property:
> >
> > enum plane_pixel_source {
> > FB,
> > COLOR,
> > LIVE_FOO,
> > LIVE_BAR,
> > ...
> > }
>
> Hi Ville,
>
> Makes sense -- this way, we'd also lay some groundwork for cases where
> drivers want to use other non-FB sources.
>
> >
> >> In addition, loosen the NULL FB checks within the atomic commit callstack
> >> to allow a NULL FB when color_fill is nonzero and add FB checks in
> >> methods where the FB was previously assumed to be non-NULL.
> >>
> >> Finally, have the DPU driver use drm_plane_state.color_fill and
> >> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
> >> and add extra checks in the DPU atomic commit callstack to account for a
> >> NULL FB in cases where color_fill is set.
> >>
> >> Some drivers support hardware that have optimizations for solid fill
> >> planes. This series aims to expose these capabilities to userspace as
> >> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> >> hardware composer HAL) that can be set by apps like the Android Gears
> >> app.
> >>
> >> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
> >> DRM format, setting COLOR_FILL to a color fill value, and setting the
> >> framebuffer to NULL.
> >
> > Is there some real reason for the format property? Ie. why not
> > just do what was the plan for the crttc background color and
> > specify the color in full 16bpc format and just pick as many
> > msbs from that as the hw can use?
>
> The format property was added because we can't assume that all hardware
> will support/use the same color format for solid fill planes. Even for
> just MSM devices, the hardware supports different variations of RGB
> formats [1].
Sure, but the driver can convert the format into whatever the hw
wants. A 1x1 color conversion is not going to be problematic ;-)
BR,
-R
> Thanks,
>
> Jessica Zhang
>
> [1]
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697
>
> >
> >>
> >> Jessica Zhang (3):
> >> drm: Introduce color fill properties for drm plane
> >> drm: Adjust atomic checks for solid fill color
> >> drm/msm/dpu: Use color_fill property for DPU planes
> >>
> >> drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++-----------
> >> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
> >> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
> >> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
> >> drivers/gpu/drm/drm_plane.c | 8 +--
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
> >> include/drm/drm_atomic_helper.h | 5 +-
> >> include/drm/drm_blend.h | 2 +
> >> include/drm/drm_plane.h | 28 ++++++++++
> >> 10 files changed, 188 insertions(+), 76 deletions(-)
> >>
> >> --
> >> 2.38.0
> >
> > --
> > Ville Syrjälä
> > Intel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2022-11-07 19:37 ` [RFC PATCH 0/3] Support for Solid Fill Planes Ville Syrjälä
2022-11-07 21:32 ` Jessica Zhang
@ 2022-11-07 23:06 ` Laurent Pinchart
2023-06-26 23:02 ` Jessica Zhang
2 siblings, 0 replies; 48+ messages in thread
From: Laurent Pinchart @ 2022-11-07 23:06 UTC (permalink / raw)
To: Ville Syrjälä
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
daniel.vetter, dmitry.baryshkov, Jessica Zhang, freedreno
On Mon, Nov 07, 2022 at 09:37:08PM +0200, Ville Syrjälä wrote:
> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> > Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> > properties. When the color fill value is set, and the framebuffer is set
> > to NULL, memory fetch will be disabled.
>
> Thinking a bit more universally I wonder if there should be
> some kind of enum property:
>
> enum plane_pixel_source {
> FB,
> COLOR,
> LIVE_FOO,
> LIVE_BAR,
> ...
> }
That's something I could use on (older) Renesas hardware, where planes
can be fed with a live source. The issue there is that the live source
is a compositor with multiple inputs, handled through the V4L2 API. How
to synchronize the configuration of the compositor and the display
engine is an unsolved problem at the moment, but it could be solved
another day.
> > In addition, loosen the NULL FB checks within the atomic commit callstack
> > to allow a NULL FB when color_fill is nonzero and add FB checks in
> > methods where the FB was previously assumed to be non-NULL.
> >
> > Finally, have the DPU driver use drm_plane_state.color_fill and
> > drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
> > and add extra checks in the DPU atomic commit callstack to account for a
> > NULL FB in cases where color_fill is set.
> >
> > Some drivers support hardware that have optimizations for solid fill
> > planes. This series aims to expose these capabilities to userspace as
> > some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> > hardware composer HAL) that can be set by apps like the Android Gears
> > app.
> >
> > Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
> > DRM format, setting COLOR_FILL to a color fill value, and setting the
> > framebuffer to NULL.
>
> Is there some real reason for the format property? Ie. why not
> just do what was the plan for the crttc background color and
> specify the color in full 16bpc format and just pick as many
> msbs from that as the hw can use?
>
> > Jessica Zhang (3):
> > drm: Introduce color fill properties for drm plane
> > drm: Adjust atomic checks for solid fill color
> > drm/msm/dpu: Use color_fill property for DPU planes
> >
> > drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++-----------
> > drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
> > drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
> > drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
> > drivers/gpu/drm/drm_plane.c | 8 +--
> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
> > include/drm/drm_atomic_helper.h | 5 +-
> > include/drm/drm_blend.h | 2 +
> > include/drm/drm_plane.h | 28 ++++++++++
> > 10 files changed, 188 insertions(+), 76 deletions(-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
2022-11-07 22:09 ` [Freedreno] " Rob Clark
@ 2022-11-08 0:22 ` Jessica Zhang
2022-11-08 3:34 ` Rob Clark
0 siblings, 1 reply; 48+ messages in thread
From: Jessica Zhang @ 2022-11-08 0:22 UTC (permalink / raw)
To: Rob Clark
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, dmitry.baryshkov, freedreno
On 11/7/2022 2:09 PM, Rob Clark wrote:
> On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>>
>>
>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>>>> properties. When the color fill value is set, and the framebuffer is set
>>>> to NULL, memory fetch will be disabled.
>>>
>>> Thinking a bit more universally I wonder if there should be
>>> some kind of enum property:
>>>
>>> enum plane_pixel_source {
>>> FB,
>>> COLOR,
>>> LIVE_FOO,
>>> LIVE_BAR,
>>> ...
>>> }
>>
>> Hi Ville,
>>
>> Makes sense -- this way, we'd also lay some groundwork for cases where
>> drivers want to use other non-FB sources.
>>
>>>
>>>> In addition, loosen the NULL FB checks within the atomic commit callstack
>>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>>>> methods where the FB was previously assumed to be non-NULL.
>>>>
>>>> Finally, have the DPU driver use drm_plane_state.color_fill and
>>>> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
>>>> and add extra checks in the DPU atomic commit callstack to account for a
>>>> NULL FB in cases where color_fill is set.
>>>>
>>>> Some drivers support hardware that have optimizations for solid fill
>>>> planes. This series aims to expose these capabilities to userspace as
>>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
>>>> hardware composer HAL) that can be set by apps like the Android Gears
>>>> app.
>>>>
>>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
>>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
>>>> framebuffer to NULL.
>>>
>>> Is there some real reason for the format property? Ie. why not
>>> just do what was the plan for the crttc background color and
>>> specify the color in full 16bpc format and just pick as many
>>> msbs from that as the hw can use?
>>
>> The format property was added because we can't assume that all hardware
>> will support/use the same color format for solid fill planes. Even for
>> just MSM devices, the hardware supports different variations of RGB
>> formats [1].
>
> Sure, but the driver can convert the format into whatever the hw
> wants. A 1x1 color conversion is not going to be problematic ;-)
Hi Rob,
Hm... that's also a fair point. Just wondering, is there any advantage
of having the driver convert the format, other than not having to
implement an extra format property?
(In case we end up wrapping everything into a prop blob or something)
Thanks,
Jessica Zhang
>
> BR,
> -R
>
>> Thanks,
>>
>> Jessica Zhang
>>
>> [1]
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697
>>
>>>
>>>>
>>>> Jessica Zhang (3):
>>>> drm: Introduce color fill properties for drm plane
>>>> drm: Adjust atomic checks for solid fill color
>>>> drm/msm/dpu: Use color_fill property for DPU planes
>>>>
>>>> drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++-----------
>>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
>>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
>>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
>>>> drivers/gpu/drm/drm_plane.c | 8 +--
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
>>>> include/drm/drm_atomic_helper.h | 5 +-
>>>> include/drm/drm_blend.h | 2 +
>>>> include/drm/drm_plane.h | 28 ++++++++++
>>>> 10 files changed, 188 insertions(+), 76 deletions(-)
>>>>
>>>> --
>>>> 2.38.0
>>>
>>> --
>>> Ville Syrjälä
>>> Intel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
2022-11-08 0:22 ` Jessica Zhang
@ 2022-11-08 3:34 ` Rob Clark
2022-11-08 8:52 ` Ville Syrjälä
0 siblings, 1 reply; 48+ messages in thread
From: Rob Clark @ 2022-11-08 3:34 UTC (permalink / raw)
To: Jessica Zhang
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, dmitry.baryshkov, freedreno
On Mon, Nov 7, 2022 at 4:22 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>
> On 11/7/2022 2:09 PM, Rob Clark wrote:
> > On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> >>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> >>>> properties. When the color fill value is set, and the framebuffer is set
> >>>> to NULL, memory fetch will be disabled.
> >>>
> >>> Thinking a bit more universally I wonder if there should be
> >>> some kind of enum property:
> >>>
> >>> enum plane_pixel_source {
> >>> FB,
> >>> COLOR,
> >>> LIVE_FOO,
> >>> LIVE_BAR,
> >>> ...
> >>> }
> >>
> >> Hi Ville,
> >>
> >> Makes sense -- this way, we'd also lay some groundwork for cases where
> >> drivers want to use other non-FB sources.
> >>
> >>>
> >>>> In addition, loosen the NULL FB checks within the atomic commit callstack
> >>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
> >>>> methods where the FB was previously assumed to be non-NULL.
> >>>>
> >>>> Finally, have the DPU driver use drm_plane_state.color_fill and
> >>>> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
> >>>> and add extra checks in the DPU atomic commit callstack to account for a
> >>>> NULL FB in cases where color_fill is set.
> >>>>
> >>>> Some drivers support hardware that have optimizations for solid fill
> >>>> planes. This series aims to expose these capabilities to userspace as
> >>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> >>>> hardware composer HAL) that can be set by apps like the Android Gears
> >>>> app.
> >>>>
> >>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
> >>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
> >>>> framebuffer to NULL.
> >>>
> >>> Is there some real reason for the format property? Ie. why not
> >>> just do what was the plan for the crttc background color and
> >>> specify the color in full 16bpc format and just pick as many
> >>> msbs from that as the hw can use?
> >>
> >> The format property was added because we can't assume that all hardware
> >> will support/use the same color format for solid fill planes. Even for
> >> just MSM devices, the hardware supports different variations of RGB
> >> formats [1].
> >
> > Sure, but the driver can convert the format into whatever the hw
> > wants. A 1x1 color conversion is not going to be problematic ;-)
>
> Hi Rob,
>
> Hm... that's also a fair point. Just wondering, is there any advantage
> of having the driver convert the format, other than not having to
> implement an extra format property?
>
> (In case we end up wrapping everything into a prop blob or something)
>
It keeps the uabi simpler.. for obvious reasons you don't want the
driver to do cpu color conversion for an arbitrary size plane, which
is why we go to all the complexity to expose formats and modifiers for
"real" planes, but we are dealing with a single pixel value here,
let's not make the uabi more complex than we need to. I'd propose
making it float32[4] if float weren't a pita for kernel/uabi, but
u16[4] or u32[4] should be fine, and drivers can translate that easily
into whatever weird formats their hw wants for solid-fill.
BR,
-R
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
2022-11-08 3:34 ` Rob Clark
@ 2022-11-08 8:52 ` Ville Syrjälä
2022-11-22 23:20 ` Jessica Zhang
0 siblings, 1 reply; 48+ messages in thread
From: Ville Syrjälä @ 2022-11-08 8:52 UTC (permalink / raw)
To: Rob Clark
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, dmitry.baryshkov, Jessica Zhang,
freedreno
On Mon, Nov 07, 2022 at 07:34:43PM -0800, Rob Clark wrote:
> On Mon, Nov 7, 2022 at 4:22 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >
> >
> >
> > On 11/7/2022 2:09 PM, Rob Clark wrote:
> > > On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> > >>
> > >>
> > >>
> > >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> > >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> > >>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> > >>>> properties. When the color fill value is set, and the framebuffer is set
> > >>>> to NULL, memory fetch will be disabled.
> > >>>
> > >>> Thinking a bit more universally I wonder if there should be
> > >>> some kind of enum property:
> > >>>
> > >>> enum plane_pixel_source {
> > >>> FB,
> > >>> COLOR,
> > >>> LIVE_FOO,
> > >>> LIVE_BAR,
> > >>> ...
> > >>> }
> > >>
> > >> Hi Ville,
> > >>
> > >> Makes sense -- this way, we'd also lay some groundwork for cases where
> > >> drivers want to use other non-FB sources.
> > >>
> > >>>
> > >>>> In addition, loosen the NULL FB checks within the atomic commit callstack
> > >>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
> > >>>> methods where the FB was previously assumed to be non-NULL.
> > >>>>
> > >>>> Finally, have the DPU driver use drm_plane_state.color_fill and
> > >>>> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
> > >>>> and add extra checks in the DPU atomic commit callstack to account for a
> > >>>> NULL FB in cases where color_fill is set.
> > >>>>
> > >>>> Some drivers support hardware that have optimizations for solid fill
> > >>>> planes. This series aims to expose these capabilities to userspace as
> > >>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> > >>>> hardware composer HAL) that can be set by apps like the Android Gears
> > >>>> app.
> > >>>>
> > >>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
> > >>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
> > >>>> framebuffer to NULL.
> > >>>
> > >>> Is there some real reason for the format property? Ie. why not
> > >>> just do what was the plan for the crttc background color and
> > >>> specify the color in full 16bpc format and just pick as many
> > >>> msbs from that as the hw can use?
> > >>
> > >> The format property was added because we can't assume that all hardware
> > >> will support/use the same color format for solid fill planes. Even for
> > >> just MSM devices, the hardware supports different variations of RGB
> > >> formats [1].
> > >
> > > Sure, but the driver can convert the format into whatever the hw
> > > wants. A 1x1 color conversion is not going to be problematic ;-)
> >
> > Hi Rob,
> >
> > Hm... that's also a fair point. Just wondering, is there any advantage
> > of having the driver convert the format, other than not having to
> > implement an extra format property?
> >
> > (In case we end up wrapping everything into a prop blob or something)
> >
>
> It keeps the uabi simpler.. for obvious reasons you don't want the
> driver to do cpu color conversion for an arbitrary size plane, which
> is why we go to all the complexity to expose formats and modifiers for
> "real" planes, but we are dealing with a single pixel value here,
> let's not make the uabi more complex than we need to. I'd propose
> making it float32[4] if float weren't a pita for kernel/uabi, but
> u16[4] or u32[4] should be fine, and drivers can translate that easily
> into whatever weird formats their hw wants for solid-fill.
u16[4] fits into a single u64 property value.
That was the plan for the background prop as well:
https://lore.kernel.org/all/20190703125442.GW5942@intel.com/T/
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-10-29 11:23 ` Dmitry Baryshkov
2022-10-31 21:58 ` Jessica Zhang
@ 2022-11-08 18:25 ` Simon Ser
2022-11-09 13:52 ` Daniel Vetter
1 sibling, 1 reply; 48+ messages in thread
From: Simon Ser @ 2022-11-08 18:25 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, daniel.vetter, Jessica Zhang, freedreno
On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On 29/10/2022 01:59, Jessica Zhang wrote:
>
> > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > drm_plane. In addition, add support for setting and getting the values
> > of these properties.
> >
> > COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> > represents the format of the color fill. Userspace can set enable solid
> > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > framebuffer to NULL.
>
> I suppose that COLOR_FILL should override framebuffer rather than
> requiring that FB is set to NULL. In other words, if color_filL_format
> is non-zero, it would make sense to ignore the FB. Then one can use the
> color_fill_format property to quickly switch between filled plane and
> FB-backed one.
That would be inconsistent with the rest of the KMS uAPI. For instance,
the kernel will error out if CRTC has active=0 but a connector is still
linked to the CRTC. IOW, the current uAPI errors out if the KMS state
is inconsistent.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-10-29 12:08 ` Dmitry Baryshkov
2022-10-31 22:24 ` Jessica Zhang
@ 2022-11-08 18:50 ` Simon Ser
2022-11-08 22:01 ` Sebastian Wick
1 sibling, 1 reply; 48+ messages in thread
From: Simon Ser @ 2022-11-08 18:50 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Pekka Paalanen, linux-arm-msm, quic_abhinavk, dri-devel, swboyd,
seanpaul, laurent.pinchart, daniel.vetter, Jessica Zhang,
wayland-devel, freedreno
cc'ing Pekka and wayland-devel for userspace devs feedback on the new uAPI.
On Saturday, October 29th, 2022 at 14:08, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On 29/10/2022 01:59, Jessica Zhang wrote:
> > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > drm_plane. In addition, add support for setting and getting the values
> > of these properties.
> >
> > COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> > represents the format of the color fill. Userspace can set enable solid
> > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > framebuffer to NULL.
> >
> > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>
> Planes report supported formats using the drm_mode_getplane(). You'd
> also need to tell userspace, which formats are supported for color fill.
> I don't think one supports e.g. YV12.
>
> A bit of generic comment for the discussion (this is an RFC anyway).
> Using color_fill/color_fill_format properties sounds simple, but this
> might be not generic enough. Limiting color_fill to 32 bits would
> prevent anybody from using floating point formats (e.g.
> DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
> e.g. using 64-bit for the color_fill value, but then this doesn't sound
> extensible too much.
>
> So, a question for other hardware maintainers. Do we have hardware that
> supports such 'color filled' planes? Do we want to support format
> modifiers for filling color/data? Because what I have in mind is closer
> to the blob structure, which can then be used for filling the plane:
>
> struct color_fill_blob {
> u32 pixel_format;
> u64 modifiers4];
> u32 pixel_data_size; // fixme: is this necessary?
> u8 pixel_data[];
> };
>
> And then... This sounds a lot like a custom framebuffer.
>
> So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
> flag to the framebuffers, which would e.g. mean that the FB gets stamped
> all over the plane. This would also save us from changing if (!fb)
> checks all over the drm core.
>
> Another approach might be using a format modifier instead of the FB flag.
>
> What do you think?
First off, we only need to represent the value of a single pixel here. So I'm
not quite following why we need format modifiers. Format modifiers describe how
pixels are laid out in memory. Since there's a single pixel described, this
is non-sensical to me, the format modifier is always LINEAR.
Then, I can understand why putting the pixel_format in there is tempting to
guarantee future extensibility, but it also adds complexity. For instance, how
does user-space figure out which formats can be used for COLOR_FILL? Can
user-space use any format supported by the plane? What does it mean for
multi-planar formats? Do we really want the kernel to have conversion logic for
all existing formats? Do we need to also add a new read-only blob prop to
indicate supported COLOR_FILL formats?
We've recently-ish standardized a new Wayland protocol [1] which has the same
purpose as this new kernel uAPI. The conclusion there was that using 32-bit
values for each channel (R, G, B, A) would be enough for almost all use-cases.
The driver can convert these high-precision values to what the hardware expects.
The only concern was about sending values outside of the [0.0, 1.0] range,
which may have HDR use-cases.
So, there are multiple ways to go about this. I can think of:
- Put "RGBA32" in the name of the prop, and if we ever need a different
color format, pick a different name.
- Define a struct with an enum of possible fill kinds:
#define FILL_COLOR_RGBA32 1
#define FILL_COLOR_F32 2
struct color_fill_blob { u32 kind; u8 data[]; };
- Define a struct with a version and RGBA values:
struct color_fill_blob { u32 version; u32 rgba[4]; };
If we need to add more formats later, or new metadata:
struct color_fill_blob2 { u32 version; /* new fields */ };
where version must be set to 2.
- Define a struct with a "pixel_format" prop, but force user-space to use a
fixed format for now. Later, if we need another format, add a new prop to
advertise supported formats.
- More complicated solutions, e.g. advertise the list of supported formats from
the start.
[1]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-08 18:50 ` Simon Ser
@ 2022-11-08 22:01 ` Sebastian Wick
2022-11-09 9:18 ` Pekka Paalanen
0 siblings, 1 reply; 48+ messages in thread
From: Sebastian Wick @ 2022-11-08 22:01 UTC (permalink / raw)
To: Simon Ser
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, Pekka Paalanen,
seanpaul, laurent.pinchart, daniel.vetter, Dmitry Baryshkov,
Jessica Zhang, wayland-devel, freedreno
On Tue, Nov 8, 2022 at 7:51 PM Simon Ser <contact@emersion.fr> wrote:
>
> cc'ing Pekka and wayland-devel for userspace devs feedback on the new uAPI.
>
> On Saturday, October 29th, 2022 at 14:08, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>
> > On 29/10/2022 01:59, Jessica Zhang wrote:
> > > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > > drm_plane. In addition, add support for setting and getting the values
> > > of these properties.
> > >
> > > COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> > > represents the format of the color fill. Userspace can set enable solid
> > > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > > framebuffer to NULL.
> > >
> > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >
> > Planes report supported formats using the drm_mode_getplane(). You'd
> > also need to tell userspace, which formats are supported for color fill.
> > I don't think one supports e.g. YV12.
> >
> > A bit of generic comment for the discussion (this is an RFC anyway).
> > Using color_fill/color_fill_format properties sounds simple, but this
> > might be not generic enough. Limiting color_fill to 32 bits would
> > prevent anybody from using floating point formats (e.g.
> > DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
> > e.g. using 64-bit for the color_fill value, but then this doesn't sound
> > extensible too much.
> >
> > So, a question for other hardware maintainers. Do we have hardware that
> > supports such 'color filled' planes? Do we want to support format
> > modifiers for filling color/data? Because what I have in mind is closer
> > to the blob structure, which can then be used for filling the plane:
> >
> > struct color_fill_blob {
> > u32 pixel_format;
> > u64 modifiers4];
> > u32 pixel_data_size; // fixme: is this necessary?
> > u8 pixel_data[];
> > };
> >
> > And then... This sounds a lot like a custom framebuffer.
> >
> > So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
> > flag to the framebuffers, which would e.g. mean that the FB gets stamped
> > all over the plane. This would also save us from changing if (!fb)
> > checks all over the drm core.
> >
> > Another approach might be using a format modifier instead of the FB flag.
> >
> > What do you think?
>
> First off, we only need to represent the value of a single pixel here. So I'm
> not quite following why we need format modifiers. Format modifiers describe how
> pixels are laid out in memory. Since there's a single pixel described, this
> is non-sensical to me, the format modifier is always LINEAR.
>
> Then, I can understand why putting the pixel_format in there is tempting to
> guarantee future extensibility, but it also adds complexity. For instance, how
> does user-space figure out which formats can be used for COLOR_FILL? Can
> user-space use any format supported by the plane? What does it mean for
> multi-planar formats? Do we really want the kernel to have conversion logic for
> all existing formats? Do we need to also add a new read-only blob prop to
> indicate supported COLOR_FILL formats?
>
> We've recently-ish standardized a new Wayland protocol [1] which has the same
> purpose as this new kernel uAPI. The conclusion there was that using 32-bit
> values for each channel (R, G, B, A) would be enough for almost all use-cases.
> The driver can convert these high-precision values to what the hardware expects.
> The only concern was about sending values outside of the [0.0, 1.0] range,
> which may have HDR use-cases.
>
> So, there are multiple ways to go about this. I can think of:
>
> - Put "RGBA32" in the name of the prop, and if we ever need a different
> color format, pick a different name.
> - Define a struct with an enum of possible fill kinds:
> #define FILL_COLOR_RGBA32 1
> #define FILL_COLOR_F32 2
> struct color_fill_blob { u32 kind; u8 data[]; };
> - Define a struct with a version and RGBA values:
> struct color_fill_blob { u32 version; u32 rgba[4]; };
> If we need to add more formats later, or new metadata:
> struct color_fill_blob2 { u32 version; /* new fields */ };
> where version must be set to 2.
> - Define a struct with a "pixel_format" prop, but force user-space to use a
> fixed format for now. Later, if we need another format, add a new prop to
> advertise supported formats.
> - More complicated solutions, e.g. advertise the list of supported formats from
> the start.
>
> [1]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
>
Agreeing with most of what you said here. However, what's the idea
behind a format anyway? The 4 values provided here are fed directly
into the color pipeline which seems to define the color channels it's
working on as RGBA (or doesn't define anything at all). The only
reason I can think of is that hardware might support only ingesting
values either in a format with high bit depth color channels and no
alpha or a format with low bit depth color but with alpha, so choosing
between the formats provides a real trade-off. Is that actually
something hardware might be restricted to or do they all just support
ingesting the color data with enough precision on every channel?
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-08 22:01 ` Sebastian Wick
@ 2022-11-09 9:18 ` Pekka Paalanen
2022-11-23 23:27 ` Jessica Zhang
0 siblings, 1 reply; 48+ messages in thread
From: Pekka Paalanen @ 2022-11-09 9:18 UTC (permalink / raw)
To: Jessica Zhang
Cc: Sebastian Wick, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart, linux-arm-msm, Dmitry Baryshkov,
wayland-devel, freedreno
[-- Attachment #1: Type: text/plain, Size: 9420 bytes --]
On Tue, 8 Nov 2022 23:01:47 +0100
Sebastian Wick <sebastian.wick@redhat.com> wrote:
> On Tue, Nov 8, 2022 at 7:51 PM Simon Ser <contact@emersion.fr> wrote:
> >
> > cc'ing Pekka and wayland-devel for userspace devs feedback on the new uAPI.
Hi all,
thanks! Comments below.
> >
> > On Saturday, October 29th, 2022 at 14:08, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> >
> > > On 29/10/2022 01:59, Jessica Zhang wrote:
> > > > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > > > drm_plane. In addition, add support for setting and getting the values
> > > > of these properties.
> > > >
> > > > COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> > > > represents the format of the color fill. Userspace can set enable solid
> > > > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > > > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > > > framebuffer to NULL.
> > > >
> > > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> > >
> > > Planes report supported formats using the drm_mode_getplane(). You'd
> > > also need to tell userspace, which formats are supported for color fill.
> > > I don't think one supports e.g. YV12.
> > >
> > > A bit of generic comment for the discussion (this is an RFC anyway).
> > > Using color_fill/color_fill_format properties sounds simple, but this
> > > might be not generic enough. Limiting color_fill to 32 bits would
> > > prevent anybody from using floating point formats (e.g.
> > > DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
> > > e.g. using 64-bit for the color_fill value, but then this doesn't sound
> > > extensible too much.
> > >
> > > So, a question for other hardware maintainers. Do we have hardware that
> > > supports such 'color filled' planes? Do we want to support format
> > > modifiers for filling color/data? Because what I have in mind is closer
> > > to the blob structure, which can then be used for filling the plane:
> > >
> > > struct color_fill_blob {
> > > u32 pixel_format;
> > > u64 modifiers4];
> > > u32 pixel_data_size; // fixme: is this necessary?
> > > u8 pixel_data[];
> > > };
> > >
> > > And then... This sounds a lot like a custom framebuffer.
> > >
> > > So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
> > > flag to the framebuffers, which would e.g. mean that the FB gets stamped
> > > all over the plane. This would also save us from changing if (!fb)
> > > checks all over the drm core.
> > >
> > > Another approach might be using a format modifier instead of the FB flag.
> > >
> > > What do you think?
> >
> > First off, we only need to represent the value of a single pixel here. So I'm
> > not quite following why we need format modifiers. Format modifiers describe how
> > pixels are laid out in memory. Since there's a single pixel described, this
> > is non-sensical to me, the format modifier is always LINEAR.
Agreed.
> >
> > Then, I can understand why putting the pixel_format in there is tempting to
> > guarantee future extensibility, but it also adds complexity. For instance, how
> > does user-space figure out which formats can be used for COLOR_FILL? Can
> > user-space use any format supported by the plane? What does it mean for
> > multi-planar formats? Do we really want the kernel to have conversion logic for
> > all existing formats? Do we need to also add a new read-only blob prop to
> > indicate supported COLOR_FILL formats?
Right. This does not seem to require pixel formats at all.
The point of pixel formats is to be able to feed large amounts of data
as-is into hardware and avoid the CPU ever touching it. You do that
with DRM FBs pointing to suitably allocated hardware buffers. But here
we have exactly one pixel, which I imagine will always be read by the
CPU so the driver will convert it into a hardware-specific format and
program it; probably the driver will not create an internal DRM FB for
it.
The above might also be a reason to not model this as a special-case
DRM FB in UAPI. Or, at least you need a whole new ioctl to create such
DRM FB to avoid the need to allocate e.g. a dumb buffer or a
GPU-specific buffer.
What one does need is what Sebastian brought up: does it support alpha
or not?
Userspace would also be interested in the supported precision of the
values, but the hardware pixel component order is irrelevant because the
driver will always convert the one pixel with CPU anyway.
YUV vs. RGB is a another question. The KMS color pipeline is defined in
terms of RGBA as far as I know, and alpha-blending YUV values makes no
sense. So will there ever be any need to set an YUV fill? I have a hard
time imagining it.
If you do set an YUV fill, the KMS color pipeline most likely needs to
convert it to RGBA for blending, and then you need the plane properties
COLOR_ENCODING and COLOR_RANGE.
But why bother when userspace can convert that one pixel to RGBA itself
anyway?
> > We've recently-ish standardized a new Wayland protocol [1] which has the same
> > purpose as this new kernel uAPI. The conclusion there was that using 32-bit
> > values for each channel (R, G, B, A) would be enough for almost all use-cases.
> > The driver can convert these high-precision values to what the hardware expects.
> > The only concern was about sending values outside of the [0.0, 1.0] range,
> > which may have HDR use-cases.
This is what I would suggest, yes. This representation has enough
precision to be future-proof, and the driver will be converting the
value anyway.
The question about values outside of the unit range is a good one, too.
With Wayland, we can simply add another request to set a value in
floating-point if that turns up necessary.
Whether that will ever be necessary is connected to how the DRM KMS
abstract color pipeline is modelled, and that you must define from the
beginning:
If DRM KMS gets color processing related plane properties like CTM,
GAMMA or DEGAMMA (they already exist for CRTC, and these have been
proposed for planes quite some time ago), does the fill color go
through all these operations, or will the fill color skip all these
operations and go straight to plane blending?
Whether values outside of the unit range will ever be needed depends
also on the userspace design. Userspace could choose the value range
freely if the KMS color pipeline elements happen to support that range.
However things like LUTs are naturally limited to unit range input, so
using values outside of the unit range might be difficult if not
impossible. Therefore I'm not concerned about this question much.
> > So, there are multiple ways to go about this. I can think of:
> >
> > - Put "RGBA32" in the name of the prop, and if we ever need a different
> > color format, pick a different name.
I could see problems with that if the same device supports more than
one kind. How to turn off all but one color fill property?
What if userspace understands an old color fill property but not the
new one, and the new one happens to be set for some reason? Userspace
will attempt to use the old property without setting the new property
to nil, and fail.
> > - Define a struct with an enum of possible fill kinds:
> > #define FILL_COLOR_RGBA32 1
> > #define FILL_COLOR_F32 2
> > struct color_fill_blob { u32 kind; u8 data[]; };
This could work.
Btw. maybe call it RGBA32323232 to make it follow the drm_fourcc naming
convention. Some userspace libraries already use RGBA32 to mean 32 bit
per pixel instead of per channel.
How will userspace know what kinds are supported?
> > - Define a struct with a version and RGBA values:
> > struct color_fill_blob { u32 version; u32 rgba[4]; };
> > If we need to add more formats later, or new metadata:
> > struct color_fill_blob2 { u32 version; /* new fields */ };
> > where version must be set to 2.
This could work.
> > - Define a struct with a "pixel_format" prop, but force user-space to use a
> > fixed format for now. Later, if we need another format, add a new prop to
> > advertise supported formats.
> > - More complicated solutions, e.g. advertise the list of supported formats from
> > the start.
Feels more complicated than necessary.
Anyway, the idea of creating a blob and then setting that into some KMS
plane property sounds a very good mechanism.
> >
> > [1]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
> >
>
> Agreeing with most of what you said here. However, what's the idea
> behind a format anyway? The 4 values provided here are fed directly
> into the color pipeline which seems to define the color channels it's
> working on as RGBA (or doesn't define anything at all). The only
> reason I can think of is that hardware might support only ingesting
> values either in a format with high bit depth color channels and no
> alpha or a format with low bit depth color but with alpha, so choosing
> between the formats provides a real trade-off. Is that actually
> something hardware might be restricted to or do they all just support
> ingesting the color data with enough precision on every channel?
Right.
Thanks,
pq
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-08 18:25 ` Simon Ser
@ 2022-11-09 13:52 ` Daniel Vetter
2022-11-09 13:53 ` Dmitry Baryshkov
0 siblings, 1 reply; 48+ messages in thread
From: Daniel Vetter @ 2022-11-09 13:52 UTC (permalink / raw)
To: Simon Ser
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, daniel.vetter, Dmitry Baryshkov, Jessica Zhang,
freedreno
On Tue, Nov 08, 2022 at 06:25:29PM +0000, Simon Ser wrote:
> On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>
> > On 29/10/2022 01:59, Jessica Zhang wrote:
> >
> > > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > > drm_plane. In addition, add support for setting and getting the values
> > > of these properties.
> > >
> > > COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> > > represents the format of the color fill. Userspace can set enable solid
> > > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > > framebuffer to NULL.
> >
> > I suppose that COLOR_FILL should override framebuffer rather than
> > requiring that FB is set to NULL. In other words, if color_filL_format
> > is non-zero, it would make sense to ignore the FB. Then one can use the
> > color_fill_format property to quickly switch between filled plane and
> > FB-backed one.
>
> That would be inconsistent with the rest of the KMS uAPI. For instance,
> the kernel will error out if CRTC has active=0 but a connector is still
> linked to the CRTC. IOW, the current uAPI errors out if the KMS state
> is inconsistent.
So if the use-case here really is to solid-fill a plane (and not just
provide a background color for the crtc overall), then I guess we could
also extend addfb to make that happen. We've talked in the past about
propertery-fying framebuffer objects, and that would sort out this uapi
wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at least.
But if the use-cases are all background color then just doing the crtc
background color would be tons simpler (and likely also easier to support
for more hardware).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-09 13:52 ` Daniel Vetter
@ 2022-11-09 13:53 ` Dmitry Baryshkov
2022-11-09 13:59 ` Daniel Vetter
0 siblings, 1 reply; 48+ messages in thread
From: Dmitry Baryshkov @ 2022-11-09 13:53 UTC (permalink / raw)
To: Daniel Vetter, Simon Ser
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, daniel.vetter, Jessica Zhang, freedreno
On 09/11/2022 16:52, Daniel Vetter wrote:
> On Tue, Nov 08, 2022 at 06:25:29PM +0000, Simon Ser wrote:
>> On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>>
>>> On 29/10/2022 01:59, Jessica Zhang wrote:
>>>
>>>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
>>>> drm_plane. In addition, add support for setting and getting the values
>>>> of these properties.
>>>>
>>>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
>>>> represents the format of the color fill. Userspace can set enable solid
>>>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
>>>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
>>>> framebuffer to NULL.
>>>
>>> I suppose that COLOR_FILL should override framebuffer rather than
>>> requiring that FB is set to NULL. In other words, if color_filL_format
>>> is non-zero, it would make sense to ignore the FB. Then one can use the
>>> color_fill_format property to quickly switch between filled plane and
>>> FB-backed one.
>>
>> That would be inconsistent with the rest of the KMS uAPI. For instance,
>> the kernel will error out if CRTC has active=0 but a connector is still
>> linked to the CRTC. IOW, the current uAPI errors out if the KMS state
>> is inconsistent.
>
> So if the use-case here really is to solid-fill a plane (and not just
> provide a background color for the crtc overall), then I guess we could
> also extend addfb to make that happen. We've talked in the past about
> propertery-fying framebuffer objects, and that would sort out this uapi
> wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at least.
>
> But if the use-cases are all background color then just doing the crtc
> background color would be tons simpler (and likely also easier to support
> for more hardware).
No. The hardware supports multiple color-filled planes, which do not
have to cover the whole CRTC.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-09 13:53 ` Dmitry Baryshkov
@ 2022-11-09 13:59 ` Daniel Vetter
2022-11-10 1:44 ` Jessica Zhang
0 siblings, 1 reply; 48+ messages in thread
From: Daniel Vetter @ 2022-11-09 13:59 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: quic_abhinavk, dri-devel, swboyd, daniel.vetter, seanpaul,
laurent.pinchart, linux-arm-msm, Jessica Zhang, freedreno
On Wed, Nov 09, 2022 at 04:53:45PM +0300, Dmitry Baryshkov wrote:
> On 09/11/2022 16:52, Daniel Vetter wrote:
> > On Tue, Nov 08, 2022 at 06:25:29PM +0000, Simon Ser wrote:
> > > On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > > On 29/10/2022 01:59, Jessica Zhang wrote:
> > > >
> > > > > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > > > > drm_plane. In addition, add support for setting and getting the values
> > > > > of these properties.
> > > > >
> > > > > COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> > > > > represents the format of the color fill. Userspace can set enable solid
> > > > > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > > > > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > > > > framebuffer to NULL.
> > > >
> > > > I suppose that COLOR_FILL should override framebuffer rather than
> > > > requiring that FB is set to NULL. In other words, if color_filL_format
> > > > is non-zero, it would make sense to ignore the FB. Then one can use the
> > > > color_fill_format property to quickly switch between filled plane and
> > > > FB-backed one.
> > >
> > > That would be inconsistent with the rest of the KMS uAPI. For instance,
> > > the kernel will error out if CRTC has active=0 but a connector is still
> > > linked to the CRTC. IOW, the current uAPI errors out if the KMS state
> > > is inconsistent.
> >
> > So if the use-case here really is to solid-fill a plane (and not just
> > provide a background color for the crtc overall), then I guess we could
> > also extend addfb to make that happen. We've talked in the past about
> > propertery-fying framebuffer objects, and that would sort out this uapi
> > wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at least.
> >
> > But if the use-cases are all background color then just doing the crtc
> > background color would be tons simpler (and likely also easier to support
> > for more hardware).
>
> No. The hardware supports multiple color-filled planes, which do not have to
> cover the whole CRTC.
The use case here means the userspace use-case. What the hw can do on any
given chip kinda doesnt matter, which is why I'm asking. KMD uapi is not
meant to reflect 100% exactly what a specific chip can do, but instead:
- provide features userspace actually needs. If you want per-plane fill,
you need userspace that makes use of per-plane fill, and if all you have
is crtc background, then that's it.
- we should create uapi with an eye towards what's actually possible on a
reasonable set of drivers and hw. Sometimes that means a slightly more
restricted set so that it's possible to implement in more places,
especially if that restricted feature set still gets the job done for
userspace.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-09 13:59 ` Daniel Vetter
@ 2022-11-10 1:44 ` Jessica Zhang
2022-11-11 9:45 ` Daniel Vetter
0 siblings, 1 reply; 48+ messages in thread
From: Jessica Zhang @ 2022-11-10 1:44 UTC (permalink / raw)
To: Daniel Vetter, Dmitry Baryshkov
Cc: quic_abhinavk, dri-devel, swboyd, daniel.vetter, seanpaul,
laurent.pinchart, linux-arm-msm, freedreno
On 11/9/2022 5:59 AM, Daniel Vetter wrote:
> On Wed, Nov 09, 2022 at 04:53:45PM +0300, Dmitry Baryshkov wrote:
>> On 09/11/2022 16:52, Daniel Vetter wrote:
>>> On Tue, Nov 08, 2022 at 06:25:29PM +0000, Simon Ser wrote:
>>>> On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>>>>
>>>>> On 29/10/2022 01:59, Jessica Zhang wrote:
>>>>>
>>>>>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
>>>>>> drm_plane. In addition, add support for setting and getting the values
>>>>>> of these properties.
>>>>>>
>>>>>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
>>>>>> represents the format of the color fill. Userspace can set enable solid
>>>>>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
>>>>>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
>>>>>> framebuffer to NULL.
>>>>>
>>>>> I suppose that COLOR_FILL should override framebuffer rather than
>>>>> requiring that FB is set to NULL. In other words, if color_filL_format
>>>>> is non-zero, it would make sense to ignore the FB. Then one can use the
>>>>> color_fill_format property to quickly switch between filled plane and
>>>>> FB-backed one.
>>>>
>>>> That would be inconsistent with the rest of the KMS uAPI. For instance,
>>>> the kernel will error out if CRTC has active=0 but a connector is still
>>>> linked to the CRTC. IOW, the current uAPI errors out if the KMS state
>>>> is inconsistent.
>>>
>>> So if the use-case here really is to solid-fill a plane (and not just
>>> provide a background color for the crtc overall), then I guess we could
>>> also extend addfb to make that happen. We've talked in the past about
>>> propertery-fying framebuffer objects, and that would sort out this uapi
>>> wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at least.
>>>
>>> But if the use-cases are all background color then just doing the crtc
>>> background color would be tons simpler (and likely also easier to support
>>> for more hardware).
>>
>> No. The hardware supports multiple color-filled planes, which do not have to
>> cover the whole CRTC.
>
> The use case here means the userspace use-case. What the hw can do on any
> given chip kinda doesnt matter, which is why I'm asking. KMD uapi is not
> meant to reflect 100% exactly what a specific chip can do, but instead:
> - provide features userspace actually needs. If you want per-plane fill,
> you need userspace that makes use of per-plane fill, and if all you have
> is crtc background, then that's it.
Hey Daniel,
The userspace use case we're trying to support is the Android HWC
SOLID_FILL hint here [1], which is specifying per-plane fill.
Thanks,
Jessica Zhang
[1]
https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52
> - we should create uapi with an eye towards what's actually possible on a
> reasonable set of drivers and hw. Sometimes that means a slightly more
> restricted set so that it's possible to implement in more places,
> especially if that restricted feature set still gets the job done for
> userspace.
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-10 1:44 ` Jessica Zhang
@ 2022-11-11 9:45 ` Daniel Vetter
2022-11-18 19:15 ` Jessica Zhang
0 siblings, 1 reply; 48+ messages in thread
From: Daniel Vetter @ 2022-11-11 9:45 UTC (permalink / raw)
To: Jessica Zhang
Cc: quic_abhinavk, dri-devel, swboyd, daniel.vetter, seanpaul,
laurent.pinchart, linux-arm-msm, Dmitry Baryshkov, freedreno
On Wed, Nov 09, 2022 at 05:44:37PM -0800, Jessica Zhang wrote:
>
>
> On 11/9/2022 5:59 AM, Daniel Vetter wrote:
> > On Wed, Nov 09, 2022 at 04:53:45PM +0300, Dmitry Baryshkov wrote:
> > > On 09/11/2022 16:52, Daniel Vetter wrote:
> > > > On Tue, Nov 08, 2022 at 06:25:29PM +0000, Simon Ser wrote:
> > > > > On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > > On 29/10/2022 01:59, Jessica Zhang wrote:
> > > > > >
> > > > > > > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > > > > > > drm_plane. In addition, add support for setting and getting the values
> > > > > > > of these properties.
> > > > > > >
> > > > > > > COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> > > > > > > represents the format of the color fill. Userspace can set enable solid
> > > > > > > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > > > > > > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > > > > > > framebuffer to NULL.
> > > > > >
> > > > > > I suppose that COLOR_FILL should override framebuffer rather than
> > > > > > requiring that FB is set to NULL. In other words, if color_filL_format
> > > > > > is non-zero, it would make sense to ignore the FB. Then one can use the
> > > > > > color_fill_format property to quickly switch between filled plane and
> > > > > > FB-backed one.
> > > > >
> > > > > That would be inconsistent with the rest of the KMS uAPI. For instance,
> > > > > the kernel will error out if CRTC has active=0 but a connector is still
> > > > > linked to the CRTC. IOW, the current uAPI errors out if the KMS state
> > > > > is inconsistent.
> > > >
> > > > So if the use-case here really is to solid-fill a plane (and not just
> > > > provide a background color for the crtc overall), then I guess we could
> > > > also extend addfb to make that happen. We've talked in the past about
> > > > propertery-fying framebuffer objects, and that would sort out this uapi
> > > > wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at least.
> > > >
> > > > But if the use-cases are all background color then just doing the crtc
> > > > background color would be tons simpler (and likely also easier to support
> > > > for more hardware).
> > >
> > > No. The hardware supports multiple color-filled planes, which do not have to
> > > cover the whole CRTC.
> >
> > The use case here means the userspace use-case. What the hw can do on any
> > given chip kinda doesnt matter, which is why I'm asking. KMD uapi is not
> > meant to reflect 100% exactly what a specific chip can do, but instead:
> > - provide features userspace actually needs. If you want per-plane fill,
> > you need userspace that makes use of per-plane fill, and if all you have
> > is crtc background, then that's it.
>
> Hey Daniel,
>
> The userspace use case we're trying to support is the Android HWC SOLID_FILL
> hint here [1], which is specifying per-plane fill.
Does surfaceflinger actually use this for more than background fills? Yes
I'm annoying, but if we can simplify the kernel driver implementation
burden by asking compositors to do the math and simplify things, then I
think we should.
We also need an open source implementation for this that works and is
tested end-to-end. There's the drm_hwc project, but last time I've checked
there's really not much happpening there unfortunately.
-Daniel
>
> Thanks,
>
> Jessica Zhang
>
> [1] https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52
>
> > - we should create uapi with an eye towards what's actually possible on a
> > reasonable set of drivers and hw. Sometimes that means a slightly more
> > restricted set so that it's possible to implement in more places,
> > especially if that restricted feature set still gets the job done for
> > userspace.
> >
> > Cheers, Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-11 9:45 ` Daniel Vetter
@ 2022-11-18 19:15 ` Jessica Zhang
0 siblings, 0 replies; 48+ messages in thread
From: Jessica Zhang @ 2022-11-18 19:15 UTC (permalink / raw)
To: Daniel Vetter
Cc: quic_abhinavk, dri-devel, swboyd, daniel.vetter, seanpaul,
laurent.pinchart, linux-arm-msm, Dmitry Baryshkov, freedreno
On 11/11/2022 1:45 AM, Daniel Vetter wrote:
> On Wed, Nov 09, 2022 at 05:44:37PM -0800, Jessica Zhang wrote:
>>
>>
>> On 11/9/2022 5:59 AM, Daniel Vetter wrote:
>>> On Wed, Nov 09, 2022 at 04:53:45PM +0300, Dmitry Baryshkov wrote:
>>>> On 09/11/2022 16:52, Daniel Vetter wrote:
>>>>> On Tue, Nov 08, 2022 at 06:25:29PM +0000, Simon Ser wrote:
>>>>>> On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>>>>>>
>>>>>>> On 29/10/2022 01:59, Jessica Zhang wrote:
>>>>>>>
>>>>>>>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
>>>>>>>> drm_plane. In addition, add support for setting and getting the values
>>>>>>>> of these properties.
>>>>>>>>
>>>>>>>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
>>>>>>>> represents the format of the color fill. Userspace can set enable solid
>>>>>>>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
>>>>>>>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
>>>>>>>> framebuffer to NULL.
>>>>>>>
>>>>>>> I suppose that COLOR_FILL should override framebuffer rather than
>>>>>>> requiring that FB is set to NULL. In other words, if color_filL_format
>>>>>>> is non-zero, it would make sense to ignore the FB. Then one can use the
>>>>>>> color_fill_format property to quickly switch between filled plane and
>>>>>>> FB-backed one.
>>>>>>
>>>>>> That would be inconsistent with the rest of the KMS uAPI. For instance,
>>>>>> the kernel will error out if CRTC has active=0 but a connector is still
>>>>>> linked to the CRTC. IOW, the current uAPI errors out if the KMS state
>>>>>> is inconsistent.
>>>>>
>>>>> So if the use-case here really is to solid-fill a plane (and not just
>>>>> provide a background color for the crtc overall), then I guess we could
>>>>> also extend addfb to make that happen. We've talked in the past about
>>>>> propertery-fying framebuffer objects, and that would sort out this uapi
>>>>> wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at least.
>>>>>
>>>>> But if the use-cases are all background color then just doing the crtc
>>>>> background color would be tons simpler (and likely also easier to support
>>>>> for more hardware).
>>>>
>>>> No. The hardware supports multiple color-filled planes, which do not have to
>>>> cover the whole CRTC.
>>>
>>> The use case here means the userspace use-case. What the hw can do on any
>>> given chip kinda doesnt matter, which is why I'm asking. KMD uapi is not
>>> meant to reflect 100% exactly what a specific chip can do, but instead:
>>> - provide features userspace actually needs. If you want per-plane fill,
>>> you need userspace that makes use of per-plane fill, and if all you have
>>> is crtc background, then that's it.
>>
>> Hey Daniel,
>>
>> The userspace use case we're trying to support is the Android HWC SOLID_FILL
>> hint here [1], which is specifying per-plane fill.
>
> Does surfaceflinger actually use this for more than background fills? Yes
> I'm annoying, but if we can simplify the kernel driver implementation
> burden by asking compositors to do the math and simplify things, then I
> think we should.
AFAIK surfaceflinger allows apps to use this for cases beyond just
background fill -- an app, for example, can pass the hint for a plane
that only partially covers a screen and the driver would be expected to
fill just that ROI.
>
> We also need an open source implementation for this that works and is
> tested end-to-end. There's the drm_hwc project, but last time I've checked
> there's really not much happpening there unfortunately.
FWIW, Simon mentioned in a separate reply that Wayland supports a 1x1 FB
support protocol [1] for a similar purpose as this RFC series. I can
also create an IGT test meanwhile showing an example of E2E usage.
Thanks,
Jessica
[1]
https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
> -Daniel
>
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>> [1] https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52
>>
>>> - we should create uapi with an eye towards what's actually possible on a
>>> reasonable set of drivers and hw. Sometimes that means a slightly more
>>> restricted set so that it's possible to implement in more places,
>>> especially if that restricted feature set still gets the job done for
>>> userspace.
>>>
>>> Cheers, Daniel
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
2022-11-08 8:52 ` Ville Syrjälä
@ 2022-11-22 23:20 ` Jessica Zhang
0 siblings, 0 replies; 48+ messages in thread
From: Jessica Zhang @ 2022-11-22 23:20 UTC (permalink / raw)
To: Ville Syrjälä, Rob Clark
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, dmitry.baryshkov, freedreno
On 11/8/2022 12:52 AM, Ville Syrjälä wrote:
> On Mon, Nov 07, 2022 at 07:34:43PM -0800, Rob Clark wrote:
>> On Mon, Nov 7, 2022 at 4:22 PM Jessica Zhang <quic_jesszhan@quicinc.com>wrote:
>>>
>>>
>>>
>>> On 11/7/2022 2:09 PM, Rob Clark wrote:
>>>> On Mon, Nov 7, 2022 at 1:32 PM Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
>>>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>>>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>>>>>>> properties. When the color fill value is set, and the framebuffer is set
>>>>>>> to NULL, memory fetch will be disabled.
>>>>>>
>>>>>> Thinking a bit more universally I wonder if there should be
>>>>>> some kind of enum property:
>>>>>>
>>>>>> enum plane_pixel_source {
>>>>>> FB,
>>>>>> COLOR,
>>>>>> LIVE_FOO,
>>>>>> LIVE_BAR,
>>>>>> ...
>>>>>> }
>>>>>
>>>>> Hi Ville,
>>>>>
>>>>> Makes sense -- this way, we'd also lay some groundwork for cases where
>>>>> drivers want to use other non-FB sources.
>>>>>
>>>>>>
>>>>>>> In addition, loosen the NULL FB checks within the atomic commit callstack
>>>>>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>>>>>>> methods where the FB was previously assumed to be non-NULL.
>>>>>>>
>>>>>>> Finally, have the DPU driver use drm_plane_state.color_fill and
>>>>>>> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
>>>>>>> and add extra checks in the DPU atomic commit callstack to account for a
>>>>>>> NULL FB in cases where color_fill is set.
>>>>>>>
>>>>>>> Some drivers support hardware that have optimizations for solid fill
>>>>>>> planes. This series aims to expose these capabilities to userspace as
>>>>>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
>>>>>>> hardware composer HAL) that can be set by apps like the Android Gears
>>>>>>> app.
>>>>>>>
>>>>>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMATto a
>>>>>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
>>>>>>> framebuffer to NULL.
>>>>>>
>>>>>> Is there some real reason for the format property? Ie. why not
>>>>>> just do what was the plan for the crttc background color and
>>>>>> specify the color in full 16bpc format and just pick as many
>>>>>> msbs from that as the hw can use?
>>>>>
>>>>> The format property was added because we can't assume that all hardware
>>>>> will support/use the same color format for solid fill planes. Even for
>>>>> just MSM devices, the hardware supports different variations of RGB
>>>>> formats [1].
>>>>
>>>> Sure, but the driver can convert the format into whatever the hw
>>>> wants. A 1x1 color conversion is not going to be problematic ;-)
>>>
>>> Hi Rob,
>>>
>>> Hm... that's also a fair point. Just wondering, is there any advantage
>>> of having the driver convert the format, other than not having to
>>> implement an extra format property?
>>>
>>> (In case we end up wrapping everything into a prop blob or something)
>>>
>>
>> It keeps the uabi simpler.. for obvious reasons you don't want the
>> driver to do cpu color conversion for an arbitrary size plane, which
>> is why we go to all the complexity to expose formats and modifiers for
>> "real" planes, but we are dealing with a single pixel value here,
>> let's not make the uabi more complex than we need to. I'd propose
>> making it float32[4] if float weren't a pita for kernel/uabi, but
>> u16[4] or u32[4] should be fine, and drivers can translate that easily
>> into whatever weird formats their hw wants for solid-fill.
>
> u16[4] fits into a single u64 property value.
>
> That was the plan for the background prop as well:
> https://lore.kernel.org/all/20190703125442.GW5942@intel.com/T/
Got it, I think that's pretty reasonable then. Will probably use u32[4]
instead since that is what Pekka and Simon are recommending to match
Wayland's single-buffer protocol [1].
Thanks,
Jessica Zhang
[1]
https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
>
> --
> Ville Syrjälä
> Intel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-09 9:18 ` Pekka Paalanen
@ 2022-11-23 23:27 ` Jessica Zhang
2022-11-24 8:50 ` Pekka Paalanen
0 siblings, 1 reply; 48+ messages in thread
From: Jessica Zhang @ 2022-11-23 23:27 UTC (permalink / raw)
To: Pekka Paalanen
Cc: Sebastian Wick, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart, linux-arm-msm, Dmitry Baryshkov,
wayland-devel, freedreno
On 11/9/2022 1:18 AM, Pekka Paalanen wrote:
> On Tue, 8 Nov 2022 23:01:47 +0100
> Sebastian Wick <sebastian.wick@redhat.com> wrote:
>
>> On Tue, Nov 8, 2022 at 7:51 PM Simon Ser <contact@emersion.fr> wrote:
>>>
>>> cc'ing Pekka and wayland-devel for userspace devs feedback on the new uAPI.
>
> Hi all,
>
> thanks! Comments below.
Thanks for the feedback!
>
>>>
>>> On Saturday, October 29th, 2022 at 14:08, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>>>
>>>> On 29/10/2022 01:59, Jessica Zhang wrote:
>>>>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
>>>>> drm_plane. In addition, add support for setting and getting the values
>>>>> of these properties.
>>>>>
>>>>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
>>>>> represents the format of the color fill. Userspace can set enable solid
>>>>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
>>>>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
>>>>> framebuffer to NULL.
>>>>>
>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>
>>>> Planes report supported formats using the drm_mode_getplane(). You'd
>>>> also need to tell userspace, which formats are supported for color fill.
>>>> I don't think one supports e.g. YV12.
>>>>
>>>> A bit of generic comment for the discussion (this is an RFC anyway).
>>>> Using color_fill/color_fill_format properties sounds simple, but this
>>>> might be not generic enough. Limiting color_fill to 32 bits would
>>>> prevent anybody from using floating point formats (e.g.
>>>> DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
>>>> e.g. using 64-bit for the color_fill value, but then this doesn't sound
>>>> extensible too much.
>>>>
>>>> So, a question for other hardware maintainers. Do we have hardware that
>>>> supports such 'color filled' planes? Do we want to support format
>>>> modifiers for filling color/data? Because what I have in mind is closer
>>>> to the blob structure, which can then be used for filling the plane:
>>>>
>>>> struct color_fill_blob {
>>>> u32 pixel_format;
>>>> u64 modifiers4];
>>>> u32 pixel_data_size; // fixme: is this necessary?
>>>> u8 pixel_data[];
>>>> };
>>>>
>>>> And then... This sounds a lot like a custom framebuffer.
>>>>
>>>> So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
>>>> flag to the framebuffers, which would e.g. mean that the FB gets stamped
>>>> all over the plane. This would also save us from changing if (!fb)
>>>> checks all over the drm core.
>>>>
>>>> Another approach might be using a format modifier instead of the FB flag.
>>>>
>>>> What do you think?
>>>
>>> First off, we only need to represent the value of a single pixel here. So I'm
>>> not quite following why we need format modifiers. Format modifiers describe how
>>> pixels are laid out in memory. Since there's a single pixel described, this
>>> is non-sensical to me, the format modifier is always LINEAR.
>
> Agreed.
>
>>>
>>> Then, I can understand why putting the pixel_format in there is tempting to
>>> guarantee future extensibility, but it also adds complexity. For instance, how
>>> does user-space figure out which formats can be used for COLOR_FILL? Can
>>> user-space use any format supported by the plane? What does it mean for
>>> multi-planar formats? Do we really want the kernel to have conversion logic for
>>> all existing formats? Do we need to also add a new read-only blob prop to
>>> indicate supported COLOR_FILL formats?
FWIW the formats supported by solid_fill wouldn't necessarily be all the
formats supported by the plane (ex. for msm/dpu, solid_fill only
supports all RGB color variants, though planes can normally support YUV
formats too).
That being said, I'm ok with having the solid_fill take in only
RGBA32323232 format based on the comments below.
>
> Right. This does not seem to require pixel formats at all.
>
> The point of pixel formats is to be able to feed large amounts of data
> as-is into hardware and avoid the CPU ever touching it. You do that
> with DRM FBs pointing to suitably allocated hardware buffers. But here
> we have exactly one pixel, which I imagine will always be read by the
> CPU so the driver will convert it into a hardware-specific format and
> program it; probably the driver will not create an internal DRM FB for
> it. >
> The above might also be a reason to not model this as a special-case
> DRM FB in UAPI. Or, at least you need a whole new ioctl to create such
> DRM FB to avoid the need to allocate e.g. a dumb buffer or a
> GPU-specific buffer. >
> What one does need is what Sebastian brought up: does it support alpha
> or not?
Hmm, the drm_plane struct already supports an alpha property so it seems
a bit redundant to also have a separate alpha value in the solid fill color.
That being said, we could have it so that setting the alpha for the
solid_fill property will also change the value of the plane's alpha
property too.
>
> Userspace would also be interested in the supported precision of the
> values, but the hardware pixel component order is irrelevant because the
> driver will always convert the one pixel with CPU anyway.
>
> YUV vs. RGB is a another question. The KMS color pipeline is defined in
> terms of RGBA as far as I know, and alpha-blending YUV values makes no
> sense. So will there ever be any need to set an YUV fill? I have a hard
> time imagining it.
>
> If you do set an YUV fill, the KMS color pipeline most likely needs to
> convert it to RGBA for blending, and then you need the plane properties
> COLOR_ENCODING and COLOR_RANGE.
>
> But why bother when userspace can convert that one pixel to RGBA itself
> anyway?
Noted, I think this is reasonable.
>
>>> We've recently-ish standardized a new Wayland protocol [1] which has the same
>>> purpose as this new kernel uAPI. The conclusion there was that using 32-bit
>>> values for each channel (R, G, B, A) would be enough for almost all use-cases.
>>> The driver can convert these high-precision values to what the hardware expects.
>>> The only concern was about sending values outside of the [0.0, 1.0] range,
>>> which may have HDR use-cases.
>
> This is what I would suggest, yes. This representation has enough
> precision to be future-proof, and the driver will be converting the
> value anyway.
>
> The question about values outside of the unit range is a good one, too.
> With Wayland, we can simply add another request to set a value in
> floating-point if that turns up necessary.
>
> Whether that will ever be necessary is connected to how the DRM KMS
> abstract color pipeline is modelled, and that you must define from the
> beginning:
>
> If DRM KMS gets color processing related plane properties like CTM,
> GAMMA or DEGAMMA (they already exist for CRTC, and these have been
> proposed for planes quite some time ago), does the fill color go
> through all these operations, or will the fill color skip all these
> operations and go straight to plane blending?
The fill color would still go through color processing operations,
though FWIW the MSM driver doesn't support GAMMA/DEGAMMA.
>
> Whether values outside of the unit range will ever be needed depends
> also on the userspace design. Userspace could choose the value range
> freely if the KMS color pipeline elements happen to support that range.
> However things like LUTs are naturally limited to unit range input, so
> using values outside of the unit range might be difficult if not
> impossible. Therefore I'm not concerned about this question much.
>
>>> So, there are multiple ways to go about this. I can think of:
>>>
>>> - Put "RGBA32" in the name of the prop, and if we ever need a different
>>> color format, pick a different name.
>
> I could see problems with that if the same device supports more than
> one kind. How to turn off all but one color fill property?
>
> What if userspace understands an old color fill property but not the
> new one, and the new one happens to be set for some reason? Userspace
> will attempt to use the old property without setting the new property
> to nil, and fail.
>
>>> - Define a struct with an enum of possible fill kinds:
>>> #define FILL_COLOR_RGBA32 1
>>> #define FILL_COLOR_F32 2
>>> struct color_fill_blob { u32 kind; u8 data[]; };
>
> This could work.
>
> Btw. maybe call it RGBA32323232 to make it follow the drm_fourcc naming
> convention. Some userspace libraries already use RGBA32 to mean 32 bit
> per pixel instead of per channel.
>
> How will userspace know what kinds are supported?
>
>>> - Define a struct with a version and RGBA values:
>>> struct color_fill_blob { u32 version; u32 rgba[4]; };
>>> If we need to add more formats later, or new metadata:
>>> struct color_fill_blob2 { u32 version; /* new fields */ };
>>> where version must be set to 2.
>
> This could work.
Leaning towards this option.
Thanks,
Jessica Zhang
>
>>> - Define a struct with a "pixel_format" prop, but force user-space to use a
>>> fixed format for now. Later, if we need another format, add a new prop to
>>> advertise supported formats.
>>> - More complicated solutions, e.g. advertise the list of supported formats from
>>> the start.
>
> Feels more complicated than necessary.
>
> Anyway, the idea of creating a blob and then setting that into some KMS
> plane property sounds a very good mechanism.
>
>>>
>>> [1]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
>>>
>>
>> Agreeing with most of what you said here. However, what's the idea
>> behind a format anyway? The 4 values provided here are fed directly
>> into the color pipeline which seems to define the color channels it's
>> working on as RGBA (or doesn't define anything at all). The only
>> reason I can think of is that hardware might support only ingesting
>> values either in a format with high bit depth color channels and no
>> alpha or a format with low bit depth color but with alpha, so choosing
>> between the formats provides a real trade-off. Is that actually
>> something hardware might be restricted to or do they all just support
>> ingesting the color data with enough precision on every channel? >
> Right.
>
>
> Thanks,
> pq
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-23 23:27 ` Jessica Zhang
@ 2022-11-24 8:50 ` Pekka Paalanen
2022-11-29 18:53 ` [Freedreno] " Jessica Zhang
0 siblings, 1 reply; 48+ messages in thread
From: Pekka Paalanen @ 2022-11-24 8:50 UTC (permalink / raw)
To: Jessica Zhang
Cc: Sebastian Wick, quic_abhinavk, dri-devel, swboyd, daniel.vetter,
seanpaul, laurent.pinchart, linux-arm-msm, Dmitry Baryshkov,
wayland-devel, freedreno
[-- Attachment #1: Type: text/plain, Size: 11709 bytes --]
On Wed, 23 Nov 2022 15:27:04 -0800
Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> On 11/9/2022 1:18 AM, Pekka Paalanen wrote:
> > On Tue, 8 Nov 2022 23:01:47 +0100
> > Sebastian Wick <sebastian.wick@redhat.com> wrote:
> >
> >> On Tue, Nov 8, 2022 at 7:51 PM Simon Ser <contact@emersion.fr> wrote:
> >>>
> >>> cc'ing Pekka and wayland-devel for userspace devs feedback on the new uAPI.
> >
> > Hi all,
> >
> > thanks! Comments below.
>
> Thanks for the feedback!
>
> >
> >>>
> >>> On Saturday, October 29th, 2022 at 14:08, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> >>>
> >>>> On 29/10/2022 01:59, Jessica Zhang wrote:
> >>>>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> >>>>> drm_plane. In addition, add support for setting and getting the values
> >>>>> of these properties.
> >>>>>
> >>>>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> >>>>> represents the format of the color fill. Userspace can set enable solid
> >>>>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> >>>>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> >>>>> framebuffer to NULL.
> >>>>>
> >>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> >>>>
> >>>> Planes report supported formats using the drm_mode_getplane(). You'd
> >>>> also need to tell userspace, which formats are supported for color fill.
> >>>> I don't think one supports e.g. YV12.
> >>>>
> >>>> A bit of generic comment for the discussion (this is an RFC anyway).
> >>>> Using color_fill/color_fill_format properties sounds simple, but this
> >>>> might be not generic enough. Limiting color_fill to 32 bits would
> >>>> prevent anybody from using floating point formats (e.g.
> >>>> DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
> >>>> e.g. using 64-bit for the color_fill value, but then this doesn't sound
> >>>> extensible too much.
> >>>>
> >>>> So, a question for other hardware maintainers. Do we have hardware that
> >>>> supports such 'color filled' planes? Do we want to support format
> >>>> modifiers for filling color/data? Because what I have in mind is closer
> >>>> to the blob structure, which can then be used for filling the plane:
> >>>>
> >>>> struct color_fill_blob {
> >>>> u32 pixel_format;
> >>>> u64 modifiers4];
> >>>> u32 pixel_data_size; // fixme: is this necessary?
> >>>> u8 pixel_data[];
> >>>> };
> >>>>
> >>>> And then... This sounds a lot like a custom framebuffer.
> >>>>
> >>>> So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
> >>>> flag to the framebuffers, which would e.g. mean that the FB gets stamped
> >>>> all over the plane. This would also save us from changing if (!fb)
> >>>> checks all over the drm core.
> >>>>
> >>>> Another approach might be using a format modifier instead of the FB flag.
> >>>>
> >>>> What do you think?
> >>>
> >>> First off, we only need to represent the value of a single pixel here. So I'm
> >>> not quite following why we need format modifiers. Format modifiers describe how
> >>> pixels are laid out in memory. Since there's a single pixel described, this
> >>> is non-sensical to me, the format modifier is always LINEAR.
> >
> > Agreed.
> >
> >>>
> >>> Then, I can understand why putting the pixel_format in there is tempting to
> >>> guarantee future extensibility, but it also adds complexity. For instance, how
> >>> does user-space figure out which formats can be used for COLOR_FILL? Can
> >>> user-space use any format supported by the plane? What does it mean for
> >>> multi-planar formats? Do we really want the kernel to have conversion logic for
> >>> all existing formats? Do we need to also add a new read-only blob prop to
> >>> indicate supported COLOR_FILL formats?
>
> FWIW the formats supported by solid_fill wouldn't necessarily be all the
> formats supported by the plane (ex. for msm/dpu, solid_fill only
> supports all RGB color variants, though planes can normally support YUV
> formats too).
>
> That being said, I'm ok with having the solid_fill take in only
> RGBA32323232 format based on the comments below.
>
> >
> > Right. This does not seem to require pixel formats at all.
> >
> > The point of pixel formats is to be able to feed large amounts of data
> > as-is into hardware and avoid the CPU ever touching it. You do that
> > with DRM FBs pointing to suitably allocated hardware buffers. But here
> > we have exactly one pixel, which I imagine will always be read by the
> > CPU so the driver will convert it into a hardware-specific format and
> > program it; probably the driver will not create an internal DRM FB for
> > it. >
> > The above might also be a reason to not model this as a special-case
> > DRM FB in UAPI. Or, at least you need a whole new ioctl to create such
> > DRM FB to avoid the need to allocate e.g. a dumb buffer or a
> > GPU-specific buffer. >
> > What one does need is what Sebastian brought up: does it support alpha
> > or not?
> Hmm, the drm_plane struct already supports an alpha property so it seems
> a bit redundant to also have a separate alpha value in the solid fill color.
Hi Jessica,
that's a good point! - Assuming that if hardware supports fill with
alpha, it supports plane-alpha with real FBs as well.
> That being said, we could have it so that setting the alpha for the
> solid_fill property will also change the value of the plane's alpha
> property too.
No! Definitely not. That would be confusing.
One must not have properties that change the value of other
non-immutable properties. It would become a real mess to handle in
userspace and for backward compatibility. Just like the kernel must not
spontaneously change the value of a non-immutable property. (Some
mistakes exist already, and I think they cause userspace to need
exceptional code for them.)
> >
> > Userspace would also be interested in the supported precision of the
> > values, but the hardware pixel component order is irrelevant because the
> > driver will always convert the one pixel with CPU anyway.
> >
> > YUV vs. RGB is a another question. The KMS color pipeline is defined in
> > terms of RGBA as far as I know, and alpha-blending YUV values makes no
> > sense. So will there ever be any need to set an YUV fill? I have a hard
> > time imagining it.
> >
> > If you do set an YUV fill, the KMS color pipeline most likely needs to
> > convert it to RGBA for blending, and then you need the plane properties
> > COLOR_ENCODING and COLOR_RANGE.
> >
> > But why bother when userspace can convert that one pixel to RGBA itself
> > anyway?
>
> Noted, I think this is reasonable.
>
> >
> >>> We've recently-ish standardized a new Wayland protocol [1] which has the same
> >>> purpose as this new kernel uAPI. The conclusion there was that using 32-bit
> >>> values for each channel (R, G, B, A) would be enough for almost all use-cases.
> >>> The driver can convert these high-precision values to what the hardware expects.
> >>> The only concern was about sending values outside of the [0.0, 1.0] range,
> >>> which may have HDR use-cases.
> >
> > This is what I would suggest, yes. This representation has enough
> > precision to be future-proof, and the driver will be converting the
> > value anyway.
> >
> > The question about values outside of the unit range is a good one, too.
> > With Wayland, we can simply add another request to set a value in
> > floating-point if that turns up necessary.
> >
> > Whether that will ever be necessary is connected to how the DRM KMS
> > abstract color pipeline is modelled, and that you must define from the
> > beginning:
> >
> > If DRM KMS gets color processing related plane properties like CTM,
> > GAMMA or DEGAMMA (they already exist for CRTC, and these have been
> > proposed for planes quite some time ago), does the fill color go
> > through all these operations, or will the fill color skip all these
> > operations and go straight to plane blending?
>
> The fill color would still go through color processing operations,
> though FWIW the MSM driver doesn't support GAMMA/DEGAMMA.
That's ok. The important bit is to define what must happen *if* such
plane properties are exposed by a driver. If they are not exposed, no
problem.
Btw. I could easily expect disagreement between different hardware
here, so I think this part will need many eyes to review.
If hardware is hard-wired to feed the fill color straight to blending,
then if fill color UAPI is defined to go through per-plane color
processing, the driver needs to apply that color processing on the CPU
before programming the hardware.
If hardware allows processing the fill color through per-plane color
processing, but fill color UAPI is defined to feed straight to blending,
then the driver can simply ignore the per-plane color properties and
program pass-through to the hardware.
For userspace, I think the choice makes little difference. Userspace
can compensate for the choice the same way a driver would, except
userspace can perhaps use more precise calculation methods. OTOH, if
fill color is intended to match the color on a real FB on another
plane, not going through the exact same computations might cause error.
Whether that error is significant depends on the use case and is
impossible to say here and now.
The important bit is to make that choice and document it.
...
> >>> - Define a struct with a version and RGBA values:
> >>> struct color_fill_blob { u32 version; u32 rgba[4]; };
> >>> If we need to add more formats later, or new metadata:
> >>> struct color_fill_blob2 { u32 version; /* new fields */ };
> >>> where version must be set to 2.
> >
> > This could work.
>
> Leaning towards this option.
Yes, it seems the best to me too. Just rgb[3] rather than rgba[4],
given the discussion about the plane alpha property. Or even u32 r;
u32 g; u32 b; to avoid having to think about the index in code.
Thanks,
pq
>
> Thanks,
>
> Jessica Zhang
>
> >
> >>> - Define a struct with a "pixel_format" prop, but force user-space to use a
> >>> fixed format for now. Later, if we need another format, add a new prop to
> >>> advertise supported formats.
> >>> - More complicated solutions, e.g. advertise the list of supported formats from
> >>> the start.
> >
> > Feels more complicated than necessary.
> >
> > Anyway, the idea of creating a blob and then setting that into some KMS
> > plane property sounds a very good mechanism.
> >
> >>>
> >>> [1]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
> >>>
> >>
> >> Agreeing with most of what you said here. However, what's the idea
> >> behind a format anyway? The 4 values provided here are fed directly
> >> into the color pipeline which seems to define the color channels it's
> >> working on as RGBA (or doesn't define anything at all). The only
> >> reason I can think of is that hardware might support only ingesting
> >> values either in a format with high bit depth color channels and no
> >> alpha or a format with low bit depth color but with alpha, so choosing
> >> between the formats provides a real trade-off. Is that actually
> >> something hardware might be restricted to or do they all just support
> >> ingesting the color data with enough precision on every channel? >
> > Right.
> >
> >
> > Thanks,
> > pq
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
2022-11-24 8:50 ` Pekka Paalanen
@ 2022-11-29 18:53 ` Jessica Zhang
0 siblings, 0 replies; 48+ messages in thread
From: Jessica Zhang @ 2022-11-29 18:53 UTC (permalink / raw)
To: Pekka Paalanen
Cc: Sebastian Wick, daniel.vetter, linux-arm-msm, quic_abhinavk,
dri-devel, swboyd, seanpaul, laurent.pinchart, Dmitry Baryshkov,
wayland-devel, freedreno
On 11/24/2022 12:50 AM, Pekka Paalanen wrote:
> On Wed, 23 Nov 2022 15:27:04 -0800
> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>> On 11/9/2022 1:18 AM, Pekka Paalanen wrote:
>>> On Tue, 8 Nov 2022 23:01:47 +0100
>>> Sebastian Wick <sebastian.wick@redhat.com> wrote:
>>>
>>>> On Tue, Nov 8, 2022 at 7:51 PM Simon Ser <contact@emersion.fr> wrote:
>>>>>
>>>>> cc'ing Pekka and wayland-devel for userspace devs feedback on the new uAPI.
>>>
>>> Hi all,
>>>
>>> thanks! Comments below.
>>
>> Thanks for the feedback!
>>
>>>
>>>>>
>>>>> On Saturday, October 29th, 2022 at 14:08, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>>>>>
>>>>>> On 29/10/2022 01:59, Jessica Zhang wrote:
>>>>>>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
>>>>>>> drm_plane. In addition, add support for setting and getting the values
>>>>>>> of these properties.
>>>>>>>
>>>>>>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
>>>>>>> represents the format of the color fill. Userspace can set enable solid
>>>>>>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
>>>>>>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
>>>>>>> framebuffer to NULL.
>>>>>>>
>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>>>>>
>>>>>> Planes report supported formats using the drm_mode_getplane(). You'd
>>>>>> also need to tell userspace, which formats are supported for color fill.
>>>>>> I don't think one supports e.g. YV12.
>>>>>>
>>>>>> A bit of generic comment for the discussion (this is an RFC anyway).
>>>>>> Using color_fill/color_fill_format properties sounds simple, but this
>>>>>> might be not generic enough. Limiting color_fill to 32 bits would
>>>>>> prevent anybody from using floating point formats (e.g.
>>>>>> DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
>>>>>> e.g. using 64-bit for the color_fill value, but then this doesn't sound
>>>>>> extensible too much.
>>>>>>
>>>>>> So, a question for other hardware maintainers. Do we have hardware that
>>>>>> supports such 'color filled' planes? Do we want to support format
>>>>>> modifiers for filling color/data? Because what I have in mind is closer
>>>>>> to the blob structure, which can then be used for filling the plane:
>>>>>>
>>>>>> struct color_fill_blob {
>>>>>> u32 pixel_format;
>>>>>> u64 modifiers4];
>>>>>> u32 pixel_data_size; // fixme: is this necessary?
>>>>>> u8 pixel_data[];
>>>>>> };
>>>>>>
>>>>>> And then... This sounds a lot like a custom framebuffer.
>>>>>>
>>>>>> So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
>>>>>> flag to the framebuffers, which would e.g. mean that the FB gets stamped
>>>>>> all over the plane. This would also save us from changing if (!fb)
>>>>>> checks all over the drm core.
>>>>>>
>>>>>> Another approach might be using a format modifier instead of the FB flag.
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> First off, we only need to represent the value of a single pixel here. So I'm
>>>>> not quite following why we need format modifiers. Format modifiers describe how
>>>>> pixels are laid out in memory. Since there's a single pixel described, this
>>>>> is non-sensical to me, the format modifier is always LINEAR.
>>>
>>> Agreed.
>>>
>>>>>
>>>>> Then, I can understand why putting the pixel_format in there is tempting to
>>>>> guarantee future extensibility, but it also adds complexity. For instance, how
>>>>> does user-space figure out which formats can be used for COLOR_FILL? Can
>>>>> user-space use any format supported by the plane? What does it mean for
>>>>> multi-planar formats? Do we really want the kernel to have conversion logic for
>>>>> all existing formats? Do we need to also add a new read-only blob prop to
>>>>> indicate supported COLOR_FILL formats?
>>
>> FWIW the formats supported by solid_fill wouldn't necessarily be all the
>> formats supported by the plane (ex. for msm/dpu, solid_fill only
>> supports all RGB color variants, though planes can normally support YUV
>> formats too).
>>
>> That being said, I'm ok with having the solid_fill take in only
>> RGBA32323232 format based on the comments below.
>>
>>>
>>> Right. This does not seem to require pixel formats at all.
>>>
>>> The point of pixel formats is to be able to feed large amounts of data
>>> as-is into hardware and avoid the CPU ever touching it. You do that
>>> with DRM FBs pointing to suitably allocated hardware buffers. But here
>>> we have exactly one pixel, which I imagine will always be read by the
>>> CPU so the driver will convert it into a hardware-specific format and
>>> program it; probably the driver will not create an internal DRM FB for
>>> it. >
>>> The above might also be a reason to not model this as a special-case
>>> DRM FB in UAPI. Or, at least you need a whole new ioctl to create such
>>> DRM FB to avoid the need to allocate e.g. a dumb buffer or a
>>> GPU-specific buffer. >
>>> What one does need is what Sebastian brought up: does it support alpha
>>> or not?
>> Hmm, the drm_plane struct already supports an alpha property so it seems
>> a bit redundant to also have a separate alpha value in the solid fill color.
>
> Hi Jessica,
>
> that's a good point! - Assuming that if hardware supports fill with
> alpha, it supports plane-alpha with real FBs as well.
>
>> That being said, we could have it so that setting the alpha for the
>> solid_fill property will also change the value of the plane's alpha
>> property too.
>
> No! Definitely not. That would be confusing.
>
> One must not have properties that change the value of other
> non-immutable properties. It would become a real mess to handle in
> userspace and for backward compatibility. Just like the kernel must not
> spontaneously change the value of a non-immutable property. (Some
> mistakes exist already, and I think they cause userspace to need
> exceptional code for them.)
Ah, got it -- will have the value be RGB323232 instead.
>
>
>>>
>>> Userspace would also be interested in the supported precision of the
>>> values, but the hardware pixel component order is irrelevant because the
>>> driver will always convert the one pixel with CPU anyway.
>>>
>>> YUV vs. RGB is a another question. The KMS color pipeline is defined in
>>> terms of RGBA as far as I know, and alpha-blending YUV values makes no
>>> sense. So will there ever be any need to set an YUV fill? I have a hard
>>> time imagining it.
>>>
>>> If you do set an YUV fill, the KMS color pipeline most likely needs to
>>> convert it to RGBA for blending, and then you need the plane properties
>>> COLOR_ENCODING and COLOR_RANGE.
>>>
>>> But why bother when userspace can convert that one pixel to RGBA itself
>>> anyway?
>>
>> Noted, I think this is reasonable.
>>
>>>
>>>>> We've recently-ish standardized a new Wayland protocol [1] which has the same
>>>>> purpose as this new kernel uAPI. The conclusion there was that using 32-bit
>>>>> values for each channel (R, G, B, A) would be enough for almost all use-cases.
>>>>> The driver can convert these high-precision values to what the hardware expects.
>>>>> The only concern was about sending values outside of the [0.0, 1.0] range,
>>>>> which may have HDR use-cases.
>>>
>>> This is what I would suggest, yes. This representation has enough
>>> precision to be future-proof, and the driver will be converting the
>>> value anyway.
>>>
>>> The question about values outside of the unit range is a good one, too.
>>> With Wayland, we can simply add another request to set a value in
>>> floating-point if that turns up necessary.
>>>
>>> Whether that will ever be necessary is connected to how the DRM KMS
>>> abstract color pipeline is modelled, and that you must define from the
>>> beginning:
>>>
>>> If DRM KMS gets color processing related plane properties like CTM,
>>> GAMMA or DEGAMMA (they already exist for CRTC, and these have been
>>> proposed for planes quite some time ago), does the fill color go
>>> through all these operations, or will the fill color skip all these
>>> operations and go straight to plane blending?
>>
>> The fill color would still go through color processing operations,
>> though FWIW the MSM driver doesn't support GAMMA/DEGAMMA.
>
> That's ok. The important bit is to define what must happen *if* such
> plane properties are exposed by a driver. If they are not exposed, no
> problem.
>
> Btw. I could easily expect disagreement between different hardware
> here, so I think this part will need many eyes to review.
Got it -- I'm not aware of any other HW outside of MSM devices that
supports a similar color fill feature, but if there are any that have
something similar I'm open to learning about how they've implemented
this feature and adjusting accordingly.
>
> If hardware is hard-wired to feed the fill color straight to blending,
> then if fill color UAPI is defined to go through per-plane color
> processing, the driver needs to apply that color processing on the CPU
> before programming the hardware.
>
> If hardware allows processing the fill color through per-plane color
> processing, but fill color UAPI is defined to feed straight to blending,
> then the driver can simply ignore the per-plane color properties and
> program pass-through to the hardware.
>
> For userspace, I think the choice makes little difference. Userspace
> can compensate for the choice the same way a driver would, except
> userspace can perhaps use more precise calculation methods. OTOH, if
> fill color is intended to match the color on a real FB on another
> plane, not going through the exact same computations might cause error.
> Whether that error is significant depends on the use case and is
> impossible to say here and now.
>
> The important bit is to make that choice and document it.
Acked.
Thanks,
Jessica Zhang
>
> ...
>
>>>>> - Define a struct with a version and RGBA values:
>>>>> struct color_fill_blob { u32 version; u32 rgba[4]; };
>>>>> If we need to add more formats later, or new metadata:
>>>>> struct color_fill_blob2 { u32 version; /* new fields */ };
>>>>> where version must be set to 2.
>>>
>>> This could work.
>>
>> Leaning towards this option.
>
> Yes, it seems the best to me too. Just rgb[3] rather than rgba[4],
> given the discussion about the plane alpha property. Or even u32 r;
> u32 g; u32 b; to avoid having to think about the index in code.
>
>
> Thanks,
> pq
>
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>>> - Define a struct with a "pixel_format" prop, but force user-space to use a
>>>>> fixed format for now. Later, if we need another format, add a new prop to
>>>>> advertise supported formats.
>>>>> - More complicated solutions, e.g. advertise the list of supported formats from
>>>>> the start.
>>>
>>> Feels more complicated than necessary.
>>>
>>> Anyway, the idea of creating a blob and then setting that into some KMS
>>> plane property sounds a very good mechanism.
>>>
>>>>>
>>>>> [1]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
>>>>>
>>>>
>>>> Agreeing with most of what you said here. However, what's the idea
>>>> behind a format anyway? The 4 values provided here are fed directly
>>>> into the color pipeline which seems to define the color channels it's
>>>> working on as RGBA (or doesn't define anything at all). The only
>>>> reason I can think of is that hardware might support only ingesting
>>>> values either in a format with high bit depth color channels and no
>>>> alpha or a format with low bit depth color but with alpha, so choosing
>>>> between the formats provides a real trade-off. Is that actually
>>>> something hardware might be restricted to or do they all just support
>>>> ingesting the color data with enough precision on every channel? >
>>> Right.
>>>
>>>
>>> Thanks,
>>> pq
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2022-11-07 19:37 ` [RFC PATCH 0/3] Support for Solid Fill Planes Ville Syrjälä
2022-11-07 21:32 ` Jessica Zhang
2022-11-07 23:06 ` Laurent Pinchart
@ 2023-06-26 23:02 ` Jessica Zhang
2023-06-27 0:06 ` Dmitry Baryshkov
2023-06-27 7:58 ` Pekka Paalanen
2 siblings, 2 replies; 48+ messages in thread
From: Jessica Zhang @ 2023-06-26 23:02 UTC (permalink / raw)
To: Ville Syrjälä
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, dmitry.baryshkov, wayland-devel,
freedreno
On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>> properties. When the color fill value is set, and the framebuffer is set
>> to NULL, memory fetch will be disabled.
>
> Thinking a bit more universally I wonder if there should be
> some kind of enum property:
>
> enum plane_pixel_source {
> FB,
> COLOR,
> LIVE_FOO,
> LIVE_BAR,
> ...
> }
Reviving this thread as this was the initial comment suggesting to
implement pixel_source as an enum.
I think the issue with having pixel_source as an enum is how to decide
what counts as a NULL commit.
Currently, setting the FB to NULL will disable the plane. So I'm
guessing we will extend that logic to "if there's no pixel_source set
for the plane, then it will be a NULL commit and disable the plane".
In that case, the question then becomes when to set the pixel_source to
NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
blob), it then forces userspace to set one property before the other.
Because of that, I'm thinking of having pixel_source be represented by a
bitmask instead. That way, we will simply unset the corresponding
pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
order to detect whether a commit is NULL or has a valid pixel source, we
can just check if pixel_source == 0.
Would be interested in any feedback on this.
Thanks,
Jessica Zhang
>
>> In addition, loosen the NULL FB checks within the atomic commit callstack
>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>> methods where the FB was previously assumed to be non-NULL.
>>
>> Finally, have the DPU driver use drm_plane_state.color_fill and
>> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
>> and add extra checks in the DPU atomic commit callstack to account for a
>> NULL FB in cases where color_fill is set.
>>
>> Some drivers support hardware that have optimizations for solid fill
>> planes. This series aims to expose these capabilities to userspace as
>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
>> hardware composer HAL) that can be set by apps like the Android Gears
>> app.
>>
>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
>> DRM format, setting COLOR_FILL to a color fill value, and setting the
>> framebuffer to NULL.
>
> Is there some real reason for the format property? Ie. why not
> just do what was the plan for the crttc background color and
> specify the color in full 16bpc format and just pick as many
> msbs from that as the hw can use?
>
>>
>> Jessica Zhang (3):
>> drm: Introduce color fill properties for drm plane
>> drm: Adjust atomic checks for solid fill color
>> drm/msm/dpu: Use color_fill property for DPU planes
>>
>> drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++-----------
>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
>> drivers/gpu/drm/drm_plane.c | 8 +--
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
>> include/drm/drm_atomic_helper.h | 5 +-
>> include/drm/drm_blend.h | 2 +
>> include/drm/drm_plane.h | 28 ++++++++++
>> 10 files changed, 188 insertions(+), 76 deletions(-)
>>
>> --
>> 2.38.0
>
> --
> Ville Syrjälä
> Intel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-26 23:02 ` Jessica Zhang
@ 2023-06-27 0:06 ` Dmitry Baryshkov
2023-06-27 0:45 ` Jessica Zhang
2023-06-27 7:58 ` Pekka Paalanen
1 sibling, 1 reply; 48+ messages in thread
From: Dmitry Baryshkov @ 2023-06-27 0:06 UTC (permalink / raw)
To: Jessica Zhang, Ville Syrjälä
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, wayland-devel, freedreno
On 27/06/2023 02:02, Jessica Zhang wrote:
>
>
> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>>> properties. When the color fill value is set, and the framebuffer is set
>>> to NULL, memory fetch will be disabled.
>>
>> Thinking a bit more universally I wonder if there should be
>> some kind of enum property:
>>
>> enum plane_pixel_source {
>> FB,
>> COLOR,
>> LIVE_FOO,
>> LIVE_BAR,
>> ...
>> }
>
> Reviving this thread as this was the initial comment suggesting to
> implement pixel_source as an enum.
>
> I think the issue with having pixel_source as an enum is how to decide
> what counts as a NULL commit.
>
> Currently, setting the FB to NULL will disable the plane. So I'm
> guessing we will extend that logic to "if there's no pixel_source set
> for the plane, then it will be a NULL commit and disable the plane".
>
> In that case, the question then becomes when to set the pixel_source to
> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
> blob), it then forces userspace to set one property before the other.
Why? The userspace should use atomic commits and as such it should all
properties at the same time.
> Because of that, I'm thinking of having pixel_source be represented by a
> bitmask instead. That way, we will simply unset the corresponding
> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
> order to detect whether a commit is NULL or has a valid pixel source, we
> can just check if pixel_source == 0.
>
> Would be interested in any feedback on this.
This is an interesting idea. Frankly speaking, I'd consider it
counter-intuitive at the first glance.
Consider it to act as a curtain. Setup the curtain (by writing the fill
colour property). Then one can close the curtain (by switching source to
colour), or open it (by switching to any other source). Bitmask wouldn't
complicate this.
>
> Thanks,
>
> Jessica Zhang
>
>>
>>> In addition, loosen the NULL FB checks within the atomic commit
>>> callstack
>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>>> methods where the FB was previously assumed to be non-NULL.
>>>
>>> Finally, have the DPU driver use drm_plane_state.color_fill and
>>> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
>>> and add extra checks in the DPU atomic commit callstack to account for a
>>> NULL FB in cases where color_fill is set.
>>>
>>> Some drivers support hardware that have optimizations for solid fill
>>> planes. This series aims to expose these capabilities to userspace as
>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
>>> hardware composer HAL) that can be set by apps like the Android Gears
>>> app.
>>>
>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
>>> framebuffer to NULL.
>>
>> Is there some real reason for the format property? Ie. why not
>> just do what was the plan for the crttc background color and
>> specify the color in full 16bpc format and just pick as many
>> msbs from that as the hw can use?
>>
>>>
>>> Jessica Zhang (3):
>>> drm: Introduce color fill properties for drm plane
>>> drm: Adjust atomic checks for solid fill color
>>> drm/msm/dpu: Use color_fill property for DPU planes
>>>
>>> drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++-----------
>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
>>> drivers/gpu/drm/drm_plane.c | 8 +--
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
>>> include/drm/drm_atomic_helper.h | 5 +-
>>> include/drm/drm_blend.h | 2 +
>>> include/drm/drm_plane.h | 28 ++++++++++
>>> 10 files changed, 188 insertions(+), 76 deletions(-)
>>>
>>> --
>>> 2.38.0
>>
>> --
>> Ville Syrjälä
>> Intel
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-27 0:06 ` Dmitry Baryshkov
@ 2023-06-27 0:45 ` Jessica Zhang
2023-06-27 1:46 ` Dmitry Baryshkov
0 siblings, 1 reply; 48+ messages in thread
From: Jessica Zhang @ 2023-06-27 0:45 UTC (permalink / raw)
To: Dmitry Baryshkov, Ville Syrjälä
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, wayland-devel, freedreno
On 6/26/2023 5:06 PM, Dmitry Baryshkov wrote:
> On 27/06/2023 02:02, Jessica Zhang wrote:
>>
>>
>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>>>> properties. When the color fill value is set, and the framebuffer is
>>>> set
>>>> to NULL, memory fetch will be disabled.
>>>
>>> Thinking a bit more universally I wonder if there should be
>>> some kind of enum property:
>>>
>>> enum plane_pixel_source {
>>> FB,
>>> COLOR,
>>> LIVE_FOO,
>>> LIVE_BAR,
>>> ...
>>> }
>>
>> Reviving this thread as this was the initial comment suggesting to
>> implement pixel_source as an enum.
>>
>> I think the issue with having pixel_source as an enum is how to decide
>> what counts as a NULL commit.
>>
>> Currently, setting the FB to NULL will disable the plane. So I'm
>> guessing we will extend that logic to "if there's no pixel_source set
>> for the plane, then it will be a NULL commit and disable the plane".
>>
>> In that case, the question then becomes when to set the pixel_source
>> to NONE. Because if we do that when setting a NULL FB (or NULL
>> solid_fill blob), it then forces userspace to set one property before
>> the other.
>
> Why? The userspace should use atomic commits and as such it should all
> properties at the same time.
Correct, userspace will set all the properties within the same atomic
commit. The issue happens when the driver iterates through each property
within the MODE_ATOMIC ioctl [1].
For reference, I'm thinking of an implementation where we're setting the
pixel_source within drm_atomic_plane_set_property().
So something like:
drm_atomic_plane_set_property( ... )
{
if (property == config->prop_fb_id) {
if (fb)
state->pixel_source = FB;
else
state->pixel_source = NONE;
} else if (property == config->prop_solid_fill) {
if (solid_fill_blob)
state->pixel_source = SOLID_FILL;
}
// ...
}
If userspace sets solid_fill to a valid blob and FB to NULL, it's
possible for driver to first set the solid_fill property then set the
fb_id property later. This would cause pixel_source to be set to NONE
after all properties have been set.
I've also considered an implementation without the `pixel_source = NONE`
line in the prop_fb_id case, but we would still need to find somewhere
to set the pixel_source to NONE in order to allow userspace to disable a
plane.
[1]
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_uapi.c#L1385
>
>> Because of that, I'm thinking of having pixel_source be represented by
>> a bitmask instead. That way, we will simply unset the corresponding
>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
>> order to detect whether a commit is NULL or has a valid pixel source,
>> we can just check if pixel_source == 0.
>>
>> Would be interested in any feedback on this.
>
> This is an interesting idea. Frankly speaking, I'd consider it
> counter-intuitive at the first glance.
>
> Consider it to act as a curtain. Setup the curtain (by writing the fill
> colour property). Then one can close the curtain (by switching source to
> colour), or open it (by switching to any other source). Bitmask wouldn't
> complicate this.
So if I'm understanding your analogy correctly, pixel_source won't
necessarily be set whenever the FB or solid_fill properties are set. So
that way we can have both FB *and* solid_fill set at the same time, but
only the source that pixel_source is set to would be displayed.
Thanks,
Jessica Zhang
>
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>> In addition, loosen the NULL FB checks within the atomic commit
>>>> callstack
>>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>>>> methods where the FB was previously assumed to be non-NULL.
>>>>
>>>> Finally, have the DPU driver use drm_plane_state.color_fill and
>>>> drm_plane_state.color_fill_format instead of
>>>> dpu_plane_state.color_fill,
>>>> and add extra checks in the DPU atomic commit callstack to account
>>>> for a
>>>> NULL FB in cases where color_fill is set.
>>>>
>>>> Some drivers support hardware that have optimizations for solid fill
>>>> planes. This series aims to expose these capabilities to userspace as
>>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
>>>> hardware composer HAL) that can be set by apps like the Android Gears
>>>> app.
>>>>
>>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT
>>>> to a
>>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
>>>> framebuffer to NULL.
>>>
>>> Is there some real reason for the format property? Ie. why not
>>> just do what was the plan for the crttc background color and
>>> specify the color in full 16bpc format and just pick as many
>>> msbs from that as the hw can use?
>>>
>>>>
>>>> Jessica Zhang (3):
>>>> drm: Introduce color fill properties for drm plane
>>>> drm: Adjust atomic checks for solid fill color
>>>> drm/msm/dpu: Use color_fill property for DPU planes
>>>>
>>>> drivers/gpu/drm/drm_atomic.c | 68
>>>> ++++++++++++-----------
>>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
>>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
>>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
>>>> drivers/gpu/drm/drm_plane.c | 8 +--
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
>>>> include/drm/drm_atomic_helper.h | 5 +-
>>>> include/drm/drm_blend.h | 2 +
>>>> include/drm/drm_plane.h | 28 ++++++++++
>>>> 10 files changed, 188 insertions(+), 76 deletions(-)
>>>>
>>>> --
>>>> 2.38.0
>>>
>>> --
>>> Ville Syrjälä
>>> Intel
>
> --
> With best wishes
> Dmitry
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-27 0:45 ` Jessica Zhang
@ 2023-06-27 1:46 ` Dmitry Baryshkov
0 siblings, 0 replies; 48+ messages in thread
From: Dmitry Baryshkov @ 2023-06-27 1:46 UTC (permalink / raw)
To: Jessica Zhang
Cc: linux-arm-msm, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, daniel.vetter, wayland-devel, freedreno
On Tue, 27 Jun 2023 at 03:45, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>
> On 6/26/2023 5:06 PM, Dmitry Baryshkov wrote:
> > On 27/06/2023 02:02, Jessica Zhang wrote:
> >>
> >>
> >> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> >>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> >>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> >>>> properties. When the color fill value is set, and the framebuffer is
> >>>> set
> >>>> to NULL, memory fetch will be disabled.
> >>>
> >>> Thinking a bit more universally I wonder if there should be
> >>> some kind of enum property:
> >>>
> >>> enum plane_pixel_source {
> >>> FB,
> >>> COLOR,
> >>> LIVE_FOO,
> >>> LIVE_BAR,
> >>> ...
> >>> }
> >>
> >> Reviving this thread as this was the initial comment suggesting to
> >> implement pixel_source as an enum.
> >>
> >> I think the issue with having pixel_source as an enum is how to decide
> >> what counts as a NULL commit.
> >>
> >> Currently, setting the FB to NULL will disable the plane. So I'm
> >> guessing we will extend that logic to "if there's no pixel_source set
> >> for the plane, then it will be a NULL commit and disable the plane".
> >>
> >> In that case, the question then becomes when to set the pixel_source
> >> to NONE. Because if we do that when setting a NULL FB (or NULL
> >> solid_fill blob), it then forces userspace to set one property before
> >> the other.
> >
> > Why? The userspace should use atomic commits and as such it should all
> > properties at the same time.
>
> Correct, userspace will set all the properties within the same atomic
> commit. The issue happens when the driver iterates through each property
> within the MODE_ATOMIC ioctl [1].
>
> For reference, I'm thinking of an implementation where we're setting the
> pixel_source within drm_atomic_plane_set_property().
>
> So something like:
>
> drm_atomic_plane_set_property( ... )
> {
> if (property == config->prop_fb_id) {
> if (fb)
> state->pixel_source = FB;
> else
> state->pixel_source = NONE;
> } else if (property == config->prop_solid_fill) {
> if (solid_fill_blob)
> state->pixel_source = SOLID_FILL;
> }
>
> // ...
> }
I think this is somewhat overcomplicated. Allow userspace to set these
properties as it sees fit and then in
drm_atomic_helper_check_plane_state() consider all of them to set
plane_state->visible.
We still have to remain compatible with older userspace (esp. with a
non-atomic one). It expects that a plane is enabled after setting both
CRTC and FB. So maybe you are right and we should force pixel_source
to FB if FB is set.
>
> If userspace sets solid_fill to a valid blob and FB to NULL, it's
> possible for driver to first set the solid_fill property then set the
> fb_id property later. This would cause pixel_source to be set to NONE
> after all properties have been set.
>
> I've also considered an implementation without the `pixel_source = NONE`
> line in the prop_fb_id case, but we would still need to find somewhere
> to set the pixel_source to NONE in order to allow userspace to disable a
> plane.
Good point. I don't think we would need NONE (just setting CRTC to
none or FB to none and pixel_source to FB would disable the plane),
but I might be missing something here.
>
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_uapi.c#L1385
>
> >
> >> Because of that, I'm thinking of having pixel_source be represented by
> >> a bitmask instead. That way, we will simply unset the corresponding
> >> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
> >> order to detect whether a commit is NULL or has a valid pixel source,
> >> we can just check if pixel_source == 0.
> >>
> >> Would be interested in any feedback on this.
> >
> > This is an interesting idea. Frankly speaking, I'd consider it
> > counter-intuitive at the first glance.
> >
> > Consider it to act as a curtain. Setup the curtain (by writing the fill
> > colour property). Then one can close the curtain (by switching source to
> > colour), or open it (by switching to any other source). Bitmask wouldn't
> > complicate this.
>
> So if I'm understanding your analogy correctly, pixel_source won't
> necessarily be set whenever the FB or solid_fill properties are set. So
> that way we can have both FB *and* solid_fill set at the same time, but
> only the source that pixel_source is set to would be displayed.
Yes. And if the source is not configured, the plane will be marked as
not visible.
>
> Thanks,
>
> Jessica Zhang
>
> >
> >>
> >> Thanks,
> >>
> >> Jessica Zhang
> >>
> >>>
> >>>> In addition, loosen the NULL FB checks within the atomic commit
> >>>> callstack
> >>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
> >>>> methods where the FB was previously assumed to be non-NULL.
> >>>>
> >>>> Finally, have the DPU driver use drm_plane_state.color_fill and
> >>>> drm_plane_state.color_fill_format instead of
> >>>> dpu_plane_state.color_fill,
> >>>> and add extra checks in the DPU atomic commit callstack to account
> >>>> for a
> >>>> NULL FB in cases where color_fill is set.
> >>>>
> >>>> Some drivers support hardware that have optimizations for solid fill
> >>>> planes. This series aims to expose these capabilities to userspace as
> >>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> >>>> hardware composer HAL) that can be set by apps like the Android Gears
> >>>> app.
> >>>>
> >>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT
> >>>> to a
> >>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
> >>>> framebuffer to NULL.
> >>>
> >>> Is there some real reason for the format property? Ie. why not
> >>> just do what was the plan for the crttc background color and
> >>> specify the color in full 16bpc format and just pick as many
> >>> msbs from that as the hw can use?
> >>>
> >>>>
> >>>> Jessica Zhang (3):
> >>>> drm: Introduce color fill properties for drm plane
> >>>> drm: Adjust atomic checks for solid fill color
> >>>> drm/msm/dpu: Use color_fill property for DPU planes
> >>>>
> >>>> drivers/gpu/drm/drm_atomic.c | 68
> >>>> ++++++++++++-----------
> >>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
> >>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
> >>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
> >>>> drivers/gpu/drm/drm_plane.c | 8 +--
> >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
> >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
> >>>> include/drm/drm_atomic_helper.h | 5 +-
> >>>> include/drm/drm_blend.h | 2 +
> >>>> include/drm/drm_plane.h | 28 ++++++++++
> >>>> 10 files changed, 188 insertions(+), 76 deletions(-)
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-26 23:02 ` Jessica Zhang
2023-06-27 0:06 ` Dmitry Baryshkov
@ 2023-06-27 7:58 ` Pekka Paalanen
2023-06-27 21:27 ` Jessica Zhang
1 sibling, 1 reply; 48+ messages in thread
From: Pekka Paalanen @ 2023-06-27 7:58 UTC (permalink / raw)
To: Jessica Zhang
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, dmitry.baryshkov, wayland-devel,
freedreno
[-- Attachment #1: Type: text/plain, Size: 5932 bytes --]
On Mon, 26 Jun 2023 16:02:50 -0700
Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> > On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> >> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> >> properties. When the color fill value is set, and the framebuffer is set
> >> to NULL, memory fetch will be disabled.
> >
> > Thinking a bit more universally I wonder if there should be
> > some kind of enum property:
> >
> > enum plane_pixel_source {
> > FB,
> > COLOR,
> > LIVE_FOO,
> > LIVE_BAR,
> > ...
> > }
>
> Reviving this thread as this was the initial comment suggesting to
> implement pixel_source as an enum.
>
> I think the issue with having pixel_source as an enum is how to decide
> what counts as a NULL commit.
>
> Currently, setting the FB to NULL will disable the plane. So I'm
> guessing we will extend that logic to "if there's no pixel_source set
> for the plane, then it will be a NULL commit and disable the plane".
>
> In that case, the question then becomes when to set the pixel_source to
> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
> blob), it then forces userspace to set one property before the other.
Right, that won't work.
There is no ordering between each property being set inside a single
atomic commit. They can all be applied to kernel-internal state
theoretically simultaneously, or any arbitrary random order, and the
end result must always be the same. Hence, setting one property cannot
change the state of another mutable property. I believe that doing
otherwise would make userspace fragile and hard to get right.
I guess there might be an exception to that rule when the same property
is set multiple times in a single atomic commit; the last setting in
the array prevails. That's universal and not a special-case between two
specific properties.
> Because of that, I'm thinking of having pixel_source be represented by a
> bitmask instead. That way, we will simply unset the corresponding
> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
> order to detect whether a commit is NULL or has a valid pixel source, we
> can just check if pixel_source == 0.
Sounds fine to me at first hand, but isn't there the enum property that
says if the kernel must look at solid_fill blob *or* FB_ID?
If enum prop says "use solid_fill prop", the why would changes to FB_ID
do anything? Is it for backwards-compatibility with KMS clients that do
not know about the enum prop?
It seems like that kind of backwards-compatiblity will cause problems
in trying to reason about the atomic state, as explained above, leading
to very delicate and fragile conditions where things work intuitively.
Hence, I'm not sure backwards-compatibility is wanted. This won't be
the first or the last KMS property where an unexpected value left over
will make old atomic KMS clients silently malfunction up to showing no
recognisable picture at all. *If* that problem needs solving, there
have been ideas floating around about resetting everything to nice
values so that userspace can ignore what it does not understand. So far
there has been no real interest in solving that problem in the kernel
though.
Legacy non-atomic UAPI wrappers can do whatever they want, and program
any (new) properties they want in order to implement the legacy
expectations, so that does not seem to be a problem.
Thanks,
pq
>
> Would be interested in any feedback on this.
>
> Thanks,
>
> Jessica Zhang
>
> >
> >> In addition, loosen the NULL FB checks within the atomic commit callstack
> >> to allow a NULL FB when color_fill is nonzero and add FB checks in
> >> methods where the FB was previously assumed to be non-NULL.
> >>
> >> Finally, have the DPU driver use drm_plane_state.color_fill and
> >> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
> >> and add extra checks in the DPU atomic commit callstack to account for a
> >> NULL FB in cases where color_fill is set.
> >>
> >> Some drivers support hardware that have optimizations for solid fill
> >> planes. This series aims to expose these capabilities to userspace as
> >> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
> >> hardware composer HAL) that can be set by apps like the Android Gears
> >> app.
> >>
> >> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
> >> DRM format, setting COLOR_FILL to a color fill value, and setting the
> >> framebuffer to NULL.
> >
> > Is there some real reason for the format property? Ie. why not
> > just do what was the plan for the crttc background color and
> > specify the color in full 16bpc format and just pick as many
> > msbs from that as the hw can use?
> >
> >>
> >> Jessica Zhang (3):
> >> drm: Introduce color fill properties for drm plane
> >> drm: Adjust atomic checks for solid fill color
> >> drm/msm/dpu: Use color_fill property for DPU planes
> >>
> >> drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++-----------
> >> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
> >> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
> >> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
> >> drivers/gpu/drm/drm_plane.c | 8 +--
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
> >> include/drm/drm_atomic_helper.h | 5 +-
> >> include/drm/drm_blend.h | 2 +
> >> include/drm/drm_plane.h | 28 ++++++++++
> >> 10 files changed, 188 insertions(+), 76 deletions(-)
> >>
> >> --
> >> 2.38.0
> >
> > --
> > Ville Syrjälä
> > Intel
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-27 7:58 ` Pekka Paalanen
@ 2023-06-27 21:27 ` Jessica Zhang
2023-06-27 21:59 ` Dmitry Baryshkov
0 siblings, 1 reply; 48+ messages in thread
From: Jessica Zhang @ 2023-06-27 21:27 UTC (permalink / raw)
To: Pekka Paalanen, dmitry.baryshkov
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, wayland-devel, freedreno
On 6/27/2023 12:58 AM, Pekka Paalanen wrote:
> On Mon, 26 Jun 2023 16:02:50 -0700
> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>>>> properties. When the color fill value is set, and the framebuffer is set
>>>> to NULL, memory fetch will be disabled.
>>>
>>> Thinking a bit more universally I wonder if there should be
>>> some kind of enum property:
>>>
>>> enum plane_pixel_source {
>>> FB,
>>> COLOR,
>>> LIVE_FOO,
>>> LIVE_BAR,
>>> ...
>>> }
>>
>> Reviving this thread as this was the initial comment suggesting to
>> implement pixel_source as an enum.
>>
>> I think the issue with having pixel_source as an enum is how to decide
>> what counts as a NULL commit.
>>
>> Currently, setting the FB to NULL will disable the plane. So I'm
>> guessing we will extend that logic to "if there's no pixel_source set
>> for the plane, then it will be a NULL commit and disable the plane".
>>
>> In that case, the question then becomes when to set the pixel_source to
>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
>> blob), it then forces userspace to set one property before the other.
>
> Right, that won't work.
>
> There is no ordering between each property being set inside a single
> atomic commit. They can all be applied to kernel-internal state
> theoretically simultaneously, or any arbitrary random order, and the
> end result must always be the same. Hence, setting one property cannot
> change the state of another mutable property. I believe that doing
> otherwise would make userspace fragile and hard to get right.
>
> I guess there might be an exception to that rule when the same property
> is set multiple times in a single atomic commit; the last setting in
> the array prevails. That's universal and not a special-case between two
> specific properties.
>
>> Because of that, I'm thinking of having pixel_source be represented by a
>> bitmask instead. That way, we will simply unset the corresponding
>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
>> order to detect whether a commit is NULL or has a valid pixel source, we
>> can just check if pixel_source == 0.
>
> Sounds fine to me at first hand, but isn't there the enum property that
> says if the kernel must look at solid_fill blob *or* FB_ID?
>
> If enum prop says "use solid_fill prop", the why would changes to FB_ID
> do anything? Is it for backwards-compatibility with KMS clients that do
> not know about the enum prop?
>
> It seems like that kind of backwards-compatiblity will cause problems
> in trying to reason about the atomic state, as explained above, leading
> to very delicate and fragile conditions where things work intuitively.
> Hence, I'm not sure backwards-compatibility is wanted. This won't be
> the first or the last KMS property where an unexpected value left over
> will make old atomic KMS clients silently malfunction up to showing no
> recognisable picture at all. *If* that problem needs solving, there
> have been ideas floating around about resetting everything to nice
> values so that userspace can ignore what it does not understand. So far
> there has been no real interest in solving that problem in the kernel
> though.
>
> Legacy non-atomic UAPI wrappers can do whatever they want, and program
> any (new) properties they want in order to implement the legacy
> expectations, so that does not seem to be a problem.
Hi Pekka and Dmitry,
After reading through both of your comments, I think I have a better
understanding of the pixel_source implementation now.
So to summarize, we want to expose another property called
"pixel_source" to userspace that will default to FB (as to not break
legacy userspace).
If userspace wants to use solid fill planes, it will set both the
solid_fill *and* pixel_source properties to a valid blob and COLOR
respectively. If it wants to use FB, it will set FB_ID and pixel_source
to a valid FB and FB.
Here's a table illustrating what I've described above:
+-----------------+-------------------------+-------------------------+
| Use Case | Legacy Userspace | solid_fill-aware |
| | | Userspace |
+=================+=========================+=========================+
| Valid FB | pixel_source = FB | pixel_source = FB |
| | FB_ID = valid FB | FB_ID = valid FB |
+-----------------+-------------------------+-------------------------+
| Valid | pixel_source = COLOR | N/A |
| solid_fill blob | solid_fill = valid blob | |
+-----------------+-------------------------+-------------------------+
| NULL commit | pixel_source = FB | pixel_source = FB |
| | FB_ID = NULL | FB_ID = NULL |
+-----------------+-------------------------+-------------------------+
Is there anything I'm missing or needs to be clarified?
Thanks,
Jessica Zhang
>
>
> Thanks,
> pq
>
>
>>
>> Would be interested in any feedback on this.
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>> In addition, loosen the NULL FB checks within the atomic commit callstack
>>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>>>> methods where the FB was previously assumed to be non-NULL.
>>>>
>>>> Finally, have the DPU driver use drm_plane_state.color_fill and
>>>> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
>>>> and add extra checks in the DPU atomic commit callstack to account for a
>>>> NULL FB in cases where color_fill is set.
>>>>
>>>> Some drivers support hardware that have optimizations for solid fill
>>>> planes. This series aims to expose these capabilities to userspace as
>>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
>>>> hardware composer HAL) that can be set by apps like the Android Gears
>>>> app.
>>>>
>>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
>>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
>>>> framebuffer to NULL.
>>>
>>> Is there some real reason for the format property? Ie. why not
>>> just do what was the plan for the crttc background color and
>>> specify the color in full 16bpc format and just pick as many
>>> msbs from that as the hw can use?
>>>
>>>>
>>>> Jessica Zhang (3):
>>>> drm: Introduce color fill properties for drm plane
>>>> drm: Adjust atomic checks for solid fill color
>>>> drm/msm/dpu: Use color_fill property for DPU planes
>>>>
>>>> drivers/gpu/drm/drm_atomic.c | 68 ++++++++++++-----------
>>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
>>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
>>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
>>>> drivers/gpu/drm/drm_plane.c | 8 +--
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
>>>> include/drm/drm_atomic_helper.h | 5 +-
>>>> include/drm/drm_blend.h | 2 +
>>>> include/drm/drm_plane.h | 28 ++++++++++
>>>> 10 files changed, 188 insertions(+), 76 deletions(-)
>>>>
>>>> --
>>>> 2.38.0
>>>
>>> --
>>> Ville Syrjälä
>>> Intel
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-27 21:27 ` Jessica Zhang
@ 2023-06-27 21:59 ` Dmitry Baryshkov
2023-06-27 22:10 ` Abhinav Kumar
0 siblings, 1 reply; 48+ messages in thread
From: Dmitry Baryshkov @ 2023-06-27 21:59 UTC (permalink / raw)
To: Jessica Zhang, Pekka Paalanen
Cc: daniel.vetter, quic_abhinavk, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, wayland-devel, freedreno
On 28/06/2023 00:27, Jessica Zhang wrote:
>
>
> On 6/27/2023 12:58 AM, Pekka Paalanen wrote:
>> On Mon, 26 Jun 2023 16:02:50 -0700
>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>
>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>>>>> properties. When the color fill value is set, and the framebuffer
>>>>> is set
>>>>> to NULL, memory fetch will be disabled.
>>>>
>>>> Thinking a bit more universally I wonder if there should be
>>>> some kind of enum property:
>>>>
>>>> enum plane_pixel_source {
>>>> FB,
>>>> COLOR,
>>>> LIVE_FOO,
>>>> LIVE_BAR,
>>>> ...
>>>> }
>>>
>>> Reviving this thread as this was the initial comment suggesting to
>>> implement pixel_source as an enum.
>>>
>>> I think the issue with having pixel_source as an enum is how to decide
>>> what counts as a NULL commit.
>>>
>>> Currently, setting the FB to NULL will disable the plane. So I'm
>>> guessing we will extend that logic to "if there's no pixel_source set
>>> for the plane, then it will be a NULL commit and disable the plane".
>>>
>>> In that case, the question then becomes when to set the pixel_source to
>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
>>> blob), it then forces userspace to set one property before the other.
>>
>> Right, that won't work.
>>
>> There is no ordering between each property being set inside a single
>> atomic commit. They can all be applied to kernel-internal state
>> theoretically simultaneously, or any arbitrary random order, and the
>> end result must always be the same. Hence, setting one property cannot
>> change the state of another mutable property. I believe that doing
>> otherwise would make userspace fragile and hard to get right.
>>
>> I guess there might be an exception to that rule when the same property
>> is set multiple times in a single atomic commit; the last setting in
>> the array prevails. That's universal and not a special-case between two
>> specific properties.
>>
>>> Because of that, I'm thinking of having pixel_source be represented by a
>>> bitmask instead. That way, we will simply unset the corresponding
>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
>>> order to detect whether a commit is NULL or has a valid pixel source, we
>>> can just check if pixel_source == 0.
>>
>> Sounds fine to me at first hand, but isn't there the enum property that
>> says if the kernel must look at solid_fill blob *or* FB_ID?
>>
>> If enum prop says "use solid_fill prop", the why would changes to FB_ID
>> do anything? Is it for backwards-compatibility with KMS clients that do
>> not know about the enum prop?
>>
>> It seems like that kind of backwards-compatiblity will cause problems
>> in trying to reason about the atomic state, as explained above, leading
>> to very delicate and fragile conditions where things work intuitively.
>> Hence, I'm not sure backwards-compatibility is wanted. This won't be
>> the first or the last KMS property where an unexpected value left over
>> will make old atomic KMS clients silently malfunction up to showing no
>> recognisable picture at all. *If* that problem needs solving, there
>> have been ideas floating around about resetting everything to nice
>> values so that userspace can ignore what it does not understand. So far
>> there has been no real interest in solving that problem in the kernel
>> though.
>>
>> Legacy non-atomic UAPI wrappers can do whatever they want, and program
>> any (new) properties they want in order to implement the legacy
>> expectations, so that does not seem to be a problem.
>
> Hi Pekka and Dmitry,
>
> After reading through both of your comments, I think I have a better
> understanding of the pixel_source implementation now.
>
> So to summarize, we want to expose another property called
> "pixel_source" to userspace that will default to FB (as to not break
> legacy userspace).
>
> If userspace wants to use solid fill planes, it will set both the
> solid_fill *and* pixel_source properties to a valid blob and COLOR
> respectively. If it wants to use FB, it will set FB_ID and pixel_source
> to a valid FB and FB.
>
> Here's a table illustrating what I've described above:
>
> +-----------------+-------------------------+-------------------------+
> | Use Case | Legacy Userspace | solid_fill-aware |
> | | | Userspace |
> +=================+=========================+=========================+
> | Valid FB | pixel_source = FB | pixel_source = FB |
> | | FB_ID = valid FB | FB_ID = valid FB |
> +-----------------+-------------------------+-------------------------+
> | Valid | pixel_source = COLOR | N/A |
> | solid_fill blob | solid_fill = valid blob | |
Probably these two cells were swapped.
> +-----------------+-------------------------+-------------------------+
> | NULL commit | pixel_source = FB | pixel_source = FB |
> | | FB_ID = NULL | FB_ID = NULL |
> +-----------------+-------------------------+-------------------------+
| or:
| pixel_source = COLOR
| solid_fill = NULL
>
> Is there anything I'm missing or needs to be clarified?
>
LGTM otherwise
> Thanks,
>
> Jessica Zhang
>
>>
>>
>> Thanks,
>> pq
>>
>>
>>>
>>> Would be interested in any feedback on this.
>>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>>>> In addition, loosen the NULL FB checks within the atomic commit
>>>>> callstack
>>>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>>>>> methods where the FB was previously assumed to be non-NULL.
>>>>>
>>>>> Finally, have the DPU driver use drm_plane_state.color_fill and
>>>>> drm_plane_state.color_fill_format instead of
>>>>> dpu_plane_state.color_fill,
>>>>> and add extra checks in the DPU atomic commit callstack to account
>>>>> for a
>>>>> NULL FB in cases where color_fill is set.
>>>>>
>>>>> Some drivers support hardware that have optimizations for solid fill
>>>>> planes. This series aims to expose these capabilities to userspace as
>>>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the
>>>>> Android
>>>>> hardware composer HAL) that can be set by apps like the Android Gears
>>>>> app.
>>>>>
>>>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT
>>>>> to a
>>>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
>>>>> framebuffer to NULL.
>>>>
>>>> Is there some real reason for the format property? Ie. why not
>>>> just do what was the plan for the crttc background color and
>>>> specify the color in full 16bpc format and just pick as many
>>>> msbs from that as the hw can use?
>>>>>
>>>>> Jessica Zhang (3):
>>>>> drm: Introduce color fill properties for drm plane
>>>>> drm: Adjust atomic checks for solid fill color
>>>>> drm/msm/dpu: Use color_fill property for DPU planes
>>>>>
>>>>> drivers/gpu/drm/drm_atomic.c | 68
>>>>> ++++++++++++-----------
>>>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
>>>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
>>>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
>>>>> drivers/gpu/drm/drm_plane.c | 8 +--
>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66
>>>>> ++++++++++++++--------
>>>>> include/drm/drm_atomic_helper.h | 5 +-
>>>>> include/drm/drm_blend.h | 2 +
>>>>> include/drm/drm_plane.h | 28 ++++++++++
>>>>> 10 files changed, 188 insertions(+), 76 deletions(-)
>>>>>
>>>>> --
>>>>> 2.38.0
>>>>
>>>> --
>>>> Ville Syrjälä
>>>> Intel
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-27 21:59 ` Dmitry Baryshkov
@ 2023-06-27 22:10 ` Abhinav Kumar
2023-06-28 7:34 ` Pekka Paalanen
2023-06-29 18:52 ` Jessica Zhang
0 siblings, 2 replies; 48+ messages in thread
From: Abhinav Kumar @ 2023-06-27 22:10 UTC (permalink / raw)
To: Dmitry Baryshkov, Jessica Zhang, Pekka Paalanen
Cc: daniel.vetter, dri-devel, swboyd, seanpaul, laurent.pinchart,
linux-arm-msm, wayland-devel, freedreno
On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote:
> On 28/06/2023 00:27, Jessica Zhang wrote:
>>
>>
>> On 6/27/2023 12:58 AM, Pekka Paalanen wrote:
>>> On Mon, 26 Jun 2023 16:02:50 -0700
>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>
>>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
>>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>>>>>> properties. When the color fill value is set, and the framebuffer
>>>>>> is set
>>>>>> to NULL, memory fetch will be disabled.
>>>>>
>>>>> Thinking a bit more universally I wonder if there should be
>>>>> some kind of enum property:
>>>>>
>>>>> enum plane_pixel_source {
>>>>> FB,
>>>>> COLOR,
>>>>> LIVE_FOO,
>>>>> LIVE_BAR,
>>>>> ...
>>>>> }
>>>>
>>>> Reviving this thread as this was the initial comment suggesting to
>>>> implement pixel_source as an enum.
>>>>
>>>> I think the issue with having pixel_source as an enum is how to decide
>>>> what counts as a NULL commit.
>>>>
>>>> Currently, setting the FB to NULL will disable the plane. So I'm
>>>> guessing we will extend that logic to "if there's no pixel_source set
>>>> for the plane, then it will be a NULL commit and disable the plane".
>>>>
>>>> In that case, the question then becomes when to set the pixel_source to
>>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
>>>> blob), it then forces userspace to set one property before the other.
>>>
>>> Right, that won't work.
>>>
>>> There is no ordering between each property being set inside a single
>>> atomic commit. They can all be applied to kernel-internal state
>>> theoretically simultaneously, or any arbitrary random order, and the
>>> end result must always be the same. Hence, setting one property cannot
>>> change the state of another mutable property. I believe that doing
>>> otherwise would make userspace fragile and hard to get right.
>>>
>>> I guess there might be an exception to that rule when the same property
>>> is set multiple times in a single atomic commit; the last setting in
>>> the array prevails. That's universal and not a special-case between two
>>> specific properties.
>>>
>>>> Because of that, I'm thinking of having pixel_source be represented
>>>> by a
>>>> bitmask instead. That way, we will simply unset the corresponding
>>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
>>>> order to detect whether a commit is NULL or has a valid pixel
>>>> source, we
>>>> can just check if pixel_source == 0.
>>>
>>> Sounds fine to me at first hand, but isn't there the enum property that
>>> says if the kernel must look at solid_fill blob *or* FB_ID?
>>>
>>> If enum prop says "use solid_fill prop", the why would changes to FB_ID
>>> do anything? Is it for backwards-compatibility with KMS clients that do
>>> not know about the enum prop?
>>>
>>> It seems like that kind of backwards-compatiblity will cause problems
>>> in trying to reason about the atomic state, as explained above, leading
>>> to very delicate and fragile conditions where things work intuitively.
>>> Hence, I'm not sure backwards-compatibility is wanted. This won't be
>>> the first or the last KMS property where an unexpected value left over
>>> will make old atomic KMS clients silently malfunction up to showing no
>>> recognisable picture at all. *If* that problem needs solving, there
>>> have been ideas floating around about resetting everything to nice
>>> values so that userspace can ignore what it does not understand. So far
>>> there has been no real interest in solving that problem in the kernel
>>> though.
>>>
>>> Legacy non-atomic UAPI wrappers can do whatever they want, and program
>>> any (new) properties they want in order to implement the legacy
>>> expectations, so that does not seem to be a problem.
>>
>> Hi Pekka and Dmitry,
>>
>> After reading through both of your comments, I think I have a better
>> understanding of the pixel_source implementation now.
>>
>> So to summarize, we want to expose another property called
>> "pixel_source" to userspace that will default to FB (as to not break
>> legacy userspace).
>>
>> If userspace wants to use solid fill planes, it will set both the
>> solid_fill *and* pixel_source properties to a valid blob and COLOR
>> respectively. If it wants to use FB, it will set FB_ID and
>> pixel_source to a valid FB and FB.
>>
>> Here's a table illustrating what I've described above:
>>
>> +-----------------+-------------------------+-------------------------+
>> | Use Case | Legacy Userspace | solid_fill-aware |
>> | | | Userspace |
>> +=================+=========================+=========================+
>> | Valid FB | pixel_source = FB | pixel_source = FB |
>> | | FB_ID = valid FB | FB_ID = valid FB |
>> +-----------------+-------------------------+-------------------------+
>> | Valid | pixel_source = COLOR | N/A |
>> | solid_fill blob | solid_fill = valid blob | |
>
> Probably these two cells were swapped.
>
Ack, yes they were swapped.
>> +-----------------+-------------------------+-------------------------+
>> | NULL commit | pixel_source = FB | pixel_source = FB |
>> | | FB_ID = NULL | FB_ID = NULL |
>> +-----------------+-------------------------+-------------------------+
>
> | or:
> | pixel_source = COLOR
> | solid_fill = NULL
>>
>> Is there anything I'm missing or needs to be clarified?
>>
>
> LGTM otherwise
>
LGTM too.
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>
>>> Thanks,
>>> pq
>>>
>>>
>>>>
>>>> Would be interested in any feedback on this.
>>>>
>>>> Thanks,
>>>>
>>>> Jessica Zhang
>>>>
>>>>>> In addition, loosen the NULL FB checks within the atomic commit
>>>>>> callstack
>>>>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>>>>>> methods where the FB was previously assumed to be non-NULL.
>>>>>>
>>>>>> Finally, have the DPU driver use drm_plane_state.color_fill and
>>>>>> drm_plane_state.color_fill_format instead of
>>>>>> dpu_plane_state.color_fill,
>>>>>> and add extra checks in the DPU atomic commit callstack to account
>>>>>> for a
>>>>>> NULL FB in cases where color_fill is set.
>>>>>>
>>>>>> Some drivers support hardware that have optimizations for solid fill
>>>>>> planes. This series aims to expose these capabilities to userspace as
>>>>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the
>>>>>> Android
>>>>>> hardware composer HAL) that can be set by apps like the Android Gears
>>>>>> app.
>>>>>>
>>>>>> Userspace can set the color_fill value by setting
>>>>>> COLOR_FILL_FORMAT to a
>>>>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
>>>>>> framebuffer to NULL.
>>>>>
>>>>> Is there some real reason for the format property? Ie. why not
>>>>> just do what was the plan for the crttc background color and
>>>>> specify the color in full 16bpc format and just pick as many
>>>>> msbs from that as the hw can use?
>>>>>>
>>>>>> Jessica Zhang (3):
>>>>>> drm: Introduce color fill properties for drm plane
>>>>>> drm: Adjust atomic checks for solid fill color
>>>>>> drm/msm/dpu: Use color_fill property for DPU planes
>>>>>>
>>>>>> drivers/gpu/drm/drm_atomic.c | 68
>>>>>> ++++++++++++-----------
>>>>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
>>>>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
>>>>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
>>>>>> drivers/gpu/drm/drm_plane.c | 8 +--
>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66
>>>>>> ++++++++++++++--------
>>>>>> include/drm/drm_atomic_helper.h | 5 +-
>>>>>> include/drm/drm_blend.h | 2 +
>>>>>> include/drm/drm_plane.h | 28 ++++++++++
>>>>>> 10 files changed, 188 insertions(+), 76 deletions(-)
>>>>>>
>>>>>> --
>>>>>> 2.38.0
>>>>>
>>>>> --
>>>>> Ville Syrjälä
>>>>> Intel
>>>
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-27 22:10 ` Abhinav Kumar
@ 2023-06-28 7:34 ` Pekka Paalanen
2023-06-28 16:40 ` Jessica Zhang
2023-06-29 18:52 ` Jessica Zhang
1 sibling, 1 reply; 48+ messages in thread
From: Pekka Paalanen @ 2023-06-28 7:34 UTC (permalink / raw)
To: Abhinav Kumar
Cc: daniel.vetter, dri-devel, swboyd, seanpaul, laurent.pinchart,
linux-arm-msm, Dmitry Baryshkov, Jessica Zhang, wayland-devel,
freedreno
[-- Attachment #1: Type: text/plain, Size: 7443 bytes --]
On Tue, 27 Jun 2023 15:10:19 -0700
Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote:
> > On 28/06/2023 00:27, Jessica Zhang wrote:
> >>
> >>
> >> On 6/27/2023 12:58 AM, Pekka Paalanen wrote:
> >>> On Mon, 26 Jun 2023 16:02:50 -0700
> >>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >>>
> >>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> >>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> >>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> >>>>>> properties. When the color fill value is set, and the framebuffer
> >>>>>> is set
> >>>>>> to NULL, memory fetch will be disabled.
> >>>>>
> >>>>> Thinking a bit more universally I wonder if there should be
> >>>>> some kind of enum property:
> >>>>>
> >>>>> enum plane_pixel_source {
> >>>>> FB,
> >>>>> COLOR,
> >>>>> LIVE_FOO,
> >>>>> LIVE_BAR,
> >>>>> ...
> >>>>> }
> >>>>
> >>>> Reviving this thread as this was the initial comment suggesting to
> >>>> implement pixel_source as an enum.
> >>>>
> >>>> I think the issue with having pixel_source as an enum is how to decide
> >>>> what counts as a NULL commit.
> >>>>
> >>>> Currently, setting the FB to NULL will disable the plane. So I'm
> >>>> guessing we will extend that logic to "if there's no pixel_source set
> >>>> for the plane, then it will be a NULL commit and disable the plane".
> >>>>
> >>>> In that case, the question then becomes when to set the pixel_source to
> >>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
> >>>> blob), it then forces userspace to set one property before the other.
> >>>
> >>> Right, that won't work.
> >>>
> >>> There is no ordering between each property being set inside a single
> >>> atomic commit. They can all be applied to kernel-internal state
> >>> theoretically simultaneously, or any arbitrary random order, and the
> >>> end result must always be the same. Hence, setting one property cannot
> >>> change the state of another mutable property. I believe that doing
> >>> otherwise would make userspace fragile and hard to get right.
> >>>
> >>> I guess there might be an exception to that rule when the same property
> >>> is set multiple times in a single atomic commit; the last setting in
> >>> the array prevails. That's universal and not a special-case between two
> >>> specific properties.
> >>>
> >>>> Because of that, I'm thinking of having pixel_source be represented
> >>>> by a
> >>>> bitmask instead. That way, we will simply unset the corresponding
> >>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
> >>>> order to detect whether a commit is NULL or has a valid pixel
> >>>> source, we
> >>>> can just check if pixel_source == 0.
> >>>
> >>> Sounds fine to me at first hand, but isn't there the enum property that
> >>> says if the kernel must look at solid_fill blob *or* FB_ID?
> >>>
> >>> If enum prop says "use solid_fill prop", the why would changes to FB_ID
> >>> do anything? Is it for backwards-compatibility with KMS clients that do
> >>> not know about the enum prop?
> >>>
> >>> It seems like that kind of backwards-compatiblity will cause problems
> >>> in trying to reason about the atomic state, as explained above, leading
> >>> to very delicate and fragile conditions where things work intuitively.
> >>> Hence, I'm not sure backwards-compatibility is wanted. This won't be
> >>> the first or the last KMS property where an unexpected value left over
> >>> will make old atomic KMS clients silently malfunction up to showing no
> >>> recognisable picture at all. *If* that problem needs solving, there
> >>> have been ideas floating around about resetting everything to nice
> >>> values so that userspace can ignore what it does not understand. So far
> >>> there has been no real interest in solving that problem in the kernel
> >>> though.
> >>>
> >>> Legacy non-atomic UAPI wrappers can do whatever they want, and program
> >>> any (new) properties they want in order to implement the legacy
> >>> expectations, so that does not seem to be a problem.
> >>
> >> Hi Pekka and Dmitry,
> >>
> >> After reading through both of your comments, I think I have a better
> >> understanding of the pixel_source implementation now.
> >>
> >> So to summarize, we want to expose another property called
> >> "pixel_source" to userspace that will default to FB (as to not break
> >> legacy userspace).
> >>
> >> If userspace wants to use solid fill planes, it will set both the
> >> solid_fill *and* pixel_source properties to a valid blob and COLOR
> >> respectively. If it wants to use FB, it will set FB_ID and
> >> pixel_source to a valid FB and FB.
> >>
> >> Here's a table illustrating what I've described above:
> >>
> >> +-----------------+-------------------------+-------------------------+
> >> | Use Case | Legacy Userspace | solid_fill-aware |
> >> | | | Userspace |
> >> +=================+=========================+=========================+
> >> | Valid FB | pixel_source = FB | pixel_source = FB |
> >> | | FB_ID = valid FB | FB_ID = valid FB |
> >> +-----------------+-------------------------+-------------------------+
> >> | Valid | pixel_source = COLOR | N/A |
> >> | solid_fill blob | solid_fill = valid blob | |
> >
> > Probably these two cells were swapped.
> >
>
> Ack, yes they were swapped.
>
> >> +-----------------+-------------------------+-------------------------+
> >> | NULL commit | pixel_source = FB | pixel_source = FB |
> >> | | FB_ID = NULL | FB_ID = NULL |
> >> +-----------------+-------------------------+-------------------------+
> >
> > | or:
> > | pixel_source = COLOR
> > | solid_fill = NULL
> >>
> >> Is there anything I'm missing or needs to be clarified?
> >>
> >
> > LGTM otherwise
> >
>
> LGTM too.
Hi,
yeah, that sounds fine to me, if everyone thinks that adding a new
property for pixel_source is a good idea. I just assumed it was already
agreed, and based my comments on that.
I don't really remember much of the discussion about a special FB that
is actually a solid fill vs. this two new properties design, so I
cannot currently give an opinion on that, or any other design.
Btw. there may be some confusion about "legacy userspace" which usually
refers to pre-atomic userspace, and old atomic userspace that does not
understand the new properties. That makes no difference in the table
here though, the legacy ioctls should just smash pixel_source.
Thanks,
pq
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-28 7:34 ` Pekka Paalanen
@ 2023-06-28 16:40 ` Jessica Zhang
2023-06-29 7:29 ` Pekka Paalanen
0 siblings, 1 reply; 48+ messages in thread
From: Jessica Zhang @ 2023-06-28 16:40 UTC (permalink / raw)
To: Pekka Paalanen, Abhinav Kumar
Cc: daniel.vetter, dri-devel, swboyd, seanpaul, laurent.pinchart,
linux-arm-msm, Dmitry Baryshkov, wayland-devel, freedreno
On 6/28/2023 12:34 AM, Pekka Paalanen wrote:
> On Tue, 27 Jun 2023 15:10:19 -0700
> Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>> On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote:
>>> On 28/06/2023 00:27, Jessica Zhang wrote:
>>>>
>>>>
>>>> On 6/27/2023 12:58 AM, Pekka Paalanen wrote:
>>>>> On Mon, 26 Jun 2023 16:02:50 -0700
>>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>>
>>>>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
>>>>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>>>>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>>>>>>>> properties. When the color fill value is set, and the framebuffer
>>>>>>>> is set
>>>>>>>> to NULL, memory fetch will be disabled.
>>>>>>>
>>>>>>> Thinking a bit more universally I wonder if there should be
>>>>>>> some kind of enum property:
>>>>>>>
>>>>>>> enum plane_pixel_source {
>>>>>>> FB,
>>>>>>> COLOR,
>>>>>>> LIVE_FOO,
>>>>>>> LIVE_BAR,
>>>>>>> ...
>>>>>>> }
>>>>>>
>>>>>> Reviving this thread as this was the initial comment suggesting to
>>>>>> implement pixel_source as an enum.
>>>>>>
>>>>>> I think the issue with having pixel_source as an enum is how to decide
>>>>>> what counts as a NULL commit.
>>>>>>
>>>>>> Currently, setting the FB to NULL will disable the plane. So I'm
>>>>>> guessing we will extend that logic to "if there's no pixel_source set
>>>>>> for the plane, then it will be a NULL commit and disable the plane".
>>>>>>
>>>>>> In that case, the question then becomes when to set the pixel_source to
>>>>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
>>>>>> blob), it then forces userspace to set one property before the other.
>>>>>
>>>>> Right, that won't work.
>>>>>
>>>>> There is no ordering between each property being set inside a single
>>>>> atomic commit. They can all be applied to kernel-internal state
>>>>> theoretically simultaneously, or any arbitrary random order, and the
>>>>> end result must always be the same. Hence, setting one property cannot
>>>>> change the state of another mutable property. I believe that doing
>>>>> otherwise would make userspace fragile and hard to get right.
>>>>>
>>>>> I guess there might be an exception to that rule when the same property
>>>>> is set multiple times in a single atomic commit; the last setting in
>>>>> the array prevails. That's universal and not a special-case between two
>>>>> specific properties.
>>>>>
>>>>>> Because of that, I'm thinking of having pixel_source be represented
>>>>>> by a
>>>>>> bitmask instead. That way, we will simply unset the corresponding
>>>>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
>>>>>> order to detect whether a commit is NULL or has a valid pixel
>>>>>> source, we
>>>>>> can just check if pixel_source == 0.
>>>>>
>>>>> Sounds fine to me at first hand, but isn't there the enum property that
>>>>> says if the kernel must look at solid_fill blob *or* FB_ID?
>>>>>
>>>>> If enum prop says "use solid_fill prop", the why would changes to FB_ID
>>>>> do anything? Is it for backwards-compatibility with KMS clients that do
>>>>> not know about the enum prop?
>>>>>
>>>>> It seems like that kind of backwards-compatiblity will cause problems
>>>>> in trying to reason about the atomic state, as explained above, leading
>>>>> to very delicate and fragile conditions where things work intuitively.
>>>>> Hence, I'm not sure backwards-compatibility is wanted. This won't be
>>>>> the first or the last KMS property where an unexpected value left over
>>>>> will make old atomic KMS clients silently malfunction up to showing no
>>>>> recognisable picture at all. *If* that problem needs solving, there
>>>>> have been ideas floating around about resetting everything to nice
>>>>> values so that userspace can ignore what it does not understand. So far
>>>>> there has been no real interest in solving that problem in the kernel
>>>>> though.
>>>>>
>>>>> Legacy non-atomic UAPI wrappers can do whatever they want, and program
>>>>> any (new) properties they want in order to implement the legacy
>>>>> expectations, so that does not seem to be a problem.
>>>>
>>>> Hi Pekka and Dmitry,
>>>>
>>>> After reading through both of your comments, I think I have a better
>>>> understanding of the pixel_source implementation now.
>>>>
>>>> So to summarize, we want to expose another property called
>>>> "pixel_source" to userspace that will default to FB (as to not break
>>>> legacy userspace).
>>>>
>>>> If userspace wants to use solid fill planes, it will set both the
>>>> solid_fill *and* pixel_source properties to a valid blob and COLOR
>>>> respectively. If it wants to use FB, it will set FB_ID and
>>>> pixel_source to a valid FB and FB.
>>>>
>>>> Here's a table illustrating what I've described above:
>>>>
>>>> +-----------------+-------------------------+-------------------------+
>>>> | Use Case | Legacy Userspace | solid_fill-aware |
>>>> | | | Userspace |
>>>> +=================+=========================+=========================+
>>>> | Valid FB | pixel_source = FB | pixel_source = FB |
>>>> | | FB_ID = valid FB | FB_ID = valid FB |
>>>> +-----------------+-------------------------+-------------------------+
>>>> | Valid | pixel_source = COLOR | N/A |
>>>> | solid_fill blob | solid_fill = valid blob | |
>>>
>>> Probably these two cells were swapped.
>>>
>>
>> Ack, yes they were swapped.
>>
>>>> +-----------------+-------------------------+-------------------------+
>>>> | NULL commit | pixel_source = FB | pixel_source = FB |
>>>> | | FB_ID = NULL | FB_ID = NULL |
>>>> +-----------------+-------------------------+-------------------------+
>>>
>>> | or:
>>> | pixel_source = COLOR
>>> | solid_fill = NULL
>>>>
>>>> Is there anything I'm missing or needs to be clarified?
>>>>
>>>
>>> LGTM otherwise
>>>
>>
>> LGTM too.
>
> Hi,
>
> yeah, that sounds fine to me, if everyone thinks that adding a new
> property for pixel_source is a good idea. I just assumed it was already
> agreed, and based my comments on that.
>
> I don't really remember much of the discussion about a special FB that
> is actually a solid fill vs. this two new properties design, so I
> cannot currently give an opinion on that, or any other design.
Hi Pekka,
It was discussed in the v3 of this series, but we came to the conclusion
that allocating an FB for solid_fill was unnecessary since solid fill
does not require memory fetch.
Thanks,
Jessica Zhang
>
> Btw. there may be some confusion about "legacy userspace" which usually
> refers to pre-atomic userspace, and old atomic userspace that does not
> understand the new properties. That makes no difference in the table
> here though, the legacy ioctls should just smash pixel_source.
>
>
> Thanks,
> pq
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-28 16:40 ` Jessica Zhang
@ 2023-06-29 7:29 ` Pekka Paalanen
2023-06-29 18:53 ` [Freedreno] " Jessica Zhang
0 siblings, 1 reply; 48+ messages in thread
From: Pekka Paalanen @ 2023-06-29 7:29 UTC (permalink / raw)
To: Jessica Zhang
Cc: daniel.vetter, Abhinav Kumar, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, Dmitry Baryshkov, wayland-devel,
freedreno
[-- Attachment #1: Type: text/plain, Size: 8528 bytes --]
On Wed, 28 Jun 2023 09:40:21 -0700
Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> On 6/28/2023 12:34 AM, Pekka Paalanen wrote:
> > On Tue, 27 Jun 2023 15:10:19 -0700
> > Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >
> >> On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote:
> >>> On 28/06/2023 00:27, Jessica Zhang wrote:
> >>>>
> >>>>
> >>>> On 6/27/2023 12:58 AM, Pekka Paalanen wrote:
> >>>>> On Mon, 26 Jun 2023 16:02:50 -0700
> >>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
> >>>>>
> >>>>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
> >>>>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
> >>>>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
> >>>>>>>> properties. When the color fill value is set, and the framebuffer
> >>>>>>>> is set
> >>>>>>>> to NULL, memory fetch will be disabled.
> >>>>>>>
> >>>>>>> Thinking a bit more universally I wonder if there should be
> >>>>>>> some kind of enum property:
> >>>>>>>
> >>>>>>> enum plane_pixel_source {
> >>>>>>> FB,
> >>>>>>> COLOR,
> >>>>>>> LIVE_FOO,
> >>>>>>> LIVE_BAR,
> >>>>>>> ...
> >>>>>>> }
> >>>>>>
> >>>>>> Reviving this thread as this was the initial comment suggesting to
> >>>>>> implement pixel_source as an enum.
> >>>>>>
> >>>>>> I think the issue with having pixel_source as an enum is how to decide
> >>>>>> what counts as a NULL commit.
> >>>>>>
> >>>>>> Currently, setting the FB to NULL will disable the plane. So I'm
> >>>>>> guessing we will extend that logic to "if there's no pixel_source set
> >>>>>> for the plane, then it will be a NULL commit and disable the plane".
> >>>>>>
> >>>>>> In that case, the question then becomes when to set the pixel_source to
> >>>>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
> >>>>>> blob), it then forces userspace to set one property before the other.
> >>>>>
> >>>>> Right, that won't work.
> >>>>>
> >>>>> There is no ordering between each property being set inside a single
> >>>>> atomic commit. They can all be applied to kernel-internal state
> >>>>> theoretically simultaneously, or any arbitrary random order, and the
> >>>>> end result must always be the same. Hence, setting one property cannot
> >>>>> change the state of another mutable property. I believe that doing
> >>>>> otherwise would make userspace fragile and hard to get right.
> >>>>>
> >>>>> I guess there might be an exception to that rule when the same property
> >>>>> is set multiple times in a single atomic commit; the last setting in
> >>>>> the array prevails. That's universal and not a special-case between two
> >>>>> specific properties.
> >>>>>
> >>>>>> Because of that, I'm thinking of having pixel_source be represented
> >>>>>> by a
> >>>>>> bitmask instead. That way, we will simply unset the corresponding
> >>>>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
> >>>>>> order to detect whether a commit is NULL or has a valid pixel
> >>>>>> source, we
> >>>>>> can just check if pixel_source == 0.
> >>>>>
> >>>>> Sounds fine to me at first hand, but isn't there the enum property that
> >>>>> says if the kernel must look at solid_fill blob *or* FB_ID?
> >>>>>
> >>>>> If enum prop says "use solid_fill prop", the why would changes to FB_ID
> >>>>> do anything? Is it for backwards-compatibility with KMS clients that do
> >>>>> not know about the enum prop?
> >>>>>
> >>>>> It seems like that kind of backwards-compatiblity will cause problems
> >>>>> in trying to reason about the atomic state, as explained above, leading
> >>>>> to very delicate and fragile conditions where things work intuitively.
> >>>>> Hence, I'm not sure backwards-compatibility is wanted. This won't be
> >>>>> the first or the last KMS property where an unexpected value left over
> >>>>> will make old atomic KMS clients silently malfunction up to showing no
> >>>>> recognisable picture at all. *If* that problem needs solving, there
> >>>>> have been ideas floating around about resetting everything to nice
> >>>>> values so that userspace can ignore what it does not understand. So far
> >>>>> there has been no real interest in solving that problem in the kernel
> >>>>> though.
> >>>>>
> >>>>> Legacy non-atomic UAPI wrappers can do whatever they want, and program
> >>>>> any (new) properties they want in order to implement the legacy
> >>>>> expectations, so that does not seem to be a problem.
> >>>>
> >>>> Hi Pekka and Dmitry,
> >>>>
> >>>> After reading through both of your comments, I think I have a better
> >>>> understanding of the pixel_source implementation now.
> >>>>
> >>>> So to summarize, we want to expose another property called
> >>>> "pixel_source" to userspace that will default to FB (as to not break
> >>>> legacy userspace).
> >>>>
> >>>> If userspace wants to use solid fill planes, it will set both the
> >>>> solid_fill *and* pixel_source properties to a valid blob and COLOR
> >>>> respectively. If it wants to use FB, it will set FB_ID and
> >>>> pixel_source to a valid FB and FB.
> >>>>
> >>>> Here's a table illustrating what I've described above:
> >>>>
> >>>> +-----------------+-------------------------+-------------------------+
> >>>> | Use Case | Legacy Userspace | solid_fill-aware |
> >>>> | | | Userspace |
> >>>> +=================+=========================+=========================+
> >>>> | Valid FB | pixel_source = FB | pixel_source = FB |
> >>>> | | FB_ID = valid FB | FB_ID = valid FB |
> >>>> +-----------------+-------------------------+-------------------------+
> >>>> | Valid | pixel_source = COLOR | N/A |
> >>>> | solid_fill blob | solid_fill = valid blob | |
> >>>
> >>> Probably these two cells were swapped.
> >>>
> >>
> >> Ack, yes they were swapped.
> >>
> >>>> +-----------------+-------------------------+-------------------------+
> >>>> | NULL commit | pixel_source = FB | pixel_source = FB |
> >>>> | | FB_ID = NULL | FB_ID = NULL |
> >>>> +-----------------+-------------------------+-------------------------+
> >>>
> >>> | or:
> >>> | pixel_source = COLOR
> >>> | solid_fill = NULL
> >>>>
> >>>> Is there anything I'm missing or needs to be clarified?
> >>>>
> >>>
> >>> LGTM otherwise
> >>>
> >>
> >> LGTM too.
> >
> > Hi,
> >
> > yeah, that sounds fine to me, if everyone thinks that adding a new
> > property for pixel_source is a good idea. I just assumed it was already
> > agreed, and based my comments on that.
> >
> > I don't really remember much of the discussion about a special FB that
> > is actually a solid fill vs. this two new properties design, so I
> > cannot currently give an opinion on that, or any other design.
>
> Hi Pekka,
>
> It was discussed in the v3 of this series, but we came to the conclusion
> that allocating an FB for solid_fill was unnecessary since solid fill
> does not require memory fetch.
Hi,
it just occurred to me that the pixel_source property could be replaced
with the rule that if a solid_fill blob id is 0, then use FD_IB. Or
vice versa. But if someone then adds a third way of setting content on
a plane, it would result in a chain of "if this is 0, then use the next
one" and only if all are 0, there is no content.
I'm not sure if that's better or worse. Both designs seem to have the
same backwards compatibility issues, and the exact same impact to
legacy SetCrtc ioctl. Maybe pixel_source property is easier to document
and understand though when there is no "if this does not exist or is 0
then ..." chain.
So, pixel_source is fine by me.
Thanks,
pq
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-27 22:10 ` Abhinav Kumar
2023-06-28 7:34 ` Pekka Paalanen
@ 2023-06-29 18:52 ` Jessica Zhang
1 sibling, 0 replies; 48+ messages in thread
From: Jessica Zhang @ 2023-06-29 18:52 UTC (permalink / raw)
To: Abhinav Kumar, Dmitry Baryshkov, Pekka Paalanen
Cc: daniel.vetter, dri-devel, swboyd, seanpaul, laurent.pinchart,
linux-arm-msm, wayland-devel, freedreno
On 6/27/2023 3:10 PM, Abhinav Kumar wrote:
>
>
> On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote:
>> On 28/06/2023 00:27, Jessica Zhang wrote:
>>>
>>>
>>> On 6/27/2023 12:58 AM, Pekka Paalanen wrote:
>>>> On Mon, 26 Jun 2023 16:02:50 -0700
>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>
>>>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
>>>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>>>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>>>>>>> properties. When the color fill value is set, and the framebuffer
>>>>>>> is set
>>>>>>> to NULL, memory fetch will be disabled.
>>>>>>
>>>>>> Thinking a bit more universally I wonder if there should be
>>>>>> some kind of enum property:
>>>>>>
>>>>>> enum plane_pixel_source {
>>>>>> FB,
>>>>>> COLOR,
>>>>>> LIVE_FOO,
>>>>>> LIVE_BAR,
>>>>>> ...
>>>>>> }
>>>>>
>>>>> Reviving this thread as this was the initial comment suggesting to
>>>>> implement pixel_source as an enum.
>>>>>
>>>>> I think the issue with having pixel_source as an enum is how to decide
>>>>> what counts as a NULL commit.
>>>>>
>>>>> Currently, setting the FB to NULL will disable the plane. So I'm
>>>>> guessing we will extend that logic to "if there's no pixel_source set
>>>>> for the plane, then it will be a NULL commit and disable the plane".
>>>>>
>>>>> In that case, the question then becomes when to set the
>>>>> pixel_source to
>>>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
>>>>> blob), it then forces userspace to set one property before the other.
>>>>
>>>> Right, that won't work.
>>>>
>>>> There is no ordering between each property being set inside a single
>>>> atomic commit. They can all be applied to kernel-internal state
>>>> theoretically simultaneously, or any arbitrary random order, and the
>>>> end result must always be the same. Hence, setting one property cannot
>>>> change the state of another mutable property. I believe that doing
>>>> otherwise would make userspace fragile and hard to get right.
>>>>
>>>> I guess there might be an exception to that rule when the same property
>>>> is set multiple times in a single atomic commit; the last setting in
>>>> the array prevails. That's universal and not a special-case between two
>>>> specific properties.
>>>>
>>>>> Because of that, I'm thinking of having pixel_source be represented
>>>>> by a
>>>>> bitmask instead. That way, we will simply unset the corresponding
>>>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
>>>>> order to detect whether a commit is NULL or has a valid pixel
>>>>> source, we
>>>>> can just check if pixel_source == 0.
>>>>
>>>> Sounds fine to me at first hand, but isn't there the enum property that
>>>> says if the kernel must look at solid_fill blob *or* FB_ID?
>>>>
>>>> If enum prop says "use solid_fill prop", the why would changes to FB_ID
>>>> do anything? Is it for backwards-compatibility with KMS clients that do
>>>> not know about the enum prop?
>>>>
>>>> It seems like that kind of backwards-compatiblity will cause problems
>>>> in trying to reason about the atomic state, as explained above, leading
>>>> to very delicate and fragile conditions where things work intuitively.
>>>> Hence, I'm not sure backwards-compatibility is wanted. This won't be
>>>> the first or the last KMS property where an unexpected value left over
>>>> will make old atomic KMS clients silently malfunction up to showing no
>>>> recognisable picture at all. *If* that problem needs solving, there
>>>> have been ideas floating around about resetting everything to nice
>>>> values so that userspace can ignore what it does not understand. So far
>>>> there has been no real interest in solving that problem in the kernel
>>>> though.
>>>>
>>>> Legacy non-atomic UAPI wrappers can do whatever they want, and program
>>>> any (new) properties they want in order to implement the legacy
>>>> expectations, so that does not seem to be a problem.
>>>
>>> Hi Pekka and Dmitry,
>>>
>>> After reading through both of your comments, I think I have a better
>>> understanding of the pixel_source implementation now.
>>>
>>> So to summarize, we want to expose another property called
>>> "pixel_source" to userspace that will default to FB (as to not break
>>> legacy userspace).
>>>
>>> If userspace wants to use solid fill planes, it will set both the
>>> solid_fill *and* pixel_source properties to a valid blob and COLOR
>>> respectively. If it wants to use FB, it will set FB_ID and
>>> pixel_source to a valid FB and FB.
>>>
>>> Here's a table illustrating what I've described above:
>>>
>>> +-----------------+-------------------------+-------------------------+
>>> | Use Case | Legacy Userspace | solid_fill-aware |
>>> | | | Userspace |
>>> +=================+=========================+=========================+
>>> | Valid FB | pixel_source = FB | pixel_source = FB |
>>> | | FB_ID = valid FB | FB_ID = valid FB |
>>> +-----------------+-------------------------+-------------------------+
>>> | Valid | pixel_source = COLOR | N/A |
>>> | solid_fill blob | solid_fill = valid blob | |
>>
>> Probably these two cells were swapped.
>>
>
> Ack, yes they were swapped.
>
>>> +-----------------+-------------------------+-------------------------+
>>> | NULL commit | pixel_source = FB | pixel_source = FB |
>>> | | FB_ID = NULL | FB_ID = NULL |
>>> +-----------------+-------------------------+-------------------------+
>>
>> | or:
>> | pixel_source = COLOR
>> | solid_fill = NULL
Hi Dmitry and Abhinav,
Sorry, forgot to respond to this comment.
Yep, if pixel_source is set to COLOR, then a NULL solid_fill blob will
count as a NULL commit.
>>>
>>> Is there anything I'm missing or needs to be clarified?
>>>
>>
>> LGTM otherwise
>>
>
> LGTM too.
Thanks for the feedback!
BR,
Jessica Zhang
>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>>>
>>>>
>>>> Thanks,
>>>> pq
>>>>
>>>>
>>>>>
>>>>> Would be interested in any feedback on this.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jessica Zhang
>>>>>
>>>>>>> In addition, loosen the NULL FB checks within the atomic commit
>>>>>>> callstack
>>>>>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>>>>>>> methods where the FB was previously assumed to be non-NULL.
>>>>>>>
>>>>>>> Finally, have the DPU driver use drm_plane_state.color_fill and
>>>>>>> drm_plane_state.color_fill_format instead of
>>>>>>> dpu_plane_state.color_fill,
>>>>>>> and add extra checks in the DPU atomic commit callstack to
>>>>>>> account for a
>>>>>>> NULL FB in cases where color_fill is set.
>>>>>>>
>>>>>>> Some drivers support hardware that have optimizations for solid fill
>>>>>>> planes. This series aims to expose these capabilities to
>>>>>>> userspace as
>>>>>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the
>>>>>>> Android
>>>>>>> hardware composer HAL) that can be set by apps like the Android
>>>>>>> Gears
>>>>>>> app.
>>>>>>>
>>>>>>> Userspace can set the color_fill value by setting
>>>>>>> COLOR_FILL_FORMAT to a
>>>>>>> DRM format, setting COLOR_FILL to a color fill value, and setting
>>>>>>> the
>>>>>>> framebuffer to NULL.
>>>>>>
>>>>>> Is there some real reason for the format property? Ie. why not
>>>>>> just do what was the plan for the crttc background color and
>>>>>> specify the color in full 16bpc format and just pick as many
>>>>>> msbs from that as the hw can use?
>>>>>>>
>>>>>>> Jessica Zhang (3):
>>>>>>> drm: Introduce color fill properties for drm plane
>>>>>>> drm: Adjust atomic checks for solid fill color
>>>>>>> drm/msm/dpu: Use color_fill property for DPU planes
>>>>>>>
>>>>>>> drivers/gpu/drm/drm_atomic.c | 68
>>>>>>> ++++++++++++-----------
>>>>>>> drivers/gpu/drm/drm_atomic_helper.c | 34 +++++++-----
>>>>>>> drivers/gpu/drm/drm_atomic_uapi.c | 8 +++
>>>>>>> drivers/gpu/drm/drm_blend.c | 38 +++++++++++++
>>>>>>> drivers/gpu/drm/drm_plane.c | 8 +--
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 7 ++-
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66
>>>>>>> ++++++++++++++--------
>>>>>>> include/drm/drm_atomic_helper.h | 5 +-
>>>>>>> include/drm/drm_blend.h | 2 +
>>>>>>> include/drm/drm_plane.h | 28 ++++++++++
>>>>>>> 10 files changed, 188 insertions(+), 76 deletions(-)
>>>>>>>
>>>>>>> --
>>>>>>> 2.38.0
>>>>>>
>>>>>> --
>>>>>> Ville Syrjälä
>>>>>> Intel
>>>>
>>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
2023-06-29 7:29 ` Pekka Paalanen
@ 2023-06-29 18:53 ` Jessica Zhang
0 siblings, 0 replies; 48+ messages in thread
From: Jessica Zhang @ 2023-06-29 18:53 UTC (permalink / raw)
To: Pekka Paalanen
Cc: daniel.vetter, Abhinav Kumar, dri-devel, swboyd, seanpaul,
laurent.pinchart, linux-arm-msm, Dmitry Baryshkov, wayland-devel,
freedreno
On 6/29/2023 12:29 AM, Pekka Paalanen wrote:
> On Wed, 28 Jun 2023 09:40:21 -0700
> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>> On 6/28/2023 12:34 AM, Pekka Paalanen wrote:
>>> On Tue, 27 Jun 2023 15:10:19 -0700
>>> Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>
>>>> On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote:
>>>>> On 28/06/2023 00:27, Jessica Zhang wrote:
>>>>>>
>>>>>>
>>>>>> On 6/27/2023 12:58 AM, Pekka Paalanen wrote:
>>>>>>> On Mon, 26 Jun 2023 16:02:50 -0700
>>>>>>> Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>>>>>>>
>>>>>>>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
>>>>>>>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>>>>>>>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>>>>>>>>>> properties. When the color fill value is set, and the framebuffer
>>>>>>>>>> is set
>>>>>>>>>> to NULL, memory fetch will be disabled.
>>>>>>>>>
>>>>>>>>> Thinking a bit more universally I wonder if there should be
>>>>>>>>> some kind of enum property:
>>>>>>>>>
>>>>>>>>> enum plane_pixel_source {
>>>>>>>>> FB,
>>>>>>>>> COLOR,
>>>>>>>>> LIVE_FOO,
>>>>>>>>> LIVE_BAR,
>>>>>>>>> ...
>>>>>>>>> }
>>>>>>>>
>>>>>>>> Reviving this thread as this was the initial comment suggesting to
>>>>>>>> implement pixel_source as an enum.
>>>>>>>>
>>>>>>>> I think the issue with having pixel_source as an enum is how to decide
>>>>>>>> what counts as a NULL commit.
>>>>>>>>
>>>>>>>> Currently, setting the FB to NULL will disable the plane. So I'm
>>>>>>>> guessing we will extend that logic to "if there's no pixel_source set
>>>>>>>> for the plane, then it will be a NULL commit and disable the plane".
>>>>>>>>
>>>>>>>> In that case, the question then becomes when to set the pixel_source to
>>>>>>>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
>>>>>>>> blob), it then forces userspace to set one property before the other.
>>>>>>>
>>>>>>> Right, that won't work.
>>>>>>>
>>>>>>> There is no ordering between each property being set inside a single
>>>>>>> atomic commit. They can all be applied to kernel-internal state
>>>>>>> theoretically simultaneously, or any arbitrary random order, and the
>>>>>>> end result must always be the same. Hence, setting one property cannot
>>>>>>> change the state of another mutable property. I believe that doing
>>>>>>> otherwise would make userspace fragile and hard to get right.
>>>>>>>
>>>>>>> I guess there might be an exception to that rule when the same property
>>>>>>> is set multiple times in a single atomic commit; the last setting in
>>>>>>> the array prevails. That's universal and not a special-case between two
>>>>>>> specific properties.
>>>>>>>
>>>>>>>> Because of that, I'm thinking of having pixel_source be represented
>>>>>>>> by a
>>>>>>>> bitmask instead. That way, we will simply unset the corresponding
>>>>>>>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
>>>>>>>> order to detect whether a commit is NULL or has a valid pixel
>>>>>>>> source, we
>>>>>>>> can just check if pixel_source == 0.
>>>>>>>
>>>>>>> Sounds fine to me at first hand, but isn't there the enum property that
>>>>>>> says if the kernel must look at solid_fill blob *or* FB_ID?
>>>>>>>
>>>>>>> If enum prop says "use solid_fill prop", the why would changes to FB_ID
>>>>>>> do anything? Is it for backwards-compatibility with KMS clients that do
>>>>>>> not know about the enum prop?
>>>>>>>
>>>>>>> It seems like that kind of backwards-compatiblity will cause problems
>>>>>>> in trying to reason about the atomic state, as explained above, leading
>>>>>>> to very delicate and fragile conditions where things work intuitively.
>>>>>>> Hence, I'm not sure backwards-compatibility is wanted. This won't be
>>>>>>> the first or the last KMS property where an unexpected value left over
>>>>>>> will make old atomic KMS clients silently malfunction up to showing no
>>>>>>> recognisable picture at all. *If* that problem needs solving, there
>>>>>>> have been ideas floating around about resetting everything to nice
>>>>>>> values so that userspace can ignore what it does not understand. So far
>>>>>>> there has been no real interest in solving that problem in the kernel
>>>>>>> though.
>>>>>>>
>>>>>>> Legacy non-atomic UAPI wrappers can do whatever they want, and program
>>>>>>> any (new) properties they want in order to implement the legacy
>>>>>>> expectations, so that does not seem to be a problem.
>>>>>>
>>>>>> Hi Pekka and Dmitry,
>>>>>>
>>>>>> After reading through both of your comments, I think I have a better
>>>>>> understanding of the pixel_source implementation now.
>>>>>>
>>>>>> So to summarize, we want to expose another property called
>>>>>> "pixel_source" to userspace that will default to FB (as to not break
>>>>>> legacy userspace).
>>>>>>
>>>>>> If userspace wants to use solid fill planes, it will set both the
>>>>>> solid_fill *and* pixel_source properties to a valid blob and COLOR
>>>>>> respectively. If it wants to use FB, it will set FB_ID and
>>>>>> pixel_source to a valid FB and FB.
>>>>>>
>>>>>> Here's a table illustrating what I've described above:
>>>>>>
>>>>>> +-----------------+-------------------------+-------------------------+
>>>>>> | Use Case | Legacy Userspace | solid_fill-aware |
>>>>>> | | | Userspace |
>>>>>> +=================+=========================+=========================+
>>>>>> | Valid FB | pixel_source = FB | pixel_source = FB |
>>>>>> | | FB_ID = valid FB | FB_ID = valid FB |
>>>>>> +-----------------+-------------------------+-------------------------+
>>>>>> | Valid | pixel_source = COLOR | N/A |
>>>>>> | solid_fill blob | solid_fill = valid blob | |
>>>>>
>>>>> Probably these two cells were swapped.
>>>>>
>>>>
>>>> Ack, yes they were swapped.
>>>>
>>>>>> +-----------------+-------------------------+-------------------------+
>>>>>> | NULL commit | pixel_source = FB | pixel_source = FB |
>>>>>> | | FB_ID = NULL | FB_ID = NULL |
>>>>>> +-----------------+-------------------------+-------------------------+
>>>>>
>>>>> | or:
>>>>> | pixel_source = COLOR
>>>>> | solid_fill = NULL
>>>>>>
>>>>>> Is there anything I'm missing or needs to be clarified?
>>>>>>
>>>>>
>>>>> LGTM otherwise
>>>>>
>>>>
>>>> LGTM too.
>>>
>>> Hi,
>>>
>>> yeah, that sounds fine to me, if everyone thinks that adding a new
>>> property for pixel_source is a good idea. I just assumed it was already
>>> agreed, and based my comments on that.
>>>
>>> I don't really remember much of the discussion about a special FB that
>>> is actually a solid fill vs. this two new properties design, so I
>>> cannot currently give an opinion on that, or any other design.
>>
>> Hi Pekka,
>>
>> It was discussed in the v3 of this series, but we came to the conclusion
>> that allocating an FB for solid_fill was unnecessary since solid fill
>> does not require memory fetch.
>
> Hi,
>
> it just occurred to me that the pixel_source property could be replaced
> with the rule that if a solid_fill blob id is 0, then use FD_IB. Or
> vice versa. But if someone then adds a third way of setting content on
> a plane, it would result in a chain of "if this is 0, then use the next
> one" and only if all are 0, there is no content.
Hi Pekka,
Agreed. I would prefer having a pixel_source property as it will be more
extendable than having arbitrary checks.
>
> I'm not sure if that's better or worse. Both designs seem to have the
> same backwards compatibility issues, and the exact same impact to
> legacy SetCrtc ioctl. Maybe pixel_source property is easier to document
> and understand though when there is no "if this does not exist or is 0
> then ..." chain.
FWIW, the solid_fill feature will require both the solid_fill and
pixel_source properties. I'll make a note of this in the cover letter.
>
> So, pixel_source is fine by me.
Awesome, thanks for the feedback! Will push a v4 with these changes.
Thanks,
Jessica Zhang
>
>
> Thanks,
> pq
^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2023-06-29 18:53 UTC | newest]
Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-28 22:59 [RFC PATCH 0/3] Support for Solid Fill Planes Jessica Zhang
2022-10-28 22:59 ` [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane Jessica Zhang
2022-10-29 11:23 ` Dmitry Baryshkov
2022-10-31 21:58 ` Jessica Zhang
2022-11-08 18:25 ` Simon Ser
2022-11-09 13:52 ` Daniel Vetter
2022-11-09 13:53 ` Dmitry Baryshkov
2022-11-09 13:59 ` Daniel Vetter
2022-11-10 1:44 ` Jessica Zhang
2022-11-11 9:45 ` Daniel Vetter
2022-11-18 19:15 ` Jessica Zhang
2022-10-29 12:08 ` Dmitry Baryshkov
2022-10-31 22:24 ` Jessica Zhang
2022-11-01 0:11 ` Dmitry Baryshkov
2022-11-01 17:35 ` Jessica Zhang
2022-11-08 18:50 ` Simon Ser
2022-11-08 22:01 ` Sebastian Wick
2022-11-09 9:18 ` Pekka Paalanen
2022-11-23 23:27 ` Jessica Zhang
2022-11-24 8:50 ` Pekka Paalanen
2022-11-29 18:53 ` [Freedreno] " Jessica Zhang
2022-10-28 22:59 ` [RFC PATCH 2/3] drm: Adjust atomic checks for solid fill color Jessica Zhang
2022-10-29 11:38 ` Dmitry Baryshkov
2022-10-31 20:41 ` Jessica Zhang
2022-10-28 22:59 ` [RFC PATCH 3/3] drm/msm/dpu: Use color_fill property for DPU planes Jessica Zhang
2022-10-29 11:40 ` Dmitry Baryshkov
2022-10-31 22:14 ` Jessica Zhang
2022-11-07 19:37 ` [RFC PATCH 0/3] Support for Solid Fill Planes Ville Syrjälä
2022-11-07 21:32 ` Jessica Zhang
2022-11-07 22:09 ` [Freedreno] " Rob Clark
2022-11-08 0:22 ` Jessica Zhang
2022-11-08 3:34 ` Rob Clark
2022-11-08 8:52 ` Ville Syrjälä
2022-11-22 23:20 ` Jessica Zhang
2022-11-07 23:06 ` Laurent Pinchart
2023-06-26 23:02 ` Jessica Zhang
2023-06-27 0:06 ` Dmitry Baryshkov
2023-06-27 0:45 ` Jessica Zhang
2023-06-27 1:46 ` Dmitry Baryshkov
2023-06-27 7:58 ` Pekka Paalanen
2023-06-27 21:27 ` Jessica Zhang
2023-06-27 21:59 ` Dmitry Baryshkov
2023-06-27 22:10 ` Abhinav Kumar
2023-06-28 7:34 ` Pekka Paalanen
2023-06-28 16:40 ` Jessica Zhang
2023-06-29 7:29 ` Pekka Paalanen
2023-06-29 18:53 ` [Freedreno] " Jessica Zhang
2023-06-29 18:52 ` Jessica Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).