Re: [PATCH] gpu: drm: i915: display: Avoid null values intel_plane_atomic_check_with_state

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 27 Sep 2024, Alessandro Zanni <alessandro.zanni87@gmail.com> wrote:
> This fix solves multiple Smatch errors:
>
> drivers/gpu/drm/i915/display/intel_atomic_plane.c:660
> intel_plane_atomic_check_with_state() error:
> we previously assumed 'fb' could be null (see line 648)
>
> drivers/gpu/drm/i915/display/intel_atomic_plane.c:664
> intel_plane_atomic_check_with_state()
> error: we previously assumed 'fb' could be null (see line 659)
>
> drivers/gpu/drm/i915/display/intel_atomic_plane.c:671
> intel_plane_atomic_check_with_state()
> error: we previously assumed 'fb' could be null (see line 663)
>
> We should check first if fb is not null before to access its properties.

new_plane_state->uapi.visible && !fb should not be possible, but it's
probably too hard for smatch to figure out. It's not exactly trivial for
humans to figure out either.

I'm thinking something like below to help both.

Ville, thoughts?


BR,
Jani.


diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index 3505a5b52eb9..d9da47aed55d 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -629,6 +629,9 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
 	if (ret)
 		return ret;
 
+	if (drm_WARN_ON(display->drm, new_plane_state->uapi.visible && !fb))
+		return -EINVAL;
+
 	if (fb)
 		new_crtc_state->enabled_planes |= BIT(plane->id);
 


>
> Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic_plane.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index e979786aa5cf..1606f79b39e6 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -656,18 +656,18 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  	    intel_plane_is_scaled(new_plane_state))
>  		new_crtc_state->scaled_planes |= BIT(plane->id);
>  
> -	if (new_plane_state->uapi.visible &&
> +	if (new_plane_state->uapi.visible && fb &&
>  	    intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier))
>  		new_crtc_state->nv12_planes |= BIT(plane->id);
>  
> -	if (new_plane_state->uapi.visible &&
> +	if (new_plane_state->uapi.visible && fb &&
>  	    fb->format->format == DRM_FORMAT_C8)
>  		new_crtc_state->c8_planes |= BIT(plane->id);
>  
>  	if (new_plane_state->uapi.visible || old_plane_state->uapi.visible)
>  		new_crtc_state->update_planes |= BIT(plane->id);
>  
> -	if (new_plane_state->uapi.visible &&
> +	if (new_plane_state->uapi.visible && fb &&
>  	    intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier)) {
>  		new_crtc_state->data_rate_y[plane->id] =
>  			intel_plane_data_rate(new_crtc_state, new_plane_state, 0);

-- 
Jani Nikula, Intel