Re: [PATCH RFC v4 4/7] drm/atomic: Loosen FB atomic checks
From: Jessica Zhang
Date: Fri Jun 30 2023 - 19:41:47 EST
On 6/29/2023 5:48 PM, Dmitry Baryshkov wrote:
On 30/06/2023 03:25, Jessica Zhang wrote:
Loosen the requirements for atomic and legacy commit so that, in cases
where solid fill planes is enabled but no FB is set, the commit can
still go through.
This includes adding framebuffer NULL checks in other areas to account
for FB being NULL when solid fill is enabled.
Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
---
drivers/gpu/drm/drm_atomic.c | 14 +++++++-------
drivers/gpu/drm/drm_atomic_helper.c | 34
++++++++++++++++++++--------------
drivers/gpu/drm/drm_plane.c | 8 ++++----
include/drm/drm_atomic_helper.h | 4 ++--
include/drm/drm_plane.h | 28 ++++++++++++++++++++++++++++
5 files changed, 61 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 404b984d2d9f..ec9681c25366 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -668,14 +668,14 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
const struct drm_framebuffer *fb = new_plane_state->fb;
int ret;
- /* either *both* CRTC and FB must be set, or neither */
- if (crtc && !fb) {
- drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no FB\n",
+ /* either *both* CRTC and pixel source must be set, or neither */
+ if (crtc && !drm_plane_has_visible_data(new_plane_state)) {
+ drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] CRTC set but no
visible data\n",
plane->base.id, plane->name);
return -EINVAL;
- } else if (fb && !crtc) {
- drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] FB set but no CRTC\n",
- plane->base.id, plane->name);
+ } else if (drm_plane_has_visible_data(new_plane_state) && !crtc) {
+ drm_dbg_atomic(plane->dev, "[PLANE:%d:%s] Source %d has
visible data but no CRTC\n",
+ plane->base.id, plane->name,
new_plane_state->pixel_source);
return -EINVAL;
}
@@ -706,7 +706,7 @@ static int drm_atomic_plane_check(const struct
drm_plane_state *old_plane_state,
}
- if (fb) {
+ if (new_plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB &&
fb) {
ret = drm_atomic_check_fb(new_plane_state);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 41b8066f61ff..d05ec9ef2b3e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -864,7 +864,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 (!drm_plane_has_visible_data(plane_state)) {
plane_state->visible = false;
return 0;
}
@@ -881,25 +881,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 (plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && 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 (plane_state->pixel_source == DRM_PLANE_PIXEL_SOURCE_FB && 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 (drm_plane_solid_fill_enabled(plane_state)) {
+ 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 24e7998d1731..5f19a27ba182 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -861,8 +861,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 visible data means shut it down */
+ if (!drm_plane_has_visible_data(plane->state)) {
plane->old_fb = plane->fb;
ret = plane->funcs->disable_plane(plane, ctx);
if (!ret) {
@@ -913,8 +913,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 visible data means shut it down */
+ if (!drm_plane_has_visible_data(plane->state))
return plane->funcs->disable_plane(plane, ctx);
/*
diff --git a/include/drm/drm_atomic_helper.h
b/include/drm/drm_atomic_helper.h
index 536a0b0091c3..6d97f38ac1f6 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -256,8 +256,8 @@ 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->crtc == NULL &&
drm_plane_has_visible_data(new_plane_state)) ||
+ (new_plane_state->crtc != NULL &&
!drm_plane_has_visible_data(new_plane_state)));
return old_plane_state->crtc && !new_plane_state->crtc;
}
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 73fb6cf8a5d9..f893f7a56912 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -998,6 +998,34 @@ static inline struct drm_plane
*drm_plane_find(struct drm_device *dev,
#define drm_for_each_plane(plane, dev) \
list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
+/**
+ * drm_plane_solid_fill_enabled - Check if solid fill is enabled on
plane
+ * @state: plane state
+ *
+ * Returns:
+ * Whether the plane has been assigned a solid_fill_blob
+ */
+static inline bool drm_plane_solid_fill_enabled(struct
drm_plane_state *state)
+{
+ if (!state)
+ return false;
+ return state->pixel_source == DRM_PLANE_PIXEL_SOURCE_COLOR &&
state->solid_fill_blob;
+}
+
+static inline bool drm_plane_has_visible_data(const struct
drm_plane_state *state)
+{
+ switch (state->pixel_source) {
+ case DRM_PLANE_PIXEL_SOURCE_COLOR:
+ return state->solid_fill_blob != NULL;
+
+ default:
I'd say, there should a WARN_ON for the default case and then
fallthrough to the FB case.
Hi Dmitry,
Sounds good.
Thanks,
Jessica Zhang
+ return state->fb != NULL;
+ }
+
+ return false;
+}
+
+
bool drm_any_plane_has_format(struct drm_device *dev,
u32 format, u64 modifier);
--
With best wishes
Dmitry