[Intel-gfx] [Intel-xe] [PATCH 3/4] drm/i915/lnl: support FBC on any plane

[Matt Roper]

On Mon, Aug 28, 2023 at 09:20:34AM +0300, Vinod Govindapillai wrote:
> In LNL onwards, FBC can be associated to the first three planes.

The title of this patch shouldn't say "any plane" when the reality is
that only the first three support FBC (the upper two do not).

> The FBC will be enabled for first FBC capable visible plane
> until the userspace can select one of these FBC capable plane
> explicitly

Even if we add new Intel-specific uapi to select this explicitly, is any
userspace actually going to use it?  Would it make more sense to try to
come up with a heuristic to guess which plane would benefit the most and
switch to that automatically without userspace needing to be involved in
the decision?  For that matter, do we have a real-world use case lined
up where we can see that switching FBC to a higher plane is beneficial?
Even if the hardware suddenly has this capability, it isn't necessarily
worth adding the extra complexity to the driver if we don't expect to
get real-world benefit from it.

BTW, I'm not super familiar with all the FBC-specific details, but it
feels like if we do go forward with this, the decision to select a
specific plane for FBC usage should be handled more deliberately during
the atomic check phase.  Right now it doesn't seem like we're really
making a firm decision on which plane to use, and we're probably not
handling all the cases where the register needs to be reprogrammed
if/when the FBC moves from one plane to another (potentially on a
per-frame basis).

> 
> Bspec: 69560
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c      | 29 +++++++++++++++++++
>  .../drm/i915/display/skl_universal_plane.c    |  5 +++-
>  drivers/gpu/drm/i915/i915_reg.h               |  4 +++
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 45e205a0f740..62f59630d410 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -649,6 +649,21 @@ static void skl_fbc_program_cfb_stride(struct intel_fbc *fbc)
>  		     CHICKEN_FBC_STRIDE_MASK, val);
>  }
>  
> +static u32 lnl_plane_binding(struct intel_fbc *fbc)
> +{
> +	switch (fbc->state.plane->id) {
> +	default:
> +		MISSING_CASE(fbc->state.plane->id);
> +		fallthrough;
> +	case 0:
> +		return DPFC_CTL_PLANE_BINDING_1;
> +	case 1:
> +		return DPFC_CTL_PLANE_BINDING_2;
> +	case 2:
> +		return DPFC_CTL_PLANE_BINDING_3;
> +	}
> +}
> +
>  static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
>  {
>  	const struct intel_fbc_state *fbc_state = &fbc->state;
> @@ -660,6 +675,9 @@ static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
>  	if (IS_IVYBRIDGE(i915))
>  		dpfc_ctl |= DPFC_CTL_PLANE_IVB(fbc_state->plane->i9xx_plane);
>  
> +	if (DISPLAY_VER(i915) >= 20)
> +		dpfc_ctl |= lnl_plane_binding(fbc);
> +
>  	if (fbc_state->fence_id >= 0)
>  		dpfc_ctl |= DPFC_CTL_FENCE_EN_IVB;
>  
> @@ -1250,6 +1268,17 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state,
>  		}
>  	}
>  
> +	/*
> +	 * From LNL, FBC can be assigned on any plane. Until a provision is
> +	 * provided for the userspace to select a plane for FBC, lets select
> +	 * the first visible plane that is FBC capable.
> +	 */
> +	if (DISPLAY_VER(i915) >= 20 && fbc->state.plane &&

Isn't the fbc->state.plane here redundant with the plane check on the
next line (since a NULL plane wouldn't match there either)?

> +	    fbc->state.plane != plane) {
> +		plane_state->no_fbc_reason = "fbc enabled on another plane";

If you set this here...

> +		return 0;
> +	}
> +
>  	plane_state->no_fbc_reason = NULL;
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 4d01c7ae4485..1291351c9941 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1962,7 +1962,10 @@ static bool skl_plane_has_fbc(struct drm_i915_private *dev_priv,
>  	if ((DISPLAY_RUNTIME_INFO(dev_priv)->fbc_mask & BIT(fbc_id)) == 0)
>  		return false;
>  
> -	return plane_id == PLANE_PRIMARY;
> +	if (DISPLAY_VER(dev_priv) >= 20)
> +		return plane_id <= PLANE_SPRITE1;
> +	else
> +		return plane_id == PLANE_PRIMARY;

...and also point all three FBC-capable planes at the pipe's FBC
structure, then won't intel_fbc_update() see the non-NULL reason and try
to turn off the pipe's FBC (even though it's being used on a different
plane)?


Matt

>  }
>  
>  static struct intel_fbc *skl_plane_fbc(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index aefad14ab27a..b207774f3c33 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1327,6 +1327,10 @@
>  #define   DPFC_CTL_PLANE_IVB(i9xx_plane)	REG_FIELD_PREP(DPFC_CTL_PLANE_MASK_IVB, (i9xx_plane))
>  #define   DPFC_CTL_FENCE_EN_IVB			REG_BIT(28) /* ivb+ */
>  #define   DPFC_CTL_PERSISTENT_MODE		REG_BIT(25) /* g4x-snb */
> +#define   DPFC_CTL_PLANE_BINDING_MASK		REG_GENMASK(12, 11) /* XE */
> +#define   DPFC_CTL_PLANE_BINDING_1		REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 0) /* XE */
> +#define   DPFC_CTL_PLANE_BINDING_2		REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 1) /* XE */
> +#define   DPFC_CTL_PLANE_BINDING_3		REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 2) /* XE */
>  #define   DPFC_CTL_FALSE_COLOR			REG_BIT(10) /* ivb+ */
>  #define   DPFC_CTL_SR_EN			REG_BIT(10) /* g4x only */
>  #define   DPFC_CTL_SR_EXIT_DIS			REG_BIT(9) /* g4x only */
> -- 
> 2.34.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation