Thank you for the clarification.
Reviewed-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
On Wed, 2021-11-10 at 18:16 +0000, Souza, Jose wrote:
> On Wed, 2021-11-10 at 16:05 +0000, Hogander, Jouni wrote:
> > On Tue, 2021-11-09 at 18:17 +0000, Souza, Jose wrote:
> > > On Tue, 2021-11-09 at 10:31 +0000, Hogander, Jouni wrote:
> > > > On Mon, 2021-11-08 at 13:38 -0800, José Roberto de Souza wrote:
> > > > > When a plane with a multiplanar format is added to the state
> > > > > by
> > > > > drm_atomic_add_affected_planes(), only the UV plane is
> > > > > added, so a intel_atomic_get_new_plane_state() call to get
> > > > > the Y
> > > > > plane state can return a null pointer.
> > > >
> > > > I don't understand how this could happen only sometimes? Were
> > > > you
> > > > able
> > > > to reproduce this somehow?
> > >
> > > here a example:
> > > plane 0 - primary plane with nv12 format
> > > plane 1 - overlay with any format
> > > plane 2 - primary slave
> > >
> > > userspace does a flip to overlay, so state do not have the
> > > primary
> > > plane
> > > but the primary and overlay planes overlap, so the primary is
> > > added
> > > by drm_atomic_add_affected_planes()...
> >
> > Ok, thank you for the explanation.
> >
> > > > Generally: checking linked_new_plane_state being valid pointer
> > > > makes
> > > > sense. I'm just wondering how and when this could happen and
> > > > how
> > > > that
> > > > should be handled.
> > > >
> > > > > To fix this, intel_atomic_get_plane_state() should be called
> > > > > and
> > > > > the return needs to be checked for errors, as it could return
> > > > > a
> > > > > EAGAIN
> > > > > as other atomic state could be holding the lock for the Y
> > > > > plane.
> > > > >
> > > > > Other issue with the patch being fixed is that the Y plane is
> > > > > not
> > > > > being committed to hardware because the corresponded plane
> > > > > bit is
> > > > > not
> > > > > set in update_planes when UV and Y planes are added to the
> > > > > state
> > > > > by
> > > > > drm_atomic_add_affected_planes().
> > > >
> > > > To my understanding this one is already set in
> > > > intel_display.c:icl_check_nv12_planes.
> > >
> > > primary plane will be added after this was executed.
> >
> > Ok, but then isn't it a problem also when selective fetch is not in
> > use?
>
> Yes but it is not common to call drm_atomic_add_affected_planes() or
> intel_atomic_get_plane_state() that late.
>
> > > > > Cc: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > > > > Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> > > > > Fixes: 3809991ff5f4 ("drm/i915/display: Add initial selective
> > > > > fetch
> > > > > support for biplanar formats")
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_psr.c | 12 ++++++++----
> > > > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > index 9d589d471e335..a1a663f362e7d 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -1732,13 +1732,17 @@ int
> > > > > intel_psr2_sel_fetch_update(struct
> > > > > intel_atomic_state *state,
> > > > > * same area for Y plane as well.
> > > > > */
> > > > > if (linked) {
> > > > > - struct intel_plane_state
> > > > > *linked_new_plane_state =
> > > > > - intel_atomic_get_new_plane_state(stat
> > > > > e,
> > > > > linked);
> > > > > - struct drm_rect *linked_sel_fetch_area
> > > > > =
> > > > > - &linked_new_plane_state-
> > > > > > psr2_sel_fetch_area;
> > > > > + struct intel_plane_state
> > > > > *linked_new_plane_state;
> > > > > + struct drm_rect *linked_sel_fetch_area;
> > > > >
> > > > > + linked_new_plane_state =
> > > > > intel_atomic_get_plane_state(state, linked);
> > > > > + if (IS_ERR(linked_new_plane_state))
> > > > > + return
> > > > > PTR_ERR(linked_new_plane_state);
> > > > > +
> > > > > + linked_sel_fetch_area =
> > > > > &linked_new_plane_state->psr2_sel_fetch_area;
> > > > > linked_sel_fetch_area->y1 =
> > > > > sel_fetch_area-
> > > > > > y1;
> > > > > linked_sel_fetch_area->y2 =
> > > > > sel_fetch_area-
> > > > > > y2;
> > > > > + crtc_state->update_planes |=
> > > > > BIT(linked-
> > > > > > id);
> > > > > }
> > > > > }
> > > > >
