* [PATCH] gpu: drm: i915: display: Avoid null values intel_plane_atomic_check_with_state @ 2024-09-27 0:01 Alessandro Zanni 2024-09-27 8:20 ` Jani Nikula 0 siblings, 1 reply; 6+ messages in thread From: Alessandro Zanni @ 2024-09-27 0:01 UTC (permalink / raw) To: jani.nikula, rodrigo.vivi, joonas.lahtinen, tursulin, airlied, simona Cc: Alessandro Zanni, intel-gfx, intel-xe, dri-devel, linux-kernel, skhan, anupnewsmail 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. 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); -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] gpu: drm: i915: display: Avoid null values intel_plane_atomic_check_with_state 2024-09-27 0:01 [PATCH] gpu: drm: i915: display: Avoid null values intel_plane_atomic_check_with_state Alessandro Zanni @ 2024-09-27 8:20 ` Jani Nikula 2024-09-27 11:57 ` Ville Syrjälä 0 siblings, 1 reply; 6+ messages in thread From: Jani Nikula @ 2024-09-27 8:20 UTC (permalink / raw) To: Alessandro Zanni, rodrigo.vivi, joonas.lahtinen, tursulin, airlied, simona Cc: Alessandro Zanni, intel-gfx, intel-xe, dri-devel, linux-kernel, skhan, anupnewsmail, Ville Syrjala 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] gpu: drm: i915: display: Avoid null values intel_plane_atomic_check_with_state 2024-09-27 8:20 ` Jani Nikula @ 2024-09-27 11:57 ` Ville Syrjälä 2024-09-27 13:14 ` Jani Nikula 0 siblings, 1 reply; 6+ messages in thread From: Ville Syrjälä @ 2024-09-27 11:57 UTC (permalink / raw) To: Jani Nikula Cc: Alessandro Zanni, rodrigo.vivi, joonas.lahtinen, tursulin, airlied, simona, intel-gfx, intel-xe, dri-devel, linux-kernel, skhan, anupnewsmail On Fri, Sep 27, 2024 at 11:20:32AM +0300, Jani Nikula wrote: > 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; > + 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? > 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 -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpu: drm: i915: display: Avoid null values intel_plane_atomic_check_with_state 2024-09-27 11:57 ` Ville Syrjälä @ 2024-09-27 13:14 ` Jani Nikula 2024-09-27 13:45 ` Ville Syrjälä 0 siblings, 1 reply; 6+ messages in thread From: Jani Nikula @ 2024-09-27 13:14 UTC (permalink / raw) To: Ville Syrjälä Cc: Alessandro Zanni, rodrigo.vivi, joonas.lahtinen, tursulin, airlied, simona, intel-gfx, intel-xe, dri-devel, linux-kernel, skhan, anupnewsmail On Fri, 27 Sep 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Sep 27, 2024 at 11:20:32AM +0300, Jani Nikula wrote: >> 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; >> + > > 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@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 -- Jani Nikula, Intel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpu: drm: i915: display: Avoid null values intel_plane_atomic_check_with_state 2024-09-27 13:14 ` Jani Nikula @ 2024-09-27 13:45 ` Ville Syrjälä 2024-09-27 14:07 ` Ville Syrjälä 0 siblings, 1 reply; 6+ messages in thread From: Ville Syrjälä @ 2024-09-27 13:45 UTC (permalink / raw) To: Jani Nikula Cc: Alessandro Zanni, rodrigo.vivi, joonas.lahtinen, tursulin, airlied, simona, intel-gfx, intel-xe, dri-devel, linux-kernel, skhan, anupnewsmail On Fri, Sep 27, 2024 at 04:14:17PM +0300, Jani Nikula wrote: > On Fri, 27 Sep 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Fri, Sep 27, 2024 at 11:20:32AM +0300, Jani Nikula wrote: > >> 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; > >> + > > > > 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? No, the reverse does not hold. A plane can be invisible while still having a valid fb. Eg. the plane could be positioned completely offscreen, or the entire crtc may be inactive (DPMS off). And whenever we have an fb we want to do all the check to make sure it satisfies all the requirements, whether the plane is visible or not. Otherwise we could end up confusing userspace with something like this: 1. Usespace assigns some unsupported fb to the plane but positions the plane offscreen -> success 2. Userspace moves the plane to somewhere onscreen -> fail -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpu: drm: i915: display: Avoid null values intel_plane_atomic_check_with_state 2024-09-27 13:45 ` Ville Syrjälä @ 2024-09-27 14:07 ` Ville Syrjälä 0 siblings, 0 replies; 6+ messages in thread From: Ville Syrjälä @ 2024-09-27 14:07 UTC (permalink / raw) To: Jani Nikula Cc: Alessandro Zanni, rodrigo.vivi, joonas.lahtinen, tursulin, airlied, simona, intel-gfx, intel-xe, dri-devel, linux-kernel, skhan, anupnewsmail On Fri, Sep 27, 2024 at 04:45:44PM +0300, Ville Syrjälä wrote: > On Fri, Sep 27, 2024 at 04:14:17PM +0300, Jani Nikula wrote: > > On Fri, 27 Sep 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Fri, Sep 27, 2024 at 11:20:32AM +0300, Jani Nikula wrote: > > >> 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; > > >> + > > > > > > 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? > > No, the reverse does not hold. A plane can be invisible > while still having a valid fb. Eg. the plane could be > positioned completely offscreen, or the entire crtc may > be inactive (DPMS off). > > And whenever we have an fb we want to do all the check to make sure > it satisfies all the requirements, whether the plane is visible or > not. Otherwise we could end up confusing userspace with something > like this: > > 1. Usespace assigns some unsupported fb to the plane > but positions the plane offscreen -> success > 2. Userspace moves the plane to somewhere onscreen -> fail Basically planes should have three different "enabled" states: logically_enabled: fb!=NULL (also the crtc must be logically enabled, but drm_atomic_plane_check() guarantees this for us) visible: logically_enabled && dst rectangle is at least partially within pipe_src rectangle active: visible && crtc_is_active Currently we try to make the proper distinction between logically_enabled vs. invisible, but we do not properly handle the visible vs. active case. That is, we currently mark the plane as invisible if the crtc is inactive. That means we eg. calculate watermarks as if the plane was invisible. That may cause a subsequent "DPMS on" operation to fail unexpectedly because all of a sudden we realize that we don't have enough FIFO space for this particular plane configuration. There's a FIXME somewhere in the plane code about this. -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-09-27 14:07 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-27 0:01 [PATCH] gpu: drm: i915: display: Avoid null values intel_plane_atomic_check_with_state Alessandro Zanni 2024-09-27 8:20 ` Jani Nikula 2024-09-27 11:57 ` Ville Syrjälä 2024-09-27 13:14 ` Jani Nikula 2024-09-27 13:45 ` Ville Syrjälä 2024-09-27 14:07 ` Ville Syrjälä
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox