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()...
>
> 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.
>
> >
> > 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(state,
> > 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);
> > }
> > }
> >
>
