[Intel-gfx] [PATCH] drm/i915/psr: Fix PSR2 handling of multiplanar format

[Hogander, Jouni]

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 at intel.com>
> Cc: Mika Kahola <mika.kahola at intel.com>
> Fixes: 3809991ff5f4 ("drm/i915/display: Add initial selective fetch
> support for biplanar formats")
> Signed-off-by: José Roberto de Souza <jose.souza at 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);
>  		}
>  	}
>