Messages in this thread | | | Date | Fri, 30 Jun 2023 03:48:44 +0300 | Subject | Re: [PATCH RFC v4 4/7] drm/atomic: Loosen FB atomic checks | From | Dmitry Baryshkov <> |
| |
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@quicinc.com> > --- > 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.
> + return state->fb != NULL; > + } > + > + return false; > +} > + > + > bool drm_any_plane_has_format(struct drm_device *dev, > u32 format, u64 modifier); > >
-- With best wishes Dmitry
| |