| Message ID | 20220929140714.14794-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/atomic-helper: Don't allocated plane state in CRTC check | expand |
On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote: > In drm_atomic_helper_check_crtc_state(), do not add a new plane state > to the global state if it does not exist already. Adding a new plane > state will results in overhead for the plane during the atomic-commit > step. > > For the test in drm_atomic_helper_check_crtc_state() to succeed, it is > important that the CRTC has an enabled primary plane after the commit. > This can be a newly enabled plane or an already enabled plane. So if a > plane is not part of the commit, use the plane's existing state. The new > helper drm_atomic_get_next_plane_state() returns the correct instance. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()") > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Jocelyn Falempe <jfalempe@redhat.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/drm_atomic_helper.c | 4 +--- > include/drm/drm_atomic.h | 20 ++++++++++++++++++++ > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 98cc3137c062..463d4f3fa443 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state, > > if (plane->type != DRM_PLANE_TYPE_PRIMARY) > continue; > - plane_state = drm_atomic_get_plane_state(state, plane); > - if (IS_ERR(plane_state)) > - return PTR_ERR(plane_state); > + plane_state = drm_atomic_get_next_plane_state(state, plane); > if (plane_state->fb && plane_state->crtc) { Hmm. Why this is even looking at these. If the plane is in the crtc's plane_mask then surely plane_state->crtc must already point to this crtc? And I don't think ->fb and ->crtc are allowed to disagree, so if one is set then surely the other one must be as well or we'd return an error at some point somewhere? > has_primary_plane = true; > break; > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index 10b1990bc1f6..4006f2d459e3 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state, > return state->planes[drm_plane_index(plane)].new_state; > } > > +/** > + * drm_atomic_get_next_plane_state - get plane state after atomic commit > + * @state: global atomic state object > + * @plane: plane to grab > + * > + * This function returns the plane state that the given plane will have > + * after the atomic commit. This will be the new plane state if the plane > + * is part of the global atomic state, or the current state otherwise. > + */ > +static inline struct drm_plane_state * > +drm_atomic_get_next_plane_state(struct drm_atomic_state *state, > + struct drm_plane *plane) > +{ > + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane); > + > + if (plane_state) > + return plane_state; > + return plane->state; You're not holding the appropriate lock so that does not look safe. Even if you know somewhere, somehow this sort of thing to be safe I don't think it's a good idea to promote this with any kind of official function because someone will shoot themselves in the foot with it. > +} > + > /** > * drm_atomic_get_existing_connector_state - get connector state, if it exists > * @state: global atomic state object > -- > 2.37.3
Hi, thanks for reviewing. Am 29.09.22 um 21:03 schrieb Ville Syrjälä: > On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote: >> In drm_atomic_helper_check_crtc_state(), do not add a new plane state >> to the global state if it does not exist already. Adding a new plane >> state will results in overhead for the plane during the atomic-commit >> step. >> >> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is >> important that the CRTC has an enabled primary plane after the commit. >> This can be a newly enabled plane or an already enabled plane. So if a >> plane is not part of the commit, use the plane's existing state. The new >> helper drm_atomic_get_next_plane_state() returns the correct instance. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()") >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: Jocelyn Falempe <jfalempe@redhat.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Maxime Ripard <mripard@kernel.org> >> Cc: David Airlie <airlied@linux.ie> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> Cc: dri-devel@lists.freedesktop.org >> --- >> drivers/gpu/drm/drm_atomic_helper.c | 4 +--- >> include/drm/drm_atomic.h | 20 ++++++++++++++++++++ >> 2 files changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index 98cc3137c062..463d4f3fa443 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state, >> >> if (plane->type != DRM_PLANE_TYPE_PRIMARY) >> continue; >> - plane_state = drm_atomic_get_plane_state(state, plane); >> - if (IS_ERR(plane_state)) >> - return PTR_ERR(plane_state); >> + plane_state = drm_atomic_get_next_plane_state(state, plane); >> if (plane_state->fb && plane_state->crtc) { > > Hmm. Why this is even looking at these. If the plane is in the crtc's > plane_mask then surely plane_state->crtc must already point to this > crtc? And I don't think ->fb and ->crtc are allowed to disagree, so > if one is set then surely the other one must be as well or we'd > return an error at some point somewhere? Yeah, the crtc test is done for keeping consistency. Other places also sometimes validate that these fields don't disagree. I'll remove the crtc test in the next version. The fb test is the important one. > >> has_primary_plane = true; >> break; >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >> index 10b1990bc1f6..4006f2d459e3 100644 >> --- a/include/drm/drm_atomic.h >> +++ b/include/drm/drm_atomic.h >> @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state, >> return state->planes[drm_plane_index(plane)].new_state; >> } >> >> +/** >> + * drm_atomic_get_next_plane_state - get plane state after atomic commit >> + * @state: global atomic state object >> + * @plane: plane to grab >> + * >> + * This function returns the plane state that the given plane will have >> + * after the atomic commit. This will be the new plane state if the plane >> + * is part of the global atomic state, or the current state otherwise. >> + */ >> +static inline struct drm_plane_state * >> +drm_atomic_get_next_plane_state(struct drm_atomic_state *state, >> + struct drm_plane *plane) >> +{ >> + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane); >> + >> + if (plane_state) >> + return plane_state; >> + return plane->state; > > You're not holding the appropriate lock so that does not look safe. Even > if you know somewhere, somehow this sort of thing to be safe I don't > think it's a good idea to promote this with any kind of official > function because someone will shoot themselves in the foot with it. OK. It'll be inlined with locking added. Best regards Thomas > >> +} >> + >> /** >> * drm_atomic_get_existing_connector_state - get connector state, if it exists >> * @state: global atomic state object >> -- >> 2.37.3 >
On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote: > Hi, > > thanks for reviewing. > > Am 29.09.22 um 21:03 schrieb Ville Syrjälä: > > On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote: > >> In drm_atomic_helper_check_crtc_state(), do not add a new plane state > >> to the global state if it does not exist already. Adding a new plane > >> state will results in overhead for the plane during the atomic-commit > >> step. > >> > >> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is > >> important that the CRTC has an enabled primary plane after the commit. > >> This can be a newly enabled plane or an already enabled plane. So if a > >> plane is not part of the commit, use the plane's existing state. The new > >> helper drm_atomic_get_next_plane_state() returns the correct instance. > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()") > >> Cc: Thomas Zimmermann <tzimmermann@suse.de> > >> Cc: Jocelyn Falempe <jfalempe@redhat.com> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Cc: Maxime Ripard <mripard@kernel.org> > >> Cc: David Airlie <airlied@linux.ie> > >> Cc: Daniel Vetter <daniel@ffwll.ch> > >> Cc: dri-devel@lists.freedesktop.org > >> --- > >> drivers/gpu/drm/drm_atomic_helper.c | 4 +--- > >> include/drm/drm_atomic.h | 20 ++++++++++++++++++++ > >> 2 files changed, 21 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >> index 98cc3137c062..463d4f3fa443 100644 > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state, > >> > >> if (plane->type != DRM_PLANE_TYPE_PRIMARY) > >> continue; > >> - plane_state = drm_atomic_get_plane_state(state, plane); > >> - if (IS_ERR(plane_state)) > >> - return PTR_ERR(plane_state); > >> + plane_state = drm_atomic_get_next_plane_state(state, plane); > >> if (plane_state->fb && plane_state->crtc) { > > > > Hmm. Why this is even looking at these. If the plane is in the crtc's > > plane_mask then surely plane_state->crtc must already point to this > > crtc? And I don't think ->fb and ->crtc are allowed to disagree, so > > if one is set then surely the other one must be as well or we'd > > return an error at some point somewhere? > > Yeah, the crtc test is done for keeping consistency. Other places also > sometimes validate that these fields don't disagree. I'll remove the > crtc test in the next version. The fb test is the important one. What I'm asking how can you have crtc!=NULL && fb==NULL at all here? Some other plane state check function (can't remember which one specifically) should have rejected that. So either you're checking for impossible things, or there is a bug somewhere else. So to me it looks like it should be enough to just check the plane_mask and not even bother looking at the plane state at all. > > > > >> has_primary_plane = true; > >> break; > >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > >> index 10b1990bc1f6..4006f2d459e3 100644 > >> --- a/include/drm/drm_atomic.h > >> +++ b/include/drm/drm_atomic.h > >> @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state, > >> return state->planes[drm_plane_index(plane)].new_state; > >> } > >> > >> +/** > >> + * drm_atomic_get_next_plane_state - get plane state after atomic commit > >> + * @state: global atomic state object > >> + * @plane: plane to grab > >> + * > >> + * This function returns the plane state that the given plane will have > >> + * after the atomic commit. This will be the new plane state if the plane > >> + * is part of the global atomic state, or the current state otherwise. > >> + */ > >> +static inline struct drm_plane_state * > >> +drm_atomic_get_next_plane_state(struct drm_atomic_state *state, > >> + struct drm_plane *plane) > >> +{ > >> + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane); > >> + > >> + if (plane_state) > >> + return plane_state; > >> + return plane->state; > > > > You're not holding the appropriate lock so that does not look safe. Even > > if you know somewhere, somehow this sort of thing to be safe I don't > > think it's a good idea to promote this with any kind of official > > function because someone will shoot themselves in the foot with it. > > OK. It'll be inlined with locking added. > > Best regards > Thomas > > > > >> +} > >> + > >> /** > >> * drm_atomic_get_existing_connector_state - get connector state, if it exists > >> * @state: global atomic state object > >> -- > >> 2.37.3 > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev
On Fri, Sep 30, 2022 at 12:12:09PM +0300, Ville Syrjälä wrote: > On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote: > > Hi, > > > > thanks for reviewing. > > > > Am 29.09.22 um 21:03 schrieb Ville Syrjälä: > > > On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote: > > >> In drm_atomic_helper_check_crtc_state(), do not add a new plane state > > >> to the global state if it does not exist already. Adding a new plane > > >> state will results in overhead for the plane during the atomic-commit > > >> step. > > >> > > >> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is > > >> important that the CRTC has an enabled primary plane after the commit. > > >> This can be a newly enabled plane or an already enabled plane. So if a > > >> plane is not part of the commit, use the plane's existing state. The new > > >> helper drm_atomic_get_next_plane_state() returns the correct instance. > > >> > > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > >> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()") > > >> Cc: Thomas Zimmermann <tzimmermann@suse.de> > > >> Cc: Jocelyn Falempe <jfalempe@redhat.com> > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > >> Cc: Maxime Ripard <mripard@kernel.org> > > >> Cc: David Airlie <airlied@linux.ie> > > >> Cc: Daniel Vetter <daniel@ffwll.ch> > > >> Cc: dri-devel@lists.freedesktop.org > > >> --- > > >> drivers/gpu/drm/drm_atomic_helper.c | 4 +--- > > >> include/drm/drm_atomic.h | 20 ++++++++++++++++++++ > > >> 2 files changed, 21 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > >> index 98cc3137c062..463d4f3fa443 100644 > > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > > >> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state, > > >> > > >> if (plane->type != DRM_PLANE_TYPE_PRIMARY) > > >> continue; > > >> - plane_state = drm_atomic_get_plane_state(state, plane); > > >> - if (IS_ERR(plane_state)) > > >> - return PTR_ERR(plane_state); > > >> + plane_state = drm_atomic_get_next_plane_state(state, plane); > > >> if (plane_state->fb && plane_state->crtc) { > > > > > > Hmm. Why this is even looking at these. If the plane is in the crtc's > > > plane_mask then surely plane_state->crtc must already point to this > > > crtc? And I don't think ->fb and ->crtc are allowed to disagree, so > > > if one is set then surely the other one must be as well or we'd > > > return an error at some point somewhere? > > > > Yeah, the crtc test is done for keeping consistency. Other places also > > sometimes validate that these fields don't disagree. I'll remove the > > crtc test in the next version. The fb test is the important one. > > What I'm asking how can you have crtc!=NULL && fb==NULL at all here? > Some other plane state check function (can't remember which one > specifically) should have rejected that. So either you're checking > for impossible things, or there is a bug somewhere else. Oh and btw, fb != NULL doesn't guarantee the plane is actually visible (could have been fully clipped), if that is what you're trying to check here.
Hi Am 30.09.22 um 11:12 schrieb Ville Syrjälä: > On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote: >> Hi, >> >> thanks for reviewing. >> >> Am 29.09.22 um 21:03 schrieb Ville Syrjälä: >>> On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote: >>>> In drm_atomic_helper_check_crtc_state(), do not add a new plane state >>>> to the global state if it does not exist already. Adding a new plane >>>> state will results in overhead for the plane during the atomic-commit >>>> step. >>>> >>>> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is >>>> important that the CRTC has an enabled primary plane after the commit. >>>> This can be a newly enabled plane or an already enabled plane. So if a >>>> plane is not part of the commit, use the plane's existing state. The new >>>> helper drm_atomic_get_next_plane_state() returns the correct instance. >>>> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()") >>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>>> Cc: Jocelyn Falempe <jfalempe@redhat.com> >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Cc: Maxime Ripard <mripard@kernel.org> >>>> Cc: David Airlie <airlied@linux.ie> >>>> Cc: Daniel Vetter <daniel@ffwll.ch> >>>> Cc: dri-devel@lists.freedesktop.org >>>> --- >>>> drivers/gpu/drm/drm_atomic_helper.c | 4 +--- >>>> include/drm/drm_atomic.h | 20 ++++++++++++++++++++ >>>> 2 files changed, 21 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>>> index 98cc3137c062..463d4f3fa443 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state, >>>> >>>> if (plane->type != DRM_PLANE_TYPE_PRIMARY) >>>> continue; >>>> - plane_state = drm_atomic_get_plane_state(state, plane); >>>> - if (IS_ERR(plane_state)) >>>> - return PTR_ERR(plane_state); >>>> + plane_state = drm_atomic_get_next_plane_state(state, plane); >>>> if (plane_state->fb && plane_state->crtc) { >>> >>> Hmm. Why this is even looking at these. If the plane is in the crtc's >>> plane_mask then surely plane_state->crtc must already point to this >>> crtc? And I don't think ->fb and ->crtc are allowed to disagree, so >>> if one is set then surely the other one must be as well or we'd >>> return an error at some point somewhere? >> >> Yeah, the crtc test is done for keeping consistency. Other places also >> sometimes validate that these fields don't disagree. I'll remove the >> crtc test in the next version. The fb test is the important one. > > What I'm asking how can you have crtc!=NULL && fb==NULL at all here? > Some other plane state check function (can't remember which one > specifically) should have rejected that. So either you're checking > for impossible things, or there is a bug somewhere else. > > So to me it looks like it should be enough to just check the plane_mask > and not even bother looking at the plane state at all. Indeed. Somehow I confused plane_mask with 'mask of all planes possible'. Best regards Thomas > > >> >>> >>>> has_primary_plane = true; >>>> break; >>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >>>> index 10b1990bc1f6..4006f2d459e3 100644 >>>> --- a/include/drm/drm_atomic.h >>>> +++ b/include/drm/drm_atomic.h >>>> @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state, >>>> return state->planes[drm_plane_index(plane)].new_state; >>>> } >>>> >>>> +/** >>>> + * drm_atomic_get_next_plane_state - get plane state after atomic commit >>>> + * @state: global atomic state object >>>> + * @plane: plane to grab >>>> + * >>>> + * This function returns the plane state that the given plane will have >>>> + * after the atomic commit. This will be the new plane state if the plane >>>> + * is part of the global atomic state, or the current state otherwise. >>>> + */ >>>> +static inline struct drm_plane_state * >>>> +drm_atomic_get_next_plane_state(struct drm_atomic_state *state, >>>> + struct drm_plane *plane) >>>> +{ >>>> + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane); >>>> + >>>> + if (plane_state) >>>> + return plane_state; >>>> + return plane->state; >>> >>> You're not holding the appropriate lock so that does not look safe. Even >>> if you know somewhere, somehow this sort of thing to be safe I don't >>> think it's a good idea to promote this with any kind of official >>> function because someone will shoot themselves in the foot with it. >> >> OK. It'll be inlined with locking added. >> >> Best regards >> Thomas >> >>> >>>> +} >>>> + >>>> /** >>>> * drm_atomic_get_existing_connector_state - get connector state, if it exists >>>> * @state: global atomic state object >>>> -- >>>> 2.37.3 >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany >> (HRB 36809, AG Nürnberg) >> Geschäftsführer: Ivo Totev > > > >
Hi Am 30.09.22 um 12:26 schrieb Ville Syrjälä: > On Fri, Sep 30, 2022 at 12:12:09PM +0300, Ville Syrjälä wrote: >> On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote: >>> Hi, >>> >>> thanks for reviewing. >>> >>> Am 29.09.22 um 21:03 schrieb Ville Syrjälä: >>>> On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote: >>>>> In drm_atomic_helper_check_crtc_state(), do not add a new plane state >>>>> to the global state if it does not exist already. Adding a new plane >>>>> state will results in overhead for the plane during the atomic-commit >>>>> step. >>>>> >>>>> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is >>>>> important that the CRTC has an enabled primary plane after the commit. >>>>> This can be a newly enabled plane or an already enabled plane. So if a >>>>> plane is not part of the commit, use the plane's existing state. The new >>>>> helper drm_atomic_get_next_plane_state() returns the correct instance. >>>>> >>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>>> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()") >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>>>> Cc: Jocelyn Falempe <jfalempe@redhat.com> >>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>> Cc: Maxime Ripard <mripard@kernel.org> >>>>> Cc: David Airlie <airlied@linux.ie> >>>>> Cc: Daniel Vetter <daniel@ffwll.ch> >>>>> Cc: dri-devel@lists.freedesktop.org >>>>> --- >>>>> drivers/gpu/drm/drm_atomic_helper.c | 4 +--- >>>>> include/drm/drm_atomic.h | 20 ++++++++++++++++++++ >>>>> 2 files changed, 21 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>>>> index 98cc3137c062..463d4f3fa443 100644 >>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>>> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state, >>>>> >>>>> if (plane->type != DRM_PLANE_TYPE_PRIMARY) >>>>> continue; >>>>> - plane_state = drm_atomic_get_plane_state(state, plane); >>>>> - if (IS_ERR(plane_state)) >>>>> - return PTR_ERR(plane_state); >>>>> + plane_state = drm_atomic_get_next_plane_state(state, plane); >>>>> if (plane_state->fb && plane_state->crtc) { >>>> >>>> Hmm. Why this is even looking at these. If the plane is in the crtc's >>>> plane_mask then surely plane_state->crtc must already point to this >>>> crtc? And I don't think ->fb and ->crtc are allowed to disagree, so >>>> if one is set then surely the other one must be as well or we'd >>>> return an error at some point somewhere? >>> >>> Yeah, the crtc test is done for keeping consistency. Other places also >>> sometimes validate that these fields don't disagree. I'll remove the >>> crtc test in the next version. The fb test is the important one. >> >> What I'm asking how can you have crtc!=NULL && fb==NULL at all here? >> Some other plane state check function (can't remember which one >> specifically) should have rejected that. So either you're checking >> for impossible things, or there is a bug somewhere else. > > Oh and btw, fb != NULL doesn't guarantee the plane is actually > visible (could have been fully clipped), if that is what you're > trying to check here. > No, it's really just about having a primary plane enabled on the CRTC. Best regards Thomas
On Fri, Sep 30, 2022 at 12:36:45PM +0200, Thomas Zimmermann wrote: > Hi > > Am 30.09.22 um 12:26 schrieb Ville Syrjälä: > > On Fri, Sep 30, 2022 at 12:12:09PM +0300, Ville Syrjälä wrote: > >> On Fri, Sep 30, 2022 at 11:01:52AM +0200, Thomas Zimmermann wrote: > >>> Hi, > >>> > >>> thanks for reviewing. > >>> > >>> Am 29.09.22 um 21:03 schrieb Ville Syrjälä: > >>>> On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote: > >>>>> In drm_atomic_helper_check_crtc_state(), do not add a new plane state > >>>>> to the global state if it does not exist already. Adding a new plane > >>>>> state will results in overhead for the plane during the atomic-commit > >>>>> step. > >>>>> > >>>>> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is > >>>>> important that the CRTC has an enabled primary plane after the commit. > >>>>> This can be a newly enabled plane or an already enabled plane. So if a > >>>>> plane is not part of the commit, use the plane's existing state. The new > >>>>> helper drm_atomic_get_next_plane_state() returns the correct instance. > >>>>> > >>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >>>>> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()") > >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> > >>>>> Cc: Jocelyn Falempe <jfalempe@redhat.com> > >>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>> Cc: Maxime Ripard <mripard@kernel.org> > >>>>> Cc: David Airlie <airlied@linux.ie> > >>>>> Cc: Daniel Vetter <daniel@ffwll.ch> > >>>>> Cc: dri-devel@lists.freedesktop.org > >>>>> --- > >>>>> drivers/gpu/drm/drm_atomic_helper.c | 4 +--- > >>>>> include/drm/drm_atomic.h | 20 ++++++++++++++++++++ > >>>>> 2 files changed, 21 insertions(+), 3 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >>>>> index 98cc3137c062..463d4f3fa443 100644 > >>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c > >>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >>>>> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state, > >>>>> > >>>>> if (plane->type != DRM_PLANE_TYPE_PRIMARY) > >>>>> continue; > >>>>> - plane_state = drm_atomic_get_plane_state(state, plane); > >>>>> - if (IS_ERR(plane_state)) > >>>>> - return PTR_ERR(plane_state); > >>>>> + plane_state = drm_atomic_get_next_plane_state(state, plane); > >>>>> if (plane_state->fb && plane_state->crtc) { > >>>> > >>>> Hmm. Why this is even looking at these. If the plane is in the crtc's > >>>> plane_mask then surely plane_state->crtc must already point to this > >>>> crtc? And I don't think ->fb and ->crtc are allowed to disagree, so > >>>> if one is set then surely the other one must be as well or we'd > >>>> return an error at some point somewhere? > >>> > >>> Yeah, the crtc test is done for keeping consistency. Other places also > >>> sometimes validate that these fields don't disagree. I'll remove the > >>> crtc test in the next version. The fb test is the important one. > >> > >> What I'm asking how can you have crtc!=NULL && fb==NULL at all here? > >> Some other plane state check function (can't remember which one > >> specifically) should have rejected that. So either you're checking > >> for impossible things, or there is a bug somewhere else. > > > > Oh and btw, fb != NULL doesn't guarantee the plane is actually > > visible (could have been fully clipped), if that is what you're > > trying to check here. > > > > No, it's really just about having a primary plane enabled on the CRTC. Not sure what you mean by enabled. Usually a fully clipped plane will actually be disabled in hardware.
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 98cc3137c062..463d4f3fa443 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state, if (plane->type != DRM_PLANE_TYPE_PRIMARY) continue; - plane_state = drm_atomic_get_plane_state(state, plane); - if (IS_ERR(plane_state)) - return PTR_ERR(plane_state); + plane_state = drm_atomic_get_next_plane_state(state, plane); if (plane_state->fb && plane_state->crtc) { has_primary_plane = true; break; diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 10b1990bc1f6..4006f2d459e3 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state, return state->planes[drm_plane_index(plane)].new_state; } +/** + * drm_atomic_get_next_plane_state - get plane state after atomic commit + * @state: global atomic state object + * @plane: plane to grab + * + * This function returns the plane state that the given plane will have + * after the atomic commit. This will be the new plane state if the plane + * is part of the global atomic state, or the current state otherwise. + */ +static inline struct drm_plane_state * +drm_atomic_get_next_plane_state(struct drm_atomic_state *state, + struct drm_plane *plane) +{ + struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane); + + if (plane_state) + return plane_state; + return plane->state; +} + /** * drm_atomic_get_existing_connector_state - get connector state, if it exists * @state: global atomic state object
In drm_atomic_helper_check_crtc_state(), do not add a new plane state to the global state if it does not exist already. Adding a new plane state will results in overhead for the plane during the atomic-commit step. For the test in drm_atomic_helper_check_crtc_state() to succeed, it is important that the CRTC has an enabled primary plane after the commit. This can be a newly enabled plane or an already enabled plane. So if a plane is not part of the commit, use the plane's existing state. The new helper drm_atomic_get_next_plane_state() returns the correct instance. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()") Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Jocelyn Falempe <jfalempe@redhat.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/drm_atomic_helper.c | 4 +--- include/drm/drm_atomic.h | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-)