| Message ID | 20211108213807.39865-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/psr: Fix PSR2 handling of multiplanar format | expand |
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? 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. > > Cc: Jouni Högander <jouni.hogander@intel.com> > Cc: Mika Kahola <mika.kahola@intel.com> > Fixes: 3809991ff5f4 ("drm/i915/display: Add initial selective fetch > support for biplanar formats") > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > 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); > } > } >
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@intel.com> > > Cc: Mika Kahola <mika.kahola@intel.com> > > Fixes: 3809991ff5f4 ("drm/i915/display: Add initial selective fetch > > support for biplanar formats") > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > 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); > > } > > } > > >
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? > > > > Cc: Jouni Högander <jouni.hogander@intel.com> > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > Fixes: 3809991ff5f4 ("drm/i915/display: Add initial selective > > > fetch > > > support for biplanar formats") > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > 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); > > > } > > > } > > >
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@intel.com> > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > > Fixes: 3809991ff5f4 ("drm/i915/display: Add initial selective > > > > fetch > > > > support for biplanar formats") > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > 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); > > > > } > > > > } > > > >
Thank you for the clarification. Reviewed-by: Jouni Högander <jouni.hogander@intel.com> 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@intel.com> > > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > > > Fixes: 3809991ff5f4 ("drm/i915/display: Add initial selective > > > > > fetch > > > > > support for biplanar formats") > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > --- > > > > > 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); > > > > > } > > > > > } > > > > >
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); } }
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. 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(). Cc: Jouni Högander <jouni.hogander@intel.com> Cc: Mika Kahola <mika.kahola@intel.com> Fixes: 3809991ff5f4 ("drm/i915/display: Add initial selective fetch support for biplanar formats") Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)