Re: [PATCH RFC v4 4/7] drm/atomic: Loosen FB atomic checks

[Jessica Zhang <quic_jesszhan@quicinc.com>]

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@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.

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
>