On Fri, 27 Sep 2024, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > On Fri, Sep 27, 2024 at 11:20:32AM +0300, Jani Nikula wrote: >> On Fri, 27 Sep 2024, Alessandro Zanni <alessandro.zanni87@xxxxxxxxx> 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; >> + > > We have probably 100 places that would need this. So it's going > to be extremely ugly. > > One approach I could maybe tolerate is something like > intel_plane_is_visible(plane_state) > { > if (drm_WARN_ON(visible && !fb)) > return false; > > return plane_state->visible; > } > > + s/plane_state->visible/intel_plane_is_visible(plane_state)/ > > But is that going to help these obtuse tools? That does help people, which is more important. :) I think the problem is first checking if fb is NULL, and then dereferencing it anyway. visible always means fb != NULL, but I forget, is the reverse true? Can we have fb != NULL and !visible? I mean could we change the fb check to visible check? BR, Jani. > >> if (fb) >> new_crtc_state->enabled_planes |= BIT(plane->id); >> >> >> >> > >> > Signed-off-by: Alessandro Zanni <alessandro.zanni87@xxxxxxxxx> >> > --- >> > 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 -- Jani Nikula, Intel