diff mbox series

drm/xe/display: Disable aux ccs framebuffers

Message ID 20240102182422.3823394-1-juhapekka.heikkila@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm/xe/display: Disable aux ccs framebuffers | expand

Commit Message

Juha-Pekka Heikkila Jan. 2, 2024, 6:24 p.m. UTC
Aux ccs framebuffers don't work on Xe driver hence disable them
from plane capabilities until they are fixed. Flat ccs framebuffers
work and they are left enabled. Here is separated plane capabilities
check on i915 so it can behave differencly depending on the driver.

Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
 drivers/gpu/drm/i915/Makefile                 |  1 +
 .../gpu/drm/i915/display/intel_plane_caps.c   | 68 +++++++++++++++++++
 .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
 .../drm/i915/display/skl_universal_plane.c    | 61 +----------------
 drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
 5 files changed, 107 insertions(+), 60 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h

Comments

Jani Nikula Jan. 2, 2024, 7:44 p.m. UTC | #1
On Tue, 02 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:
> Aux ccs framebuffers don't work on Xe driver hence disable them
> from plane capabilities until they are fixed. Flat ccs framebuffers
> work and they are left enabled. Here is separated plane capabilities
> check on i915 so it can behave differencly depending on the driver.

Cc: Rodrigo and xe maintainers

We need to figure out the proper workflow, the mailing lists to use, the
subject prefix to use, the acks to require, etc, for changes touching
both xe and i915.

I'd very much prefer changes to i915 display to be merged via
drm-intel-next as always. For one thing, it'll take a while to sync
stuff back from drm-xe-next to drm-intel-next, and most display
development still happens on drm-intel-next.

But this patch can't be applied to drm-intel-next, because xe doesn't
even exist on drm-intel-next yet...


BR,
Jani.


>
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  drivers/gpu/drm/i915/Makefile                 |  1 +
>  .../gpu/drm/i915/display/intel_plane_caps.c   | 68 +++++++++++++++++++
>  .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
>  .../drm/i915/display/skl_universal_plane.c    | 61 +----------------
>  drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
>  5 files changed, 107 insertions(+), 60 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e777686190ca..c5e3c2dd0a01 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -302,6 +302,7 @@ i915-y += \
>  	display/intel_overlay.o \
>  	display/intel_pch_display.o \
>  	display/intel_pch_refclk.o \
> +	display/intel_plane_caps.o \
>  	display/intel_plane_initial.o \
>  	display/intel_pmdemand.o \
>  	display/intel_psr.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> new file mode 100644
> index 000000000000..6206ae11f296
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_fb.h"
> +#include "intel_plane_caps.h"
> +
> +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> +				 enum pipe pipe, enum plane_id plane_id)
> +{
> +	/* Wa_22011186057 */
> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> +		return false;
> +
> +	if (DISPLAY_VER(i915) >= 11)
> +		return true;
> +
> +	if (IS_GEMINILAKE(i915))
> +		return pipe != PIPE_C;
> +
> +	return pipe != PIPE_C &&
> +		(plane_id == PLANE_PRIMARY ||
> +		 plane_id == PLANE_SPRITE0);
> +}
> +
> +static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> +				   enum plane_id plane_id)
> +{
> +	if (DISPLAY_VER(i915) < 12)
> +		return false;
> +
> +	/* Wa_14010477008 */
> +	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> +	    (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> +		return false;
> +
> +	/* Wa_22011186057 */
> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> +		return false;
> +
> +	return plane_id < PLANE_SPRITE4;
> +}
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id)
> +{
> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> +
> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_Y;
> +	if (DISPLAY_VER(i915) < 12)
> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> +	if (HAS_4TILE(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_4;
> +
> +	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> +		caps |= INTEL_PLANE_CAP_CCS_RC;
> +		if (DISPLAY_VER(i915) >= 12)
> +			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> +	}
> +
> +	if (gen12_plane_has_mc_ccs(i915, plane_id))
> +		caps |= INTEL_PLANE_CAP_CCS_MC;
> +
> +	return caps;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> new file mode 100644
> index 000000000000..60a941c76f23
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef __INTEL_PLANE_CAPS_H__
> +#define __INTEL_PLANE_CAPS_H__
> +
> +#include "intel_display_types.h"
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id);
> +
> +#endif /* __INTEL_PLANE_CAPS_H__ */
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 511dc1544854..f2fd3833c61d 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -17,6 +17,7 @@
>  #include "intel_fb.h"
>  #include "intel_fbc.h"
>  #include "intel_frontbuffer.h"
> +#include "intel_plane_caps.h"
>  #include "intel_psr.h"
>  #include "intel_psr_regs.h"
>  #include "skl_scaler.h"
> @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct intel_plane *plane)
>  	spin_unlock_irq(&i915->irq_lock);
>  }
>  
> -static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> -				 enum pipe pipe, enum plane_id plane_id)
> -{
> -	/* Wa_22011186057 */
> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> -		return false;
> -
> -	if (DISPLAY_VER(i915) >= 11)
> -		return true;
> -
> -	if (IS_GEMINILAKE(i915))
> -		return pipe != PIPE_C;
> -
> -	return pipe != PIPE_C &&
> -		(plane_id == PLANE_PRIMARY ||
> -		 plane_id == PLANE_SPRITE0);
> -}
> -
> -static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> -				   enum plane_id plane_id)
> -{
> -	if (DISPLAY_VER(i915) < 12)
> -		return false;
> -
> -	/* Wa_14010477008 */
> -	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> -		(IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> -		return false;
> -
> -	/* Wa_22011186057 */
> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> -		return false;
> -
> -	return plane_id < PLANE_SPRITE4;
> -}
> -
> -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
> -			     enum pipe pipe, enum plane_id plane_id)
> -{
> -	u8 caps = INTEL_PLANE_CAP_TILING_X;
> -
> -	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> -		caps |= INTEL_PLANE_CAP_TILING_Y;
> -	if (DISPLAY_VER(i915) < 12)
> -		caps |= INTEL_PLANE_CAP_TILING_Yf;
> -	if (HAS_4TILE(i915))
> -		caps |= INTEL_PLANE_CAP_TILING_4;
> -
> -	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> -		caps |= INTEL_PLANE_CAP_CCS_RC;
> -		if (DISPLAY_VER(i915) >= 12)
> -			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> -	}
> -
> -	if (gen12_plane_has_mc_ccs(i915, plane_id))
> -		caps |= INTEL_PLANE_CAP_CCS_MC;
> -
> -	return caps;
> -}
> -
>  struct intel_plane *
>  skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  			   enum pipe pipe, enum plane_id plane_id)
> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> index ccf83c12b545..425c6e6744a6 100644
> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> @@ -15,6 +15,7 @@
>  #include "intel_fb.h"
>  #include "intel_fb_pin.h"
>  #include "intel_frontbuffer.h"
> +#include "intel_plane_caps.h"
>  #include "intel_plane_initial.h"
>  
>  static bool
> @@ -289,3 +290,25 @@ void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
>  
>  	plane_config_fini(&plane_config);
>  }
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id)
> +{
> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> +
> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_Y;
> +	if (DISPLAY_VER(i915) < 12)
> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> +	if (HAS_4TILE(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_4;
> +
> +	if (HAS_FLAT_CCS(i915)) {
> +		caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
> +
> +		if (plane_id < PLANE_SPRITE4)
> +			caps |= INTEL_PLANE_CAP_CCS_MC;
> +	}
> +
> +	return caps;
> +}
Rodrigo Vivi Jan. 8, 2024, 10:18 p.m. UTC | #2
On Tue, Jan 02, 2024 at 09:44:48PM +0200, Jani Nikula wrote:
> On Tue, 02 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:
> > Aux ccs framebuffers don't work on Xe driver hence disable them
> > from plane capabilities until they are fixed. Flat ccs framebuffers
> > work and they are left enabled. Here is separated plane capabilities
> > check on i915 so it can behave differencly depending on the driver.
> 
> Cc: Rodrigo and xe maintainers
> 
> We need to figure out the proper workflow, the mailing lists to use, the
> subject prefix to use, the acks to require, etc, for changes touching
> both xe and i915.
> 
> I'd very much prefer changes to i915 display to be merged via
> drm-intel-next as always. For one thing, it'll take a while to sync
> stuff back from drm-xe-next to drm-intel-next, and most display
> development still happens on drm-intel-next.

I fully agree with you.

> 
> But this patch can't be applied to drm-intel-next, because xe doesn't
> even exist on drm-intel-next yet...

should we do a backmerge of drm-next already, or too early for that?

> 
> 
> BR,
> Jani.
> 
> 
> >
> > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
> > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/Makefile                 |  1 +
> >  .../gpu/drm/i915/display/intel_plane_caps.c   | 68 +++++++++++++++++++
> >  .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
> >  .../drm/i915/display/skl_universal_plane.c    | 61 +----------------
> >  drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
> >  5 files changed, 107 insertions(+), 60 deletions(-)
> >  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
> >  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index e777686190ca..c5e3c2dd0a01 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -302,6 +302,7 @@ i915-y += \
> >  	display/intel_overlay.o \
> >  	display/intel_pch_display.o \
> >  	display/intel_pch_refclk.o \
> > +	display/intel_plane_caps.o \
> >  	display/intel_plane_initial.o \
> >  	display/intel_pmdemand.o \
> >  	display/intel_psr.o \
> > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > new file mode 100644
> > index 000000000000..6206ae11f296
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > @@ -0,0 +1,68 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2024 Intel Corporation
> > + */
> > +
> > +#include "i915_drv.h"
> > +#include "intel_fb.h"
> > +#include "intel_plane_caps.h"
> > +
> > +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> > +				 enum pipe pipe, enum plane_id plane_id)
> > +{
> > +	/* Wa_22011186057 */
> > +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > +		return false;
> > +
> > +	if (DISPLAY_VER(i915) >= 11)
> > +		return true;
> > +
> > +	if (IS_GEMINILAKE(i915))
> > +		return pipe != PIPE_C;
> > +
> > +	return pipe != PIPE_C &&
> > +		(plane_id == PLANE_PRIMARY ||
> > +		 plane_id == PLANE_SPRITE0);
> > +}
> > +
> > +static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> > +				   enum plane_id plane_id)
> > +{
> > +	if (DISPLAY_VER(i915) < 12)
> > +		return false;
> > +
> > +	/* Wa_14010477008 */
> > +	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > +	    (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> > +		return false;
> > +
> > +	/* Wa_22011186057 */
> > +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > +		return false;
> > +
> > +	return plane_id < PLANE_SPRITE4;
> > +}
> > +
> > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > +		      enum pipe pipe, enum plane_id plane_id)
> > +{
> > +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> > +
> > +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > +		caps |= INTEL_PLANE_CAP_TILING_Y;
> > +	if (DISPLAY_VER(i915) < 12)
> > +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> > +	if (HAS_4TILE(i915))
> > +		caps |= INTEL_PLANE_CAP_TILING_4;
> > +
> > +	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> > +		caps |= INTEL_PLANE_CAP_CCS_RC;
> > +		if (DISPLAY_VER(i915) >= 12)
> > +			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> > +	}
> > +
> > +	if (gen12_plane_has_mc_ccs(i915, plane_id))
> > +		caps |= INTEL_PLANE_CAP_CCS_MC;
> > +
> > +	return caps;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > new file mode 100644
> > index 000000000000..60a941c76f23
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2024 Intel Corporation
> > + */
> > +
> > +#ifndef __INTEL_PLANE_CAPS_H__
> > +#define __INTEL_PLANE_CAPS_H__
> > +
> > +#include "intel_display_types.h"
> > +
> > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > +		      enum pipe pipe, enum plane_id plane_id);
> > +
> > +#endif /* __INTEL_PLANE_CAPS_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 511dc1544854..f2fd3833c61d 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -17,6 +17,7 @@
> >  #include "intel_fb.h"
> >  #include "intel_fbc.h"
> >  #include "intel_frontbuffer.h"
> > +#include "intel_plane_caps.h"
> >  #include "intel_psr.h"
> >  #include "intel_psr_regs.h"
> >  #include "skl_scaler.h"
> > @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct intel_plane *plane)
> >  	spin_unlock_irq(&i915->irq_lock);
> >  }
> >  
> > -static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> > -				 enum pipe pipe, enum plane_id plane_id)
> > -{
> > -	/* Wa_22011186057 */
> > -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > -		return false;
> > -
> > -	if (DISPLAY_VER(i915) >= 11)
> > -		return true;
> > -
> > -	if (IS_GEMINILAKE(i915))
> > -		return pipe != PIPE_C;
> > -
> > -	return pipe != PIPE_C &&
> > -		(plane_id == PLANE_PRIMARY ||
> > -		 plane_id == PLANE_SPRITE0);
> > -}
> > -
> > -static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> > -				   enum plane_id plane_id)
> > -{
> > -	if (DISPLAY_VER(i915) < 12)
> > -		return false;
> > -
> > -	/* Wa_14010477008 */
> > -	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > -		(IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> > -		return false;
> > -
> > -	/* Wa_22011186057 */
> > -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > -		return false;
> > -
> > -	return plane_id < PLANE_SPRITE4;
> > -}
> > -
> > -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > -			     enum pipe pipe, enum plane_id plane_id)
> > -{
> > -	u8 caps = INTEL_PLANE_CAP_TILING_X;
> > -
> > -	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > -		caps |= INTEL_PLANE_CAP_TILING_Y;
> > -	if (DISPLAY_VER(i915) < 12)
> > -		caps |= INTEL_PLANE_CAP_TILING_Yf;
> > -	if (HAS_4TILE(i915))
> > -		caps |= INTEL_PLANE_CAP_TILING_4;
> > -
> > -	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> > -		caps |= INTEL_PLANE_CAP_CCS_RC;
> > -		if (DISPLAY_VER(i915) >= 12)
> > -			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> > -	}
> > -
> > -	if (gen12_plane_has_mc_ccs(i915, plane_id))
> > -		caps |= INTEL_PLANE_CAP_CCS_MC;
> > -
> > -	return caps;
> > -}
> > -
> >  struct intel_plane *
> >  skl_universal_plane_create(struct drm_i915_private *dev_priv,
> >  			   enum pipe pipe, enum plane_id plane_id)
> > diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > index ccf83c12b545..425c6e6744a6 100644
> > --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > @@ -15,6 +15,7 @@
> >  #include "intel_fb.h"
> >  #include "intel_fb_pin.h"
> >  #include "intel_frontbuffer.h"
> > +#include "intel_plane_caps.h"
> >  #include "intel_plane_initial.h"
> >  
> >  static bool
> > @@ -289,3 +290,25 @@ void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
> >  
> >  	plane_config_fini(&plane_config);
> >  }
> > +
> > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > +		      enum pipe pipe, enum plane_id plane_id)
> > +{
> > +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> > +
> > +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > +		caps |= INTEL_PLANE_CAP_TILING_Y;
> > +	if (DISPLAY_VER(i915) < 12)
> > +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> > +	if (HAS_4TILE(i915))
> > +		caps |= INTEL_PLANE_CAP_TILING_4;
> > +
> > +	if (HAS_FLAT_CCS(i915)) {
> > +		caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
> > +
> > +		if (plane_id < PLANE_SPRITE4)
> > +			caps |= INTEL_PLANE_CAP_CCS_MC;
> > +	}
> > +
> > +	return caps;
> > +}
> 
> -- 
> Jani Nikula, Intel
Souza, Jose Jan. 9, 2024, 8:37 p.m. UTC | #3
On Tue, 2024-01-02 at 20:24 +0200, Juha-Pekka Heikkila wrote:
> Aux ccs framebuffers don't work on Xe driver hence disable them
> from plane capabilities until they are fixed. Flat ccs framebuffers
> work and they are left enabled. Here is separated plane capabilities
> check on i915 so it can behave differencly depending on the driver.
> 

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  drivers/gpu/drm/i915/Makefile                 |  1 +
>  .../gpu/drm/i915/display/intel_plane_caps.c   | 68 +++++++++++++++++++
>  .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
>  .../drm/i915/display/skl_universal_plane.c    | 61 +----------------
>  drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
>  5 files changed, 107 insertions(+), 60 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e777686190ca..c5e3c2dd0a01 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -302,6 +302,7 @@ i915-y += \
>  	display/intel_overlay.o \
>  	display/intel_pch_display.o \
>  	display/intel_pch_refclk.o \
> +	display/intel_plane_caps.o \
>  	display/intel_plane_initial.o \
>  	display/intel_pmdemand.o \
>  	display/intel_psr.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> new file mode 100644
> index 000000000000..6206ae11f296
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_fb.h"
> +#include "intel_plane_caps.h"
> +
> +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> +				 enum pipe pipe, enum plane_id plane_id)
> +{
> +	/* Wa_22011186057 */
> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> +		return false;
> +
> +	if (DISPLAY_VER(i915) >= 11)
> +		return true;
> +
> +	if (IS_GEMINILAKE(i915))
> +		return pipe != PIPE_C;
> +
> +	return pipe != PIPE_C &&
> +		(plane_id == PLANE_PRIMARY ||
> +		 plane_id == PLANE_SPRITE0);
> +}
> +
> +static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> +				   enum plane_id plane_id)
> +{
> +	if (DISPLAY_VER(i915) < 12)
> +		return false;
> +
> +	/* Wa_14010477008 */
> +	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> +	    (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> +		return false;
> +
> +	/* Wa_22011186057 */
> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> +		return false;
> +
> +	return plane_id < PLANE_SPRITE4;
> +}
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id)
> +{
> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> +
> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_Y;
> +	if (DISPLAY_VER(i915) < 12)
> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> +	if (HAS_4TILE(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_4;
> +
> +	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> +		caps |= INTEL_PLANE_CAP_CCS_RC;
> +		if (DISPLAY_VER(i915) >= 12)
> +			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> +	}
> +
> +	if (gen12_plane_has_mc_ccs(i915, plane_id))
> +		caps |= INTEL_PLANE_CAP_CCS_MC;
> +
> +	return caps;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> new file mode 100644
> index 000000000000..60a941c76f23
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef __INTEL_PLANE_CAPS_H__
> +#define __INTEL_PLANE_CAPS_H__
> +
> +#include "intel_display_types.h"
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id);
> +
> +#endif /* __INTEL_PLANE_CAPS_H__ */
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 511dc1544854..f2fd3833c61d 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -17,6 +17,7 @@
>  #include "intel_fb.h"
>  #include "intel_fbc.h"
>  #include "intel_frontbuffer.h"
> +#include "intel_plane_caps.h"
>  #include "intel_psr.h"
>  #include "intel_psr_regs.h"
>  #include "skl_scaler.h"
> @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct intel_plane *plane)
>  	spin_unlock_irq(&i915->irq_lock);
>  }
>  
> -static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> -				 enum pipe pipe, enum plane_id plane_id)
> -{
> -	/* Wa_22011186057 */
> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> -		return false;
> -
> -	if (DISPLAY_VER(i915) >= 11)
> -		return true;
> -
> -	if (IS_GEMINILAKE(i915))
> -		return pipe != PIPE_C;
> -
> -	return pipe != PIPE_C &&
> -		(plane_id == PLANE_PRIMARY ||
> -		 plane_id == PLANE_SPRITE0);
> -}
> -
> -static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> -				   enum plane_id plane_id)
> -{
> -	if (DISPLAY_VER(i915) < 12)
> -		return false;
> -
> -	/* Wa_14010477008 */
> -	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> -		(IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> -		return false;
> -
> -	/* Wa_22011186057 */
> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> -		return false;
> -
> -	return plane_id < PLANE_SPRITE4;
> -}
> -
> -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
> -			     enum pipe pipe, enum plane_id plane_id)
> -{
> -	u8 caps = INTEL_PLANE_CAP_TILING_X;
> -
> -	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> -		caps |= INTEL_PLANE_CAP_TILING_Y;
> -	if (DISPLAY_VER(i915) < 12)
> -		caps |= INTEL_PLANE_CAP_TILING_Yf;
> -	if (HAS_4TILE(i915))
> -		caps |= INTEL_PLANE_CAP_TILING_4;
> -
> -	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> -		caps |= INTEL_PLANE_CAP_CCS_RC;
> -		if (DISPLAY_VER(i915) >= 12)
> -			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> -	}
> -
> -	if (gen12_plane_has_mc_ccs(i915, plane_id))
> -		caps |= INTEL_PLANE_CAP_CCS_MC;
> -
> -	return caps;
> -}
> -
>  struct intel_plane *
>  skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  			   enum pipe pipe, enum plane_id plane_id)
> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> index ccf83c12b545..425c6e6744a6 100644
> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> @@ -15,6 +15,7 @@
>  #include "intel_fb.h"
>  #include "intel_fb_pin.h"
>  #include "intel_frontbuffer.h"
> +#include "intel_plane_caps.h"
>  #include "intel_plane_initial.h"
>  
>  static bool
> @@ -289,3 +290,25 @@ void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
>  
>  	plane_config_fini(&plane_config);
>  }
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id)
> +{
> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> +
> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_Y;
> +	if (DISPLAY_VER(i915) < 12)
> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> +	if (HAS_4TILE(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_4;
> +
> +	if (HAS_FLAT_CCS(i915)) {
> +		caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
> +
> +		if (plane_id < PLANE_SPRITE4)
> +			caps |= INTEL_PLANE_CAP_CCS_MC;
> +	}
> +
> +	return caps;
> +}
Souza, Jose Jan. 9, 2024, 8:40 p.m. UTC | #4
On Mon, 2024-01-08 at 17:18 -0500, Rodrigo Vivi wrote:
> On Tue, Jan 02, 2024 at 09:44:48PM +0200, Jani Nikula wrote:
> > On Tue, 02 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:
> > > Aux ccs framebuffers don't work on Xe driver hence disable them
> > > from plane capabilities until they are fixed. Flat ccs framebuffers
> > > work and they are left enabled. Here is separated plane capabilities
> > > check on i915 so it can behave differencly depending on the driver.
> > 
> > Cc: Rodrigo and xe maintainers
> > 
> > We need to figure out the proper workflow, the mailing lists to use, the
> > subject prefix to use, the acks to require, etc, for changes touching
> > both xe and i915.
> > 
> > I'd very much prefer changes to i915 display to be merged via
> > drm-intel-next as always. For one thing, it'll take a while to sync
> > stuff back from drm-xe-next to drm-intel-next, and most display
> > development still happens on drm-intel-next.
> 
> I fully agree with you.
> 
> > 
> > But this patch can't be applied to drm-intel-next, because xe doesn't
> > even exist on drm-intel-next yet...
> 
> should we do a backmerge of drm-next already, or too early for that?

Can we split it into 2 patches and merge it?
This is necessary to fix Wayland compositors on ADL and newer.

> 
> > 
> > 
> > BR,
> > Jani.
> > 
> > 
> > > 
> > > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
> > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > ---
> > >  drivers/gpu/drm/i915/Makefile                 |  1 +
> > >  .../gpu/drm/i915/display/intel_plane_caps.c   | 68 +++++++++++++++++++
> > >  .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
> > >  .../drm/i915/display/skl_universal_plane.c    | 61 +----------------
> > >  drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
> > >  5 files changed, 107 insertions(+), 60 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
> > >  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > 
> > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > > index e777686190ca..c5e3c2dd0a01 100644
> > > --- a/drivers/gpu/drm/i915/Makefile
> > > +++ b/drivers/gpu/drm/i915/Makefile
> > > @@ -302,6 +302,7 @@ i915-y += \
> > >  	display/intel_overlay.o \
> > >  	display/intel_pch_display.o \
> > >  	display/intel_pch_refclk.o \
> > > +	display/intel_plane_caps.o \
> > >  	display/intel_plane_initial.o \
> > >  	display/intel_pmdemand.o \
> > >  	display/intel_psr.o \
> > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > new file mode 100644
> > > index 000000000000..6206ae11f296
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > @@ -0,0 +1,68 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright © 2024 Intel Corporation
> > > + */
> > > +
> > > +#include "i915_drv.h"
> > > +#include "intel_fb.h"
> > > +#include "intel_plane_caps.h"
> > > +
> > > +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> > > +				 enum pipe pipe, enum plane_id plane_id)
> > > +{
> > > +	/* Wa_22011186057 */
> > > +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > > +		return false;
> > > +
> > > +	if (DISPLAY_VER(i915) >= 11)
> > > +		return true;
> > > +
> > > +	if (IS_GEMINILAKE(i915))
> > > +		return pipe != PIPE_C;
> > > +
> > > +	return pipe != PIPE_C &&
> > > +		(plane_id == PLANE_PRIMARY ||
> > > +		 plane_id == PLANE_SPRITE0);
> > > +}
> > > +
> > > +static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> > > +				   enum plane_id plane_id)
> > > +{
> > > +	if (DISPLAY_VER(i915) < 12)
> > > +		return false;
> > > +
> > > +	/* Wa_14010477008 */
> > > +	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > > +	    (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> > > +		return false;
> > > +
> > > +	/* Wa_22011186057 */
> > > +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > > +		return false;
> > > +
> > > +	return plane_id < PLANE_SPRITE4;
> > > +}
> > > +
> > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > +		      enum pipe pipe, enum plane_id plane_id)
> > > +{
> > > +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > +
> > > +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > +		caps |= INTEL_PLANE_CAP_TILING_Y;
> > > +	if (DISPLAY_VER(i915) < 12)
> > > +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > +	if (HAS_4TILE(i915))
> > > +		caps |= INTEL_PLANE_CAP_TILING_4;
> > > +
> > > +	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> > > +		caps |= INTEL_PLANE_CAP_CCS_RC;
> > > +		if (DISPLAY_VER(i915) >= 12)
> > > +			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> > > +	}
> > > +
> > > +	if (gen12_plane_has_mc_ccs(i915, plane_id))
> > > +		caps |= INTEL_PLANE_CAP_CCS_MC;
> > > +
> > > +	return caps;
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > new file mode 100644
> > > index 000000000000..60a941c76f23
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > @@ -0,0 +1,14 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright © 2024 Intel Corporation
> > > + */
> > > +
> > > +#ifndef __INTEL_PLANE_CAPS_H__
> > > +#define __INTEL_PLANE_CAPS_H__
> > > +
> > > +#include "intel_display_types.h"
> > > +
> > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > +		      enum pipe pipe, enum plane_id plane_id);
> > > +
> > > +#endif /* __INTEL_PLANE_CAPS_H__ */
> > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > index 511dc1544854..f2fd3833c61d 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > @@ -17,6 +17,7 @@
> > >  #include "intel_fb.h"
> > >  #include "intel_fbc.h"
> > >  #include "intel_frontbuffer.h"
> > > +#include "intel_plane_caps.h"
> > >  #include "intel_psr.h"
> > >  #include "intel_psr_regs.h"
> > >  #include "skl_scaler.h"
> > > @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct intel_plane *plane)
> > >  	spin_unlock_irq(&i915->irq_lock);
> > >  }
> > >  
> > > -static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> > > -				 enum pipe pipe, enum plane_id plane_id)
> > > -{
> > > -	/* Wa_22011186057 */
> > > -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > > -		return false;
> > > -
> > > -	if (DISPLAY_VER(i915) >= 11)
> > > -		return true;
> > > -
> > > -	if (IS_GEMINILAKE(i915))
> > > -		return pipe != PIPE_C;
> > > -
> > > -	return pipe != PIPE_C &&
> > > -		(plane_id == PLANE_PRIMARY ||
> > > -		 plane_id == PLANE_SPRITE0);
> > > -}
> > > -
> > > -static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> > > -				   enum plane_id plane_id)
> > > -{
> > > -	if (DISPLAY_VER(i915) < 12)
> > > -		return false;
> > > -
> > > -	/* Wa_14010477008 */
> > > -	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > > -		(IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> > > -		return false;
> > > -
> > > -	/* Wa_22011186057 */
> > > -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > > -		return false;
> > > -
> > > -	return plane_id < PLANE_SPRITE4;
> > > -}
> > > -
> > > -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > -			     enum pipe pipe, enum plane_id plane_id)
> > > -{
> > > -	u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > -
> > > -	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > -		caps |= INTEL_PLANE_CAP_TILING_Y;
> > > -	if (DISPLAY_VER(i915) < 12)
> > > -		caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > -	if (HAS_4TILE(i915))
> > > -		caps |= INTEL_PLANE_CAP_TILING_4;
> > > -
> > > -	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> > > -		caps |= INTEL_PLANE_CAP_CCS_RC;
> > > -		if (DISPLAY_VER(i915) >= 12)
> > > -			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> > > -	}
> > > -
> > > -	if (gen12_plane_has_mc_ccs(i915, plane_id))
> > > -		caps |= INTEL_PLANE_CAP_CCS_MC;
> > > -
> > > -	return caps;
> > > -}
> > > -
> > >  struct intel_plane *
> > >  skl_universal_plane_create(struct drm_i915_private *dev_priv,
> > >  			   enum pipe pipe, enum plane_id plane_id)
> > > diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > index ccf83c12b545..425c6e6744a6 100644
> > > --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > @@ -15,6 +15,7 @@
> > >  #include "intel_fb.h"
> > >  #include "intel_fb_pin.h"
> > >  #include "intel_frontbuffer.h"
> > > +#include "intel_plane_caps.h"
> > >  #include "intel_plane_initial.h"
> > >  
> > >  static bool
> > > @@ -289,3 +290,25 @@ void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
> > >  
> > >  	plane_config_fini(&plane_config);
> > >  }
> > > +
> > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > +		      enum pipe pipe, enum plane_id plane_id)
> > > +{
> > > +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > +
> > > +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > +		caps |= INTEL_PLANE_CAP_TILING_Y;
> > > +	if (DISPLAY_VER(i915) < 12)
> > > +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > +	if (HAS_4TILE(i915))
> > > +		caps |= INTEL_PLANE_CAP_TILING_4;
> > > +
> > > +	if (HAS_FLAT_CCS(i915)) {
> > > +		caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
> > > +
> > > +		if (plane_id < PLANE_SPRITE4)
> > > +			caps |= INTEL_PLANE_CAP_CCS_MC;
> > > +	}
> > > +
> > > +	return caps;
> > > +}
> > 
> > -- 
> > Jani Nikula, Intel
Rodrigo Vivi Jan. 10, 2024, 8:09 p.m. UTC | #5
On Tue, Jan 09, 2024 at 08:40:24PM +0000, Souza, Jose wrote:
> On Mon, 2024-01-08 at 17:18 -0500, Rodrigo Vivi wrote:
> > On Tue, Jan 02, 2024 at 09:44:48PM +0200, Jani Nikula wrote:
> > > On Tue, 02 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:
> > > > Aux ccs framebuffers don't work on Xe driver hence disable them
> > > > from plane capabilities until they are fixed. Flat ccs framebuffers
> > > > work and they are left enabled. Here is separated plane capabilities
> > > > check on i915 so it can behave differencly depending on the driver.
> > > 
> > > Cc: Rodrigo and xe maintainers
> > > 
> > > We need to figure out the proper workflow, the mailing lists to use, the
> > > subject prefix to use, the acks to require, etc, for changes touching
> > > both xe and i915.
> > > 
> > > I'd very much prefer changes to i915 display to be merged via
> > > drm-intel-next as always. For one thing, it'll take a while to sync
> > > stuff back from drm-xe-next to drm-intel-next, and most display
> > > development still happens on drm-intel-next.
> > 
> > I fully agree with you.
> > 
> > > 
> > > But this patch can't be applied to drm-intel-next, because xe doesn't
> > > even exist on drm-intel-next yet...
> > 
> > should we do a backmerge of drm-next already, or too early for that?
> 
> Can we split it into 2 patches and merge it?
> This is necessary to fix Wayland compositors on ADL and newer.

we can do either:
1. backmerge drm-next into drm-intel-next and merge this as is. (This would be with
Jani)
2. split in 2 patches, one for drm-intel-next and the other for drm-xe-next. (This would
be with Jouni)
3. merge this as is in drm-xe-next and deal with the conflicts in a future backmerge.
Since this is mostly adding a new file I don't believe that it would be a big deal.
(This would impact myself)

Since next round of drm-intel-next is mine, I'd be okay on handling that and acking
this approach number 3. But before moving forward with this I'd like to wait for
Jani's and Jouni's opinions.

> 
> > 
> > > 
> > > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > > 
> > > > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
> > > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/Makefile                 |  1 +
> > > >  .../gpu/drm/i915/display/intel_plane_caps.c   | 68 +++++++++++++++++++
> > > >  .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
> > > >  .../drm/i915/display/skl_universal_plane.c    | 61 +----------------
> > > >  drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
> > > >  5 files changed, 107 insertions(+), 60 deletions(-)
> > > >  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > >  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > > > index e777686190ca..c5e3c2dd0a01 100644
> > > > --- a/drivers/gpu/drm/i915/Makefile
> > > > +++ b/drivers/gpu/drm/i915/Makefile
> > > > @@ -302,6 +302,7 @@ i915-y += \
> > > >  	display/intel_overlay.o \
> > > >  	display/intel_pch_display.o \
> > > >  	display/intel_pch_refclk.o \
> > > > +	display/intel_plane_caps.o \
> > > >  	display/intel_plane_initial.o \
> > > >  	display/intel_pmdemand.o \
> > > >  	display/intel_psr.o \
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > new file mode 100644
> > > > index 000000000000..6206ae11f296
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > @@ -0,0 +1,68 @@
> > > > +// SPDX-License-Identifier: MIT
> > > > +/*
> > > > + * Copyright © 2024 Intel Corporation
> > > > + */
> > > > +
> > > > +#include "i915_drv.h"
> > > > +#include "intel_fb.h"
> > > > +#include "intel_plane_caps.h"
> > > > +
> > > > +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> > > > +				 enum pipe pipe, enum plane_id plane_id)
> > > > +{
> > > > +	/* Wa_22011186057 */
> > > > +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > > > +		return false;
> > > > +
> > > > +	if (DISPLAY_VER(i915) >= 11)
> > > > +		return true;
> > > > +
> > > > +	if (IS_GEMINILAKE(i915))
> > > > +		return pipe != PIPE_C;
> > > > +
> > > > +	return pipe != PIPE_C &&
> > > > +		(plane_id == PLANE_PRIMARY ||
> > > > +		 plane_id == PLANE_SPRITE0);
> > > > +}
> > > > +
> > > > +static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> > > > +				   enum plane_id plane_id)
> > > > +{
> > > > +	if (DISPLAY_VER(i915) < 12)
> > > > +		return false;
> > > > +
> > > > +	/* Wa_14010477008 */
> > > > +	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > > > +	    (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> > > > +		return false;
> > > > +
> > > > +	/* Wa_22011186057 */
> > > > +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > > > +		return false;
> > > > +
> > > > +	return plane_id < PLANE_SPRITE4;
> > > > +}
> > > > +
> > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > +		      enum pipe pipe, enum plane_id plane_id)
> > > > +{
> > > > +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > +
> > > > +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > > +		caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > +	if (DISPLAY_VER(i915) < 12)
> > > > +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > +	if (HAS_4TILE(i915))
> > > > +		caps |= INTEL_PLANE_CAP_TILING_4;
> > > > +
> > > > +	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> > > > +		caps |= INTEL_PLANE_CAP_CCS_RC;
> > > > +		if (DISPLAY_VER(i915) >= 12)
> > > > +			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> > > > +	}
> > > > +
> > > > +	if (gen12_plane_has_mc_ccs(i915, plane_id))
> > > > +		caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > +
> > > > +	return caps;
> > > > +}
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > new file mode 100644
> > > > index 000000000000..60a941c76f23
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > @@ -0,0 +1,14 @@
> > > > +/* SPDX-License-Identifier: MIT */
> > > > +/*
> > > > + * Copyright © 2024 Intel Corporation
> > > > + */
> > > > +
> > > > +#ifndef __INTEL_PLANE_CAPS_H__
> > > > +#define __INTEL_PLANE_CAPS_H__
> > > > +
> > > > +#include "intel_display_types.h"
> > > > +
> > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > +		      enum pipe pipe, enum plane_id plane_id);
> > > > +
> > > > +#endif /* __INTEL_PLANE_CAPS_H__ */
> > > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > index 511dc1544854..f2fd3833c61d 100644
> > > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > @@ -17,6 +17,7 @@
> > > >  #include "intel_fb.h"
> > > >  #include "intel_fbc.h"
> > > >  #include "intel_frontbuffer.h"
> > > > +#include "intel_plane_caps.h"
> > > >  #include "intel_psr.h"
> > > >  #include "intel_psr_regs.h"
> > > >  #include "skl_scaler.h"
> > > > @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct intel_plane *plane)
> > > >  	spin_unlock_irq(&i915->irq_lock);
> > > >  }
> > > >  
> > > > -static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> > > > -				 enum pipe pipe, enum plane_id plane_id)
> > > > -{
> > > > -	/* Wa_22011186057 */
> > > > -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > > > -		return false;
> > > > -
> > > > -	if (DISPLAY_VER(i915) >= 11)
> > > > -		return true;
> > > > -
> > > > -	if (IS_GEMINILAKE(i915))
> > > > -		return pipe != PIPE_C;
> > > > -
> > > > -	return pipe != PIPE_C &&
> > > > -		(plane_id == PLANE_PRIMARY ||
> > > > -		 plane_id == PLANE_SPRITE0);
> > > > -}
> > > > -
> > > > -static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> > > > -				   enum plane_id plane_id)
> > > > -{
> > > > -	if (DISPLAY_VER(i915) < 12)
> > > > -		return false;
> > > > -
> > > > -	/* Wa_14010477008 */
> > > > -	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > > > -		(IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> > > > -		return false;
> > > > -
> > > > -	/* Wa_22011186057 */
> > > > -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> > > > -		return false;
> > > > -
> > > > -	return plane_id < PLANE_SPRITE4;
> > > > -}
> > > > -
> > > > -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > -			     enum pipe pipe, enum plane_id plane_id)
> > > > -{
> > > > -	u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > -
> > > > -	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > > -		caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > -	if (DISPLAY_VER(i915) < 12)
> > > > -		caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > -	if (HAS_4TILE(i915))
> > > > -		caps |= INTEL_PLANE_CAP_TILING_4;
> > > > -
> > > > -	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> > > > -		caps |= INTEL_PLANE_CAP_CCS_RC;
> > > > -		if (DISPLAY_VER(i915) >= 12)
> > > > -			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> > > > -	}
> > > > -
> > > > -	if (gen12_plane_has_mc_ccs(i915, plane_id))
> > > > -		caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > -
> > > > -	return caps;
> > > > -}
> > > > -
> > > >  struct intel_plane *
> > > >  skl_universal_plane_create(struct drm_i915_private *dev_priv,
> > > >  			   enum pipe pipe, enum plane_id plane_id)
> > > > diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > index ccf83c12b545..425c6e6744a6 100644
> > > > --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > @@ -15,6 +15,7 @@
> > > >  #include "intel_fb.h"
> > > >  #include "intel_fb_pin.h"
> > > >  #include "intel_frontbuffer.h"
> > > > +#include "intel_plane_caps.h"
> > > >  #include "intel_plane_initial.h"
> > > >  
> > > >  static bool
> > > > @@ -289,3 +290,25 @@ void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
> > > >  
> > > >  	plane_config_fini(&plane_config);
> > > >  }
> > > > +
> > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > +		      enum pipe pipe, enum plane_id plane_id)
> > > > +{
> > > > +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > +
> > > > +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > > +		caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > +	if (DISPLAY_VER(i915) < 12)
> > > > +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > +	if (HAS_4TILE(i915))
> > > > +		caps |= INTEL_PLANE_CAP_TILING_4;
> > > > +
> > > > +	if (HAS_FLAT_CCS(i915)) {
> > > > +		caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
> > > > +
> > > > +		if (plane_id < PLANE_SPRITE4)
> > > > +			caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > +	}
> > > > +
> > > > +	return caps;
> > > > +}
> > > 
> > > -- 
> > > Jani Nikula, Intel
>
Hogander, Jouni Jan. 12, 2024, 12:31 p.m. UTC | #6
On Wed, 2024-01-10 at 15:09 -0500, Rodrigo Vivi wrote:
> On Tue, Jan 09, 2024 at 08:40:24PM +0000, Souza, Jose wrote:
> > On Mon, 2024-01-08 at 17:18 -0500, Rodrigo Vivi wrote:
> > > On Tue, Jan 02, 2024 at 09:44:48PM +0200, Jani Nikula wrote:
> > > > On Tue, 02 Jan 2024, Juha-Pekka Heikkila
> > > > <juhapekka.heikkila@gmail.com> wrote:
> > > > > Aux ccs framebuffers don't work on Xe driver hence disable
> > > > > them
> > > > > from plane capabilities until they are fixed. Flat ccs
> > > > > framebuffers
> > > > > work and they are left enabled. Here is separated plane
> > > > > capabilities
> > > > > check on i915 so it can behave differencly depending on the
> > > > > driver.
> > > > 
> > > > Cc: Rodrigo and xe maintainers
> > > > 
> > > > We need to figure out the proper workflow, the mailing lists to
> > > > use, the
> > > > subject prefix to use, the acks to require, etc, for changes
> > > > touching
> > > > both xe and i915.
> > > > 
> > > > I'd very much prefer changes to i915 display to be merged via
> > > > drm-intel-next as always. For one thing, it'll take a while to
> > > > sync
> > > > stuff back from drm-xe-next to drm-intel-next, and most display
> > > > development still happens on drm-intel-next.
> > > 
> > > I fully agree with you.
> > > 
> > > > 
> > > > But this patch can't be applied to drm-intel-next, because xe
> > > > doesn't
> > > > even exist on drm-intel-next yet...
> > > 
> > > should we do a backmerge of drm-next already, or too early for
> > > that?
> > 
> > Can we split it into 2 patches and merge it?
> > This is necessary to fix Wayland compositors on ADL and newer.
> 
> we can do either:
> 1. backmerge drm-next into drm-intel-next and merge this as is. (This
> would be with
> Jani)
> 2. split in 2 patches, one for drm-intel-next and the other for drm-
> xe-next. (This would
> be with Jouni)
> 3. merge this as is in drm-xe-next and deal with the conflicts in a
> future backmerge.
> Since this is mostly adding a new file I don't believe that it would
> be a big deal.
> (This would impact myself)
> 
> Since next round of drm-intel-next is mine, I'd be okay on handling
> that and acking
> this approach number 3. But before moving forward with this I'd like
> to wait for
> Jani's and Jouni's opinions.

I'm fine with approach number 3.

BR,

Jouni Högander

> 
> > 
> > > 
> > > > 
> > > > 
> > > > BR,
> > > > Jani.
> > > > 
> > > > 
> > > > > 
> > > > > Closes:
> > > > > https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
> > > > > Signed-off-by: Juha-Pekka Heikkila
> > > > > <juhapekka.heikkila@gmail.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/Makefile                 |  1 +
> > > > >  .../gpu/drm/i915/display/intel_plane_caps.c   | 68
> > > > > +++++++++++++++++++
> > > > >  .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
> > > > >  .../drm/i915/display/skl_universal_plane.c    | 61 +--------
> > > > > --------
> > > > >  drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
> > > > >  5 files changed, 107 insertions(+), 60 deletions(-)
> > > > >  create mode 100644
> > > > > drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > >  create mode 100644
> > > > > drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/Makefile
> > > > > b/drivers/gpu/drm/i915/Makefile
> > > > > index e777686190ca..c5e3c2dd0a01 100644
> > > > > --- a/drivers/gpu/drm/i915/Makefile
> > > > > +++ b/drivers/gpu/drm/i915/Makefile
> > > > > @@ -302,6 +302,7 @@ i915-y += \
> > > > >         display/intel_overlay.o \
> > > > >         display/intel_pch_display.o \
> > > > >         display/intel_pch_refclk.o \
> > > > > +       display/intel_plane_caps.o \
> > > > >         display/intel_plane_initial.o \
> > > > >         display/intel_pmdemand.o \
> > > > >         display/intel_psr.o \
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > > b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > > new file mode 100644
> > > > > index 000000000000..6206ae11f296
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > > @@ -0,0 +1,68 @@
> > > > > +// SPDX-License-Identifier: MIT
> > > > > +/*
> > > > > + * Copyright © 2024 Intel Corporation
> > > > > + */
> > > > > +
> > > > > +#include "i915_drv.h"
> > > > > +#include "intel_fb.h"
> > > > > +#include "intel_plane_caps.h"
> > > > > +
> > > > > +static bool skl_plane_has_rc_ccs(struct drm_i915_private
> > > > > *i915,
> > > > > +                                enum pipe pipe, enum
> > > > > plane_id plane_id)
> > > > > +{
> > > > > +       /* Wa_22011186057 */
> > > > > +       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
> > > > > STEP_A0, STEP_B0))
> > > > > +               return false;
> > > > > +
> > > > > +       if (DISPLAY_VER(i915) >= 11)
> > > > > +               return true;
> > > > > +
> > > > > +       if (IS_GEMINILAKE(i915))
> > > > > +               return pipe != PIPE_C;
> > > > > +
> > > > > +       return pipe != PIPE_C &&
> > > > > +               (plane_id == PLANE_PRIMARY ||
> > > > > +                plane_id == PLANE_SPRITE0);
> > > > > +}
> > > > > +
> > > > > +static bool gen12_plane_has_mc_ccs(struct drm_i915_private
> > > > > *i915,
> > > > > +                                  enum plane_id plane_id)
> > > > > +{
> > > > > +       if (DISPLAY_VER(i915) < 12)
> > > > > +               return false;
> > > > > +
> > > > > +       /* Wa_14010477008 */
> > > > > +       if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > > > > +           (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915,
> > > > > STEP_A0, STEP_D0)))
> > > > > +               return false;
> > > > > +
> > > > > +       /* Wa_22011186057 */
> > > > > +       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
> > > > > STEP_A0, STEP_B0))
> > > > > +               return false;
> > > > > +
> > > > > +       return plane_id < PLANE_SPRITE4;
> > > > > +}
> > > > > +
> > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > > +                     enum pipe pipe, enum plane_id plane_id)
> > > > > +{
> > > > > +       u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > > +
> > > > > +       if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > > > +               caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > > +       if (DISPLAY_VER(i915) < 12)
> > > > > +               caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > > +       if (HAS_4TILE(i915))
> > > > > +               caps |= INTEL_PLANE_CAP_TILING_4;
> > > > > +
> > > > > +       if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> > > > > +               caps |= INTEL_PLANE_CAP_CCS_RC;
> > > > > +               if (DISPLAY_VER(i915) >= 12)
> > > > > +                       caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> > > > > +       }
> > > > > +
> > > > > +       if (gen12_plane_has_mc_ccs(i915, plane_id))
> > > > > +               caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > > +
> > > > > +       return caps;
> > > > > +}
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > new file mode 100644
> > > > > index 000000000000..60a941c76f23
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > @@ -0,0 +1,14 @@
> > > > > +/* SPDX-License-Identifier: MIT */
> > > > > +/*
> > > > > + * Copyright © 2024 Intel Corporation
> > > > > + */
> > > > > +
> > > > > +#ifndef __INTEL_PLANE_CAPS_H__
> > > > > +#define __INTEL_PLANE_CAPS_H__
> > > > > +
> > > > > +#include "intel_display_types.h"
> > > > > +
> > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > > +                     enum pipe pipe, enum plane_id
> > > > > plane_id);
> > > > > +
> > > > > +#endif /* __INTEL_PLANE_CAPS_H__ */
> > > > > diff --git
> > > > > a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > index 511dc1544854..f2fd3833c61d 100644
> > > > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > @@ -17,6 +17,7 @@
> > > > >  #include "intel_fb.h"
> > > > >  #include "intel_fbc.h"
> > > > >  #include "intel_frontbuffer.h"
> > > > > +#include "intel_plane_caps.h"
> > > > >  #include "intel_psr.h"
> > > > >  #include "intel_psr_regs.h"
> > > > >  #include "skl_scaler.h"
> > > > > @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct
> > > > > intel_plane *plane)
> > > > >         spin_unlock_irq(&i915->irq_lock);
> > > > >  }
> > > > >  
> > > > > -static bool skl_plane_has_rc_ccs(struct drm_i915_private
> > > > > *i915,
> > > > > -                                enum pipe pipe, enum
> > > > > plane_id plane_id)
> > > > > -{
> > > > > -       /* Wa_22011186057 */
> > > > > -       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
> > > > > STEP_A0, STEP_B0))
> > > > > -               return false;
> > > > > -
> > > > > -       if (DISPLAY_VER(i915) >= 11)
> > > > > -               return true;
> > > > > -
> > > > > -       if (IS_GEMINILAKE(i915))
> > > > > -               return pipe != PIPE_C;
> > > > > -
> > > > > -       return pipe != PIPE_C &&
> > > > > -               (plane_id == PLANE_PRIMARY ||
> > > > > -                plane_id == PLANE_SPRITE0);
> > > > > -}
> > > > > -
> > > > > -static bool gen12_plane_has_mc_ccs(struct drm_i915_private
> > > > > *i915,
> > > > > -                                  enum plane_id plane_id)
> > > > > -{
> > > > > -       if (DISPLAY_VER(i915) < 12)
> > > > > -               return false;
> > > > > -
> > > > > -       /* Wa_14010477008 */
> > > > > -       if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > > > > -               (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915,
> > > > > STEP_A0, STEP_D0)))
> > > > > -               return false;
> > > > > -
> > > > > -       /* Wa_22011186057 */
> > > > > -       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
> > > > > STEP_A0, STEP_B0))
> > > > > -               return false;
> > > > > -
> > > > > -       return plane_id < PLANE_SPRITE4;
> > > > > -}
> > > > > -
> > > > > -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > > -                            enum pipe pipe, enum plane_id
> > > > > plane_id)
> > > > > -{
> > > > > -       u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > > -
> > > > > -       if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > > > -               caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > > -       if (DISPLAY_VER(i915) < 12)
> > > > > -               caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > > -       if (HAS_4TILE(i915))
> > > > > -               caps |= INTEL_PLANE_CAP_TILING_4;
> > > > > -
> > > > > -       if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> > > > > -               caps |= INTEL_PLANE_CAP_CCS_RC;
> > > > > -               if (DISPLAY_VER(i915) >= 12)
> > > > > -                       caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> > > > > -       }
> > > > > -
> > > > > -       if (gen12_plane_has_mc_ccs(i915, plane_id))
> > > > > -               caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > > -
> > > > > -       return caps;
> > > > > -}
> > > > > -
> > > > >  struct intel_plane *
> > > > >  skl_universal_plane_create(struct drm_i915_private
> > > > > *dev_priv,
> > > > >                            enum pipe pipe, enum plane_id
> > > > > plane_id)
> > > > > diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > index ccf83c12b545..425c6e6744a6 100644
> > > > > --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > @@ -15,6 +15,7 @@
> > > > >  #include "intel_fb.h"
> > > > >  #include "intel_fb_pin.h"
> > > > >  #include "intel_frontbuffer.h"
> > > > > +#include "intel_plane_caps.h"
> > > > >  #include "intel_plane_initial.h"
> > > > >  
> > > > >  static bool
> > > > > @@ -289,3 +290,25 @@ void
> > > > > intel_crtc_initial_plane_config(struct intel_crtc *crtc)
> > > > >  
> > > > >         plane_config_fini(&plane_config);
> > > > >  }
> > > > > +
> > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > > +                     enum pipe pipe, enum plane_id plane_id)
> > > > > +{
> > > > > +       u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > > +
> > > > > +       if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > > > +               caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > > +       if (DISPLAY_VER(i915) < 12)
> > > > > +               caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > > +       if (HAS_4TILE(i915))
> > > > > +               caps |= INTEL_PLANE_CAP_TILING_4;
> > > > > +
> > > > > +       if (HAS_FLAT_CCS(i915)) {
> > > > > +               caps |= INTEL_PLANE_CAP_CCS_RC |
> > > > > INTEL_PLANE_CAP_CCS_RC_CC;
> > > > > +
> > > > > +               if (plane_id < PLANE_SPRITE4)
> > > > > +                       caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > > +       }
> > > > > +
> > > > > +       return caps;
> > > > > +}
> > > > 
> > > > -- 
> > > > Jani Nikula, Intel
> >
Rodrigo Vivi Jan. 12, 2024, 9:16 p.m. UTC | #7
On Fri, Jan 12, 2024 at 07:31:51AM -0500, Hogander, Jouni wrote:
> On Wed, 2024-01-10 at 15:09 -0500, Rodrigo Vivi wrote:
> > On Tue, Jan 09, 2024 at 08:40:24PM +0000, Souza, Jose wrote:
> > > On Mon, 2024-01-08 at 17:18 -0500, Rodrigo Vivi wrote:
> > > > On Tue, Jan 02, 2024 at 09:44:48PM +0200, Jani Nikula wrote:
> > > > > On Tue, 02 Jan 2024, Juha-Pekka Heikkila
> > > > > <juhapekka.heikkila@gmail.com> wrote:
> > > > > > Aux ccs framebuffers don't work on Xe driver hence disable
> > > > > > them
> > > > > > from plane capabilities until they are fixed. Flat ccs
> > > > > > framebuffers
> > > > > > work and they are left enabled. Here is separated plane
> > > > > > capabilities
> > > > > > check on i915 so it can behave differencly depending on the
> > > > > > driver.
> > > > > 
> > > > > Cc: Rodrigo and xe maintainers
> > > > > 
> > > > > We need to figure out the proper workflow, the mailing lists to
> > > > > use, the
> > > > > subject prefix to use, the acks to require, etc, for changes
> > > > > touching
> > > > > both xe and i915.
> > > > > 
> > > > > I'd very much prefer changes to i915 display to be merged via
> > > > > drm-intel-next as always. For one thing, it'll take a while to
> > > > > sync
> > > > > stuff back from drm-xe-next to drm-intel-next, and most display
> > > > > development still happens on drm-intel-next.
> > > > 
> > > > I fully agree with you.
> > > > 
> > > > > 
> > > > > But this patch can't be applied to drm-intel-next, because xe
> > > > > doesn't
> > > > > even exist on drm-intel-next yet...
> > > > 
> > > > should we do a backmerge of drm-next already, or too early for
> > > > that?
> > > 
> > > Can we split it into 2 patches and merge it?
> > > This is necessary to fix Wayland compositors on ADL and newer.
> > 
> > we can do either:
> > 1. backmerge drm-next into drm-intel-next and merge this as is. (This
> > would be with
> > Jani)
> > 2. split in 2 patches, one for drm-intel-next and the other for drm-
> > xe-next. (This would
> > be with Jouni)
> > 3. merge this as is in drm-xe-next and deal with the conflicts in a
> > future backmerge.
> > Since this is mostly adding a new file I don't believe that it would
> > be a big deal.
> > (This would impact myself)
> > 
> > Since next round of drm-intel-next is mine, I'd be okay on handling
> > that and acking
> > this approach number 3. But before moving forward with this I'd like
> > to wait for
> > Jani's and Jouni's opinions.
> 
> I'm fine with approach number 3.

Jani, ack? or any plan to do a backmerge soon?

> 
> BR,
> 
> Jouni Högander
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > BR,
> > > > > Jani.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > Closes:
> > > > > > https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
> > > > > > Signed-off-by: Juha-Pekka Heikkila
> > > > > > <juhapekka.heikkila@gmail.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/Makefile                 |  1 +
> > > > > >  .../gpu/drm/i915/display/intel_plane_caps.c   | 68
> > > > > > +++++++++++++++++++
> > > > > >  .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
> > > > > >  .../drm/i915/display/skl_universal_plane.c    | 61 +--------
> > > > > > --------
> > > > > >  drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
> > > > > >  5 files changed, 107 insertions(+), 60 deletions(-)
> > > > > >  create mode 100644
> > > > > > drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > > >  create mode 100644
> > > > > > drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/Makefile
> > > > > > b/drivers/gpu/drm/i915/Makefile
> > > > > > index e777686190ca..c5e3c2dd0a01 100644
> > > > > > --- a/drivers/gpu/drm/i915/Makefile
> > > > > > +++ b/drivers/gpu/drm/i915/Makefile
> > > > > > @@ -302,6 +302,7 @@ i915-y += \
> > > > > >         display/intel_overlay.o \
> > > > > >         display/intel_pch_display.o \
> > > > > >         display/intel_pch_refclk.o \
> > > > > > +       display/intel_plane_caps.o \
> > > > > >         display/intel_plane_initial.o \
> > > > > >         display/intel_pmdemand.o \
> > > > > >         display/intel_psr.o \
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..6206ae11f296
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > > > @@ -0,0 +1,68 @@
> > > > > > +// SPDX-License-Identifier: MIT
> > > > > > +/*
> > > > > > + * Copyright © 2024 Intel Corporation
> > > > > > + */
> > > > > > +
> > > > > > +#include "i915_drv.h"
> > > > > > +#include "intel_fb.h"
> > > > > > +#include "intel_plane_caps.h"
> > > > > > +
> > > > > > +static bool skl_plane_has_rc_ccs(struct drm_i915_private
> > > > > > *i915,
> > > > > > +                                enum pipe pipe, enum
> > > > > > plane_id plane_id)
> > > > > > +{
> > > > > > +       /* Wa_22011186057 */
> > > > > > +       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
> > > > > > STEP_A0, STEP_B0))
> > > > > > +               return false;
> > > > > > +
> > > > > > +       if (DISPLAY_VER(i915) >= 11)
> > > > > > +               return true;
> > > > > > +
> > > > > > +       if (IS_GEMINILAKE(i915))
> > > > > > +               return pipe != PIPE_C;
> > > > > > +
> > > > > > +       return pipe != PIPE_C &&
> > > > > > +               (plane_id == PLANE_PRIMARY ||
> > > > > > +                plane_id == PLANE_SPRITE0);
> > > > > > +}
> > > > > > +
> > > > > > +static bool gen12_plane_has_mc_ccs(struct drm_i915_private
> > > > > > *i915,
> > > > > > +                                  enum plane_id plane_id)
> > > > > > +{
> > > > > > +       if (DISPLAY_VER(i915) < 12)
> > > > > > +               return false;
> > > > > > +
> > > > > > +       /* Wa_14010477008 */
> > > > > > +       if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > > > > > +           (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915,
> > > > > > STEP_A0, STEP_D0)))
> > > > > > +               return false;
> > > > > > +
> > > > > > +       /* Wa_22011186057 */
> > > > > > +       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
> > > > > > STEP_A0, STEP_B0))
> > > > > > +               return false;
> > > > > > +
> > > > > > +       return plane_id < PLANE_SPRITE4;
> > > > > > +}
> > > > > > +
> > > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > > > +                     enum pipe pipe, enum plane_id plane_id)
> > > > > > +{
> > > > > > +       u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > > > +
> > > > > > +       if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > > > +       if (DISPLAY_VER(i915) < 12)
> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > > > +       if (HAS_4TILE(i915))
> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_4;
> > > > > > +
> > > > > > +       if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> > > > > > +               caps |= INTEL_PLANE_CAP_CCS_RC;
> > > > > > +               if (DISPLAY_VER(i915) >= 12)
> > > > > > +                       caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> > > > > > +       }
> > > > > > +
> > > > > > +       if (gen12_plane_has_mc_ccs(i915, plane_id))
> > > > > > +               caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > > > +
> > > > > > +       return caps;
> > > > > > +}
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > > b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > > new file mode 100644
> > > > > > index 000000000000..60a941c76f23
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > > @@ -0,0 +1,14 @@
> > > > > > +/* SPDX-License-Identifier: MIT */
> > > > > > +/*
> > > > > > + * Copyright © 2024 Intel Corporation
> > > > > > + */
> > > > > > +
> > > > > > +#ifndef __INTEL_PLANE_CAPS_H__
> > > > > > +#define __INTEL_PLANE_CAPS_H__
> > > > > > +
> > > > > > +#include "intel_display_types.h"
> > > > > > +
> > > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > > > +                     enum pipe pipe, enum plane_id
> > > > > > plane_id);
> > > > > > +
> > > > > > +#endif /* __INTEL_PLANE_CAPS_H__ */
> > > > > > diff --git
> > > > > > a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > > index 511dc1544854..f2fd3833c61d 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > > @@ -17,6 +17,7 @@
> > > > > >  #include "intel_fb.h"
> > > > > >  #include "intel_fbc.h"
> > > > > >  #include "intel_frontbuffer.h"
> > > > > > +#include "intel_plane_caps.h"
> > > > > >  #include "intel_psr.h"
> > > > > >  #include "intel_psr_regs.h"
> > > > > >  #include "skl_scaler.h"
> > > > > > @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct
> > > > > > intel_plane *plane)
> > > > > >         spin_unlock_irq(&i915->irq_lock);
> > > > > >  }
> > > > > >  
> > > > > > -static bool skl_plane_has_rc_ccs(struct drm_i915_private
> > > > > > *i915,
> > > > > > -                                enum pipe pipe, enum
> > > > > > plane_id plane_id)
> > > > > > -{
> > > > > > -       /* Wa_22011186057 */
> > > > > > -       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
> > > > > > STEP_A0, STEP_B0))
> > > > > > -               return false;
> > > > > > -
> > > > > > -       if (DISPLAY_VER(i915) >= 11)
> > > > > > -               return true;
> > > > > > -
> > > > > > -       if (IS_GEMINILAKE(i915))
> > > > > > -               return pipe != PIPE_C;
> > > > > > -
> > > > > > -       return pipe != PIPE_C &&
> > > > > > -               (plane_id == PLANE_PRIMARY ||
> > > > > > -                plane_id == PLANE_SPRITE0);
> > > > > > -}
> > > > > > -
> > > > > > -static bool gen12_plane_has_mc_ccs(struct drm_i915_private
> > > > > > *i915,
> > > > > > -                                  enum plane_id plane_id)
> > > > > > -{
> > > > > > -       if (DISPLAY_VER(i915) < 12)
> > > > > > -               return false;
> > > > > > -
> > > > > > -       /* Wa_14010477008 */
> > > > > > -       if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > > > > > -               (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915,
> > > > > > STEP_A0, STEP_D0)))
> > > > > > -               return false;
> > > > > > -
> > > > > > -       /* Wa_22011186057 */
> > > > > > -       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
> > > > > > STEP_A0, STEP_B0))
> > > > > > -               return false;
> > > > > > -
> > > > > > -       return plane_id < PLANE_SPRITE4;
> > > > > > -}
> > > > > > -
> > > > > > -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > > > -                            enum pipe pipe, enum plane_id
> > > > > > plane_id)
> > > > > > -{
> > > > > > -       u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > > > -
> > > > > > -       if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > > > > -               caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > > > -       if (DISPLAY_VER(i915) < 12)
> > > > > > -               caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > > > -       if (HAS_4TILE(i915))
> > > > > > -               caps |= INTEL_PLANE_CAP_TILING_4;
> > > > > > -
> > > > > > -       if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> > > > > > -               caps |= INTEL_PLANE_CAP_CCS_RC;
> > > > > > -               if (DISPLAY_VER(i915) >= 12)
> > > > > > -                       caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> > > > > > -       }
> > > > > > -
> > > > > > -       if (gen12_plane_has_mc_ccs(i915, plane_id))
> > > > > > -               caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > > > -
> > > > > > -       return caps;
> > > > > > -}
> > > > > > -
> > > > > >  struct intel_plane *
> > > > > >  skl_universal_plane_create(struct drm_i915_private
> > > > > > *dev_priv,
> > > > > >                            enum pipe pipe, enum plane_id
> > > > > > plane_id)
> > > > > > diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > > b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > > index ccf83c12b545..425c6e6744a6 100644
> > > > > > --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > > +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > > @@ -15,6 +15,7 @@
> > > > > >  #include "intel_fb.h"
> > > > > >  #include "intel_fb_pin.h"
> > > > > >  #include "intel_frontbuffer.h"
> > > > > > +#include "intel_plane_caps.h"
> > > > > >  #include "intel_plane_initial.h"
> > > > > >  
> > > > > >  static bool
> > > > > > @@ -289,3 +290,25 @@ void
> > > > > > intel_crtc_initial_plane_config(struct intel_crtc *crtc)
> > > > > >  
> > > > > >         plane_config_fini(&plane_config);
> > > > > >  }
> > > > > > +
> > > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > > > +                     enum pipe pipe, enum plane_id plane_id)
> > > > > > +{
> > > > > > +       u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > > > +
> > > > > > +       if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > > > +       if (DISPLAY_VER(i915) < 12)
> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > > > +       if (HAS_4TILE(i915))
> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_4;
> > > > > > +
> > > > > > +       if (HAS_FLAT_CCS(i915)) {
> > > > > > +               caps |= INTEL_PLANE_CAP_CCS_RC |
> > > > > > INTEL_PLANE_CAP_CCS_RC_CC;
> > > > > > +
> > > > > > +               if (plane_id < PLANE_SPRITE4)
> > > > > > +                       caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > > > +       }
> > > > > > +
> > > > > > +       return caps;
> > > > > > +}
> > > > > 
> > > > > -- 
> > > > > Jani Nikula, Intel
> > > 
>
Jani Nikula Jan. 15, 2024, 1 p.m. UTC | #8
On Fri, 12 Jan 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Fri, Jan 12, 2024 at 07:31:51AM -0500, Hogander, Jouni wrote:
>> On Wed, 2024-01-10 at 15:09 -0500, Rodrigo Vivi wrote:
>> > On Tue, Jan 09, 2024 at 08:40:24PM +0000, Souza, Jose wrote:
>> > > On Mon, 2024-01-08 at 17:18 -0500, Rodrigo Vivi wrote:
>> > > > On Tue, Jan 02, 2024 at 09:44:48PM +0200, Jani Nikula wrote:
>> > > > > On Tue, 02 Jan 2024, Juha-Pekka Heikkila
>> > > > > <juhapekka.heikkila@gmail.com> wrote:
>> > > > > > Aux ccs framebuffers don't work on Xe driver hence disable
>> > > > > > them
>> > > > > > from plane capabilities until they are fixed. Flat ccs
>> > > > > > framebuffers
>> > > > > > work and they are left enabled. Here is separated plane
>> > > > > > capabilities
>> > > > > > check on i915 so it can behave differencly depending on the
>> > > > > > driver.
>> > > > > 
>> > > > > Cc: Rodrigo and xe maintainers
>> > > > > 
>> > > > > We need to figure out the proper workflow, the mailing lists to
>> > > > > use, the
>> > > > > subject prefix to use, the acks to require, etc, for changes
>> > > > > touching
>> > > > > both xe and i915.
>> > > > > 
>> > > > > I'd very much prefer changes to i915 display to be merged via
>> > > > > drm-intel-next as always. For one thing, it'll take a while to
>> > > > > sync
>> > > > > stuff back from drm-xe-next to drm-intel-next, and most display
>> > > > > development still happens on drm-intel-next.
>> > > > 
>> > > > I fully agree with you.
>> > > > 
>> > > > > 
>> > > > > But this patch can't be applied to drm-intel-next, because xe
>> > > > > doesn't
>> > > > > even exist on drm-intel-next yet...
>> > > > 
>> > > > should we do a backmerge of drm-next already, or too early for
>> > > > that?
>> > > 
>> > > Can we split it into 2 patches and merge it?
>> > > This is necessary to fix Wayland compositors on ADL and newer.
>> > 
>> > we can do either:
>> > 1. backmerge drm-next into drm-intel-next and merge this as is. (This
>> > would be with
>> > Jani)
>> > 2. split in 2 patches, one for drm-intel-next and the other for drm-
>> > xe-next. (This would
>> > be with Jouni)
>> > 3. merge this as is in drm-xe-next and deal with the conflicts in a
>> > future backmerge.
>> > Since this is mostly adding a new file I don't believe that it would
>> > be a big deal.
>> > (This would impact myself)
>> > 
>> > Since next round of drm-intel-next is mine, I'd be okay on handling
>> > that and acking
>> > this approach number 3. But before moving forward with this I'd like
>> > to wait for
>> > Jani's and Jouni's opinions.
>> 
>> I'm fine with approach number 3.
>
> Jani, ack? or any plan to do a backmerge soon?

I've done the backmerge, but considering this touches i915 code much
more than xe, I'd prefer this get applied via i915. Or split.

BR,
Jani.

>
>> 
>> BR,
>> 
>> Jouni Högander
>> 
>> > 
>> > > 
>> > > > 
>> > > > > 
>> > > > > 
>> > > > > BR,
>> > > > > Jani.
>> > > > > 
>> > > > > 
>> > > > > > 
>> > > > > > Closes:
>> > > > > > https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
>> > > > > > Signed-off-by: Juha-Pekka Heikkila
>> > > > > > <juhapekka.heikkila@gmail.com>
>> > > > > > ---
>> > > > > >  drivers/gpu/drm/i915/Makefile                 |  1 +
>> > > > > >  .../gpu/drm/i915/display/intel_plane_caps.c   | 68
>> > > > > > +++++++++++++++++++
>> > > > > >  .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
>> > > > > >  .../drm/i915/display/skl_universal_plane.c    | 61 +--------
>> > > > > > --------
>> > > > > >  drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
>> > > > > >  5 files changed, 107 insertions(+), 60 deletions(-)
>> > > > > >  create mode 100644
>> > > > > > drivers/gpu/drm/i915/display/intel_plane_caps.c
>> > > > > >  create mode 100644
>> > > > > > drivers/gpu/drm/i915/display/intel_plane_caps.h
>> > > > > > 
>> > > > > > diff --git a/drivers/gpu/drm/i915/Makefile
>> > > > > > b/drivers/gpu/drm/i915/Makefile
>> > > > > > index e777686190ca..c5e3c2dd0a01 100644
>> > > > > > --- a/drivers/gpu/drm/i915/Makefile
>> > > > > > +++ b/drivers/gpu/drm/i915/Makefile
>> > > > > > @@ -302,6 +302,7 @@ i915-y += \
>> > > > > >         display/intel_overlay.o \
>> > > > > >         display/intel_pch_display.o \
>> > > > > >         display/intel_pch_refclk.o \
>> > > > > > +       display/intel_plane_caps.o \
>> > > > > >         display/intel_plane_initial.o \
>> > > > > >         display/intel_pmdemand.o \
>> > > > > >         display/intel_psr.o \
>> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c
>> > > > > > b/drivers/gpu/drm/i915/display/intel_plane_caps.c
>> > > > > > new file mode 100644
>> > > > > > index 000000000000..6206ae11f296
>> > > > > > --- /dev/null
>> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
>> > > > > > @@ -0,0 +1,68 @@
>> > > > > > +// SPDX-License-Identifier: MIT
>> > > > > > +/*
>> > > > > > + * Copyright © 2024 Intel Corporation
>> > > > > > + */
>> > > > > > +
>> > > > > > +#include "i915_drv.h"
>> > > > > > +#include "intel_fb.h"
>> > > > > > +#include "intel_plane_caps.h"
>> > > > > > +
>> > > > > > +static bool skl_plane_has_rc_ccs(struct drm_i915_private
>> > > > > > *i915,
>> > > > > > +                                enum pipe pipe, enum
>> > > > > > plane_id plane_id)
>> > > > > > +{
>> > > > > > +       /* Wa_22011186057 */
>> > > > > > +       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
>> > > > > > STEP_A0, STEP_B0))
>> > > > > > +               return false;
>> > > > > > +
>> > > > > > +       if (DISPLAY_VER(i915) >= 11)
>> > > > > > +               return true;
>> > > > > > +
>> > > > > > +       if (IS_GEMINILAKE(i915))
>> > > > > > +               return pipe != PIPE_C;
>> > > > > > +
>> > > > > > +       return pipe != PIPE_C &&
>> > > > > > +               (plane_id == PLANE_PRIMARY ||
>> > > > > > +                plane_id == PLANE_SPRITE0);
>> > > > > > +}
>> > > > > > +
>> > > > > > +static bool gen12_plane_has_mc_ccs(struct drm_i915_private
>> > > > > > *i915,
>> > > > > > +                                  enum plane_id plane_id)
>> > > > > > +{
>> > > > > > +       if (DISPLAY_VER(i915) < 12)
>> > > > > > +               return false;
>> > > > > > +
>> > > > > > +       /* Wa_14010477008 */
>> > > > > > +       if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
>> > > > > > +           (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915,
>> > > > > > STEP_A0, STEP_D0)))
>> > > > > > +               return false;
>> > > > > > +
>> > > > > > +       /* Wa_22011186057 */
>> > > > > > +       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
>> > > > > > STEP_A0, STEP_B0))
>> > > > > > +               return false;
>> > > > > > +
>> > > > > > +       return plane_id < PLANE_SPRITE4;
>> > > > > > +}
>> > > > > > +
>> > > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> > > > > > +                     enum pipe pipe, enum plane_id plane_id)
>> > > > > > +{
>> > > > > > +       u8 caps = INTEL_PLANE_CAP_TILING_X;
>> > > > > > +
>> > > > > > +       if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Y;
>> > > > > > +       if (DISPLAY_VER(i915) < 12)
>> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Yf;
>> > > > > > +       if (HAS_4TILE(i915))
>> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_4;
>> > > > > > +
>> > > > > > +       if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>> > > > > > +               caps |= INTEL_PLANE_CAP_CCS_RC;
>> > > > > > +               if (DISPLAY_VER(i915) >= 12)
>> > > > > > +                       caps |= INTEL_PLANE_CAP_CCS_RC_CC;
>> > > > > > +       }
>> > > > > > +
>> > > > > > +       if (gen12_plane_has_mc_ccs(i915, plane_id))
>> > > > > > +               caps |= INTEL_PLANE_CAP_CCS_MC;
>> > > > > > +
>> > > > > > +       return caps;
>> > > > > > +}
>> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h
>> > > > > > b/drivers/gpu/drm/i915/display/intel_plane_caps.h
>> > > > > > new file mode 100644
>> > > > > > index 000000000000..60a941c76f23
>> > > > > > --- /dev/null
>> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
>> > > > > > @@ -0,0 +1,14 @@
>> > > > > > +/* SPDX-License-Identifier: MIT */
>> > > > > > +/*
>> > > > > > + * Copyright © 2024 Intel Corporation
>> > > > > > + */
>> > > > > > +
>> > > > > > +#ifndef __INTEL_PLANE_CAPS_H__
>> > > > > > +#define __INTEL_PLANE_CAPS_H__
>> > > > > > +
>> > > > > > +#include "intel_display_types.h"
>> > > > > > +
>> > > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> > > > > > +                     enum pipe pipe, enum plane_id
>> > > > > > plane_id);
>> > > > > > +
>> > > > > > +#endif /* __INTEL_PLANE_CAPS_H__ */
>> > > > > > diff --git
>> > > > > > a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > > > > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > > > > > index 511dc1544854..f2fd3833c61d 100644
>> > > > > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > > > > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> > > > > > @@ -17,6 +17,7 @@
>> > > > > >  #include "intel_fb.h"
>> > > > > >  #include "intel_fbc.h"
>> > > > > >  #include "intel_frontbuffer.h"
>> > > > > > +#include "intel_plane_caps.h"
>> > > > > >  #include "intel_psr.h"
>> > > > > >  #include "intel_psr_regs.h"
>> > > > > >  #include "skl_scaler.h"
>> > > > > > @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct
>> > > > > > intel_plane *plane)
>> > > > > >         spin_unlock_irq(&i915->irq_lock);
>> > > > > >  }
>> > > > > >  
>> > > > > > -static bool skl_plane_has_rc_ccs(struct drm_i915_private
>> > > > > > *i915,
>> > > > > > -                                enum pipe pipe, enum
>> > > > > > plane_id plane_id)
>> > > > > > -{
>> > > > > > -       /* Wa_22011186057 */
>> > > > > > -       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
>> > > > > > STEP_A0, STEP_B0))
>> > > > > > -               return false;
>> > > > > > -
>> > > > > > -       if (DISPLAY_VER(i915) >= 11)
>> > > > > > -               return true;
>> > > > > > -
>> > > > > > -       if (IS_GEMINILAKE(i915))
>> > > > > > -               return pipe != PIPE_C;
>> > > > > > -
>> > > > > > -       return pipe != PIPE_C &&
>> > > > > > -               (plane_id == PLANE_PRIMARY ||
>> > > > > > -                plane_id == PLANE_SPRITE0);
>> > > > > > -}
>> > > > > > -
>> > > > > > -static bool gen12_plane_has_mc_ccs(struct drm_i915_private
>> > > > > > *i915,
>> > > > > > -                                  enum plane_id plane_id)
>> > > > > > -{
>> > > > > > -       if (DISPLAY_VER(i915) < 12)
>> > > > > > -               return false;
>> > > > > > -
>> > > > > > -       /* Wa_14010477008 */
>> > > > > > -       if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
>> > > > > > -               (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915,
>> > > > > > STEP_A0, STEP_D0)))
>> > > > > > -               return false;
>> > > > > > -
>> > > > > > -       /* Wa_22011186057 */
>> > > > > > -       if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915,
>> > > > > > STEP_A0, STEP_B0))
>> > > > > > -               return false;
>> > > > > > -
>> > > > > > -       return plane_id < PLANE_SPRITE4;
>> > > > > > -}
>> > > > > > -
>> > > > > > -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> > > > > > -                            enum pipe pipe, enum plane_id
>> > > > > > plane_id)
>> > > > > > -{
>> > > > > > -       u8 caps = INTEL_PLANE_CAP_TILING_X;
>> > > > > > -
>> > > > > > -       if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>> > > > > > -               caps |= INTEL_PLANE_CAP_TILING_Y;
>> > > > > > -       if (DISPLAY_VER(i915) < 12)
>> > > > > > -               caps |= INTEL_PLANE_CAP_TILING_Yf;
>> > > > > > -       if (HAS_4TILE(i915))
>> > > > > > -               caps |= INTEL_PLANE_CAP_TILING_4;
>> > > > > > -
>> > > > > > -       if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>> > > > > > -               caps |= INTEL_PLANE_CAP_CCS_RC;
>> > > > > > -               if (DISPLAY_VER(i915) >= 12)
>> > > > > > -                       caps |= INTEL_PLANE_CAP_CCS_RC_CC;
>> > > > > > -       }
>> > > > > > -
>> > > > > > -       if (gen12_plane_has_mc_ccs(i915, plane_id))
>> > > > > > -               caps |= INTEL_PLANE_CAP_CCS_MC;
>> > > > > > -
>> > > > > > -       return caps;
>> > > > > > -}
>> > > > > > -
>> > > > > >  struct intel_plane *
>> > > > > >  skl_universal_plane_create(struct drm_i915_private
>> > > > > > *dev_priv,
>> > > > > >                            enum pipe pipe, enum plane_id
>> > > > > > plane_id)
>> > > > > > diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> > > > > > b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> > > > > > index ccf83c12b545..425c6e6744a6 100644
>> > > > > > --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> > > > > > +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> > > > > > @@ -15,6 +15,7 @@
>> > > > > >  #include "intel_fb.h"
>> > > > > >  #include "intel_fb_pin.h"
>> > > > > >  #include "intel_frontbuffer.h"
>> > > > > > +#include "intel_plane_caps.h"
>> > > > > >  #include "intel_plane_initial.h"
>> > > > > >  
>> > > > > >  static bool
>> > > > > > @@ -289,3 +290,25 @@ void
>> > > > > > intel_crtc_initial_plane_config(struct intel_crtc *crtc)
>> > > > > >  
>> > > > > >         plane_config_fini(&plane_config);
>> > > > > >  }
>> > > > > > +
>> > > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> > > > > > +                     enum pipe pipe, enum plane_id plane_id)
>> > > > > > +{
>> > > > > > +       u8 caps = INTEL_PLANE_CAP_TILING_X;
>> > > > > > +
>> > > > > > +       if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Y;
>> > > > > > +       if (DISPLAY_VER(i915) < 12)
>> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Yf;
>> > > > > > +       if (HAS_4TILE(i915))
>> > > > > > +               caps |= INTEL_PLANE_CAP_TILING_4;
>> > > > > > +
>> > > > > > +       if (HAS_FLAT_CCS(i915)) {
>> > > > > > +               caps |= INTEL_PLANE_CAP_CCS_RC |
>> > > > > > INTEL_PLANE_CAP_CCS_RC_CC;
>> > > > > > +
>> > > > > > +               if (plane_id < PLANE_SPRITE4)
>> > > > > > +                       caps |= INTEL_PLANE_CAP_CCS_MC;
>> > > > > > +       }
>> > > > > > +
>> > > > > > +       return caps;
>> > > > > > +}
>> > > > > 
>> > > > > -- 
>> > > > > Jani Nikula, Intel
>> > > 
>>
Hellstrom, Thomas Jan. 15, 2024, 3:05 p.m. UTC | #9
On Mon, 2024-01-15 at 15:00 +0200, Jani Nikula wrote:
> On Fri, 12 Jan 2024, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Fri, Jan 12, 2024 at 07:31:51AM -0500, Hogander, Jouni wrote:
> > > On Wed, 2024-01-10 at 15:09 -0500, Rodrigo Vivi wrote:
> > > > On Tue, Jan 09, 2024 at 08:40:24PM +0000, Souza, Jose wrote:
> > > > > On Mon, 2024-01-08 at 17:18 -0500, Rodrigo Vivi wrote:
> > > > > > On Tue, Jan 02, 2024 at 09:44:48PM +0200, Jani Nikula
> > > > > > wrote:
> > > > > > > On Tue, 02 Jan 2024, Juha-Pekka Heikkila
> > > > > > > <juhapekka.heikkila@gmail.com> wrote:
> > > > > > > > Aux ccs framebuffers don't work on Xe driver hence
> > > > > > > > disable
> > > > > > > > them
> > > > > > > > from plane capabilities until they are fixed. Flat ccs
> > > > > > > > framebuffers
> > > > > > > > work and they are left enabled. Here is separated plane
> > > > > > > > capabilities
> > > > > > > > check on i915 so it can behave differencly depending on
> > > > > > > > the
> > > > > > > > driver.
> > > > > > > 
> > > > > > > Cc: Rodrigo and xe maintainers
> > > > > > > 
> > > > > > > We need to figure out the proper workflow, the mailing
> > > > > > > lists to
> > > > > > > use, the
> > > > > > > subject prefix to use, the acks to require, etc, for
> > > > > > > changes
> > > > > > > touching
> > > > > > > both xe and i915.
> > > > > > > 
> > > > > > > I'd very much prefer changes to i915 display to be merged
> > > > > > > via
> > > > > > > drm-intel-next as always. For one thing, it'll take a
> > > > > > > while to
> > > > > > > sync
> > > > > > > stuff back from drm-xe-next to drm-intel-next, and most
> > > > > > > display
> > > > > > > development still happens on drm-intel-next.
> > > > > > 
> > > > > > I fully agree with you.
> > > > > > 
> > > > > > > 
> > > > > > > But this patch can't be applied to drm-intel-next,
> > > > > > > because xe
> > > > > > > doesn't
> > > > > > > even exist on drm-intel-next yet...
> > > > > > 
> > > > > > should we do a backmerge of drm-next already, or too early
> > > > > > for
> > > > > > that?
> > > > > 
> > > > > Can we split it into 2 patches and merge it?
> > > > > This is necessary to fix Wayland compositors on ADL and
> > > > > newer.
> > > > 
> > > > we can do either:
> > > > 1. backmerge drm-next into drm-intel-next and merge this as is.
> > > > (This
> > > > would be with
> > > > Jani)
> > > > 2. split in 2 patches, one for drm-intel-next and the other for
> > > > drm-
> > > > xe-next. (This would
> > > > be with Jouni)
> > > > 3. merge this as is in drm-xe-next and deal with the conflicts
> > > > in a
> > > > future backmerge.
> > > > Since this is mostly adding a new file I don't believe that it
> > > > would
> > > > be a big deal.
> > > > (This would impact myself)
> > > > 
> > > > Since next round of drm-intel-next is mine, I'd be okay on
> > > > handling
> > > > that and acking
> > > > this approach number 3. But before moving forward with this I'd
> > > > like
> > > > to wait for
> > > > Jani's and Jouni's opinions.
> > > 
> > > I'm fine with approach number 3.
> > 
> > Jani, ack? or any plan to do a backmerge soon?
> 
> I've done the backmerge, but considering this touches i915 code much
> more than xe, I'd prefer this get applied via i915. Or split.
> 
> BR,
> Jani.

I wonder if, moving forward, we should use a topic/display-for-xe
branch (no force-push) that can be used for display commits that need
to be in Xe during -next cycles. It could then be merged into xe and
i915 as needed?

/Thomas




> 
> > 
> > > 
> > > BR,
> > > 
> > > Jouni Högander
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > BR,
> > > > > > > Jani.
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > Closes:
> > > > > > > > https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
> > > > > > > > Signed-off-by: Juha-Pekka Heikkila
> > > > > > > > <juhapekka.heikkila@gmail.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/Makefile                 |  1 +
> > > > > > > >  .../gpu/drm/i915/display/intel_plane_caps.c   | 68
> > > > > > > > +++++++++++++++++++
> > > > > > > >  .../gpu/drm/i915/display/intel_plane_caps.h   | 14
> > > > > > > > ++++
> > > > > > > >  .../drm/i915/display/skl_universal_plane.c    | 61 +--
> > > > > > > > ------
> > > > > > > > --------
> > > > > > > >  drivers/gpu/drm/xe/display/xe_plane_initial.c | 23
> > > > > > > > +++++++
> > > > > > > >  5 files changed, 107 insertions(+), 60 deletions(-)
> > > > > > > >  create mode 100644
> > > > > > > > drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > > > > >  create mode 100644
> > > > > > > > drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/Makefile
> > > > > > > > b/drivers/gpu/drm/i915/Makefile
> > > > > > > > index e777686190ca..c5e3c2dd0a01 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/Makefile
> > > > > > > > +++ b/drivers/gpu/drm/i915/Makefile
> > > > > > > > @@ -302,6 +302,7 @@ i915-y += \
> > > > > > > >         display/intel_overlay.o \
> > > > > > > >         display/intel_pch_display.o \
> > > > > > > >         display/intel_pch_refclk.o \
> > > > > > > > +       display/intel_plane_caps.o \
> > > > > > > >         display/intel_plane_initial.o \
> > > > > > > >         display/intel_pmdemand.o \
> > > > > > > >         display/intel_psr.o \
> > > > > > > > diff --git
> > > > > > > > a/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > > > > > b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..6206ae11f296
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> > > > > > > > @@ -0,0 +1,68 @@
> > > > > > > > +// SPDX-License-Identifier: MIT
> > > > > > > > +/*
> > > > > > > > + * Copyright © 2024 Intel Corporation
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include "i915_drv.h"
> > > > > > > > +#include "intel_fb.h"
> > > > > > > > +#include "intel_plane_caps.h"
> > > > > > > > +
> > > > > > > > +static bool skl_plane_has_rc_ccs(struct
> > > > > > > > drm_i915_private
> > > > > > > > *i915,
> > > > > > > > +                                enum pipe pipe, enum
> > > > > > > > plane_id plane_id)
> > > > > > > > +{
> > > > > > > > +       /* Wa_22011186057 */
> > > > > > > > +       if (IS_ALDERLAKE_P(i915) &&
> > > > > > > > IS_DISPLAY_STEP(i915,
> > > > > > > > STEP_A0, STEP_B0))
> > > > > > > > +               return false;
> > > > > > > > +
> > > > > > > > +       if (DISPLAY_VER(i915) >= 11)
> > > > > > > > +               return true;
> > > > > > > > +
> > > > > > > > +       if (IS_GEMINILAKE(i915))
> > > > > > > > +               return pipe != PIPE_C;
> > > > > > > > +
> > > > > > > > +       return pipe != PIPE_C &&
> > > > > > > > +               (plane_id == PLANE_PRIMARY ||
> > > > > > > > +                plane_id == PLANE_SPRITE0);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static bool gen12_plane_has_mc_ccs(struct
> > > > > > > > drm_i915_private
> > > > > > > > *i915,
> > > > > > > > +                                  enum plane_id
> > > > > > > > plane_id)
> > > > > > > > +{
> > > > > > > > +       if (DISPLAY_VER(i915) < 12)
> > > > > > > > +               return false;
> > > > > > > > +
> > > > > > > > +       /* Wa_14010477008 */
> > > > > > > > +       if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > > > > > > > +           (IS_TIGERLAKE(i915) &&
> > > > > > > > IS_DISPLAY_STEP(i915,
> > > > > > > > STEP_A0, STEP_D0)))
> > > > > > > > +               return false;
> > > > > > > > +
> > > > > > > > +       /* Wa_22011186057 */
> > > > > > > > +       if (IS_ALDERLAKE_P(i915) &&
> > > > > > > > IS_DISPLAY_STEP(i915,
> > > > > > > > STEP_A0, STEP_B0))
> > > > > > > > +               return false;
> > > > > > > > +
> > > > > > > > +       return plane_id < PLANE_SPRITE4;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > > > > > +                     enum pipe pipe, enum plane_id
> > > > > > > > plane_id)
> > > > > > > > +{
> > > > > > > > +       u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > > > > > +
> > > > > > > > +       if (DISPLAY_VER(i915) < 13 ||
> > > > > > > > IS_ALDERLAKE_P(i915))
> > > > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > > > > > +       if (DISPLAY_VER(i915) < 12)
> > > > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > > > > > +       if (HAS_4TILE(i915))
> > > > > > > > +               caps |= INTEL_PLANE_CAP_TILING_4;
> > > > > > > > +
> > > > > > > > +       if (skl_plane_has_rc_ccs(i915, pipe, plane_id))
> > > > > > > > {
> > > > > > > > +               caps |= INTEL_PLANE_CAP_CCS_RC;
> > > > > > > > +               if (DISPLAY_VER(i915) >= 12)
> > > > > > > > +                       caps |=
> > > > > > > > INTEL_PLANE_CAP_CCS_RC_CC;
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       if (gen12_plane_has_mc_ccs(i915, plane_id))
> > > > > > > > +               caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > > > > > +
> > > > > > > > +       return caps;
> > > > > > > > +}
> > > > > > > > diff --git
> > > > > > > > a/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > > > > b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..60a941c76f23
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> > > > > > > > @@ -0,0 +1,14 @@
> > > > > > > > +/* SPDX-License-Identifier: MIT */
> > > > > > > > +/*
> > > > > > > > + * Copyright © 2024 Intel Corporation
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#ifndef __INTEL_PLANE_CAPS_H__
> > > > > > > > +#define __INTEL_PLANE_CAPS_H__
> > > > > > > > +
> > > > > > > > +#include "intel_display_types.h"
> > > > > > > > +
> > > > > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > > > > > +                     enum pipe pipe, enum plane_id
> > > > > > > > plane_id);
> > > > > > > > +
> > > > > > > > +#endif /* __INTEL_PLANE_CAPS_H__ */
> > > > > > > > diff --git
> > > > > > > > a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > > > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > > > > index 511dc1544854..f2fd3833c61d 100644
> > > > > > > > ---
> > > > > > > > a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > > > > +++
> > > > > > > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > > > > > > @@ -17,6 +17,7 @@
> > > > > > > >  #include "intel_fb.h"
> > > > > > > >  #include "intel_fbc.h"
> > > > > > > >  #include "intel_frontbuffer.h"
> > > > > > > > +#include "intel_plane_caps.h"
> > > > > > > >  #include "intel_psr.h"
> > > > > > > >  #include "intel_psr_regs.h"
> > > > > > > >  #include "skl_scaler.h"
> > > > > > > > @@ -2242,66 +2243,6 @@
> > > > > > > > skl_plane_disable_flip_done(struct
> > > > > > > > intel_plane *plane)
> > > > > > > >         spin_unlock_irq(&i915->irq_lock);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > -static bool skl_plane_has_rc_ccs(struct
> > > > > > > > drm_i915_private
> > > > > > > > *i915,
> > > > > > > > -                                enum pipe pipe, enum
> > > > > > > > plane_id plane_id)
> > > > > > > > -{
> > > > > > > > -       /* Wa_22011186057 */
> > > > > > > > -       if (IS_ALDERLAKE_P(i915) &&
> > > > > > > > IS_DISPLAY_STEP(i915,
> > > > > > > > STEP_A0, STEP_B0))
> > > > > > > > -               return false;
> > > > > > > > -
> > > > > > > > -       if (DISPLAY_VER(i915) >= 11)
> > > > > > > > -               return true;
> > > > > > > > -
> > > > > > > > -       if (IS_GEMINILAKE(i915))
> > > > > > > > -               return pipe != PIPE_C;
> > > > > > > > -
> > > > > > > > -       return pipe != PIPE_C &&
> > > > > > > > -               (plane_id == PLANE_PRIMARY ||
> > > > > > > > -                plane_id == PLANE_SPRITE0);
> > > > > > > > -}
> > > > > > > > -
> > > > > > > > -static bool gen12_plane_has_mc_ccs(struct
> > > > > > > > drm_i915_private
> > > > > > > > *i915,
> > > > > > > > -                                  enum plane_id
> > > > > > > > plane_id)
> > > > > > > > -{
> > > > > > > > -       if (DISPLAY_VER(i915) < 12)
> > > > > > > > -               return false;
> > > > > > > > -
> > > > > > > > -       /* Wa_14010477008 */
> > > > > > > > -       if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> > > > > > > > -               (IS_TIGERLAKE(i915) &&
> > > > > > > > IS_DISPLAY_STEP(i915,
> > > > > > > > STEP_A0, STEP_D0)))
> > > > > > > > -               return false;
> > > > > > > > -
> > > > > > > > -       /* Wa_22011186057 */
> > > > > > > > -       if (IS_ALDERLAKE_P(i915) &&
> > > > > > > > IS_DISPLAY_STEP(i915,
> > > > > > > > STEP_A0, STEP_B0))
> > > > > > > > -               return false;
> > > > > > > > -
> > > > > > > > -       return plane_id < PLANE_SPRITE4;
> > > > > > > > -}
> > > > > > > > -
> > > > > > > > -static u8 skl_get_plane_caps(struct drm_i915_private
> > > > > > > > *i915,
> > > > > > > > -                            enum pipe pipe, enum
> > > > > > > > plane_id
> > > > > > > > plane_id)
> > > > > > > > -{
> > > > > > > > -       u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > > > > > -
> > > > > > > > -       if (DISPLAY_VER(i915) < 13 ||
> > > > > > > > IS_ALDERLAKE_P(i915))
> > > > > > > > -               caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > > > > > -       if (DISPLAY_VER(i915) < 12)
> > > > > > > > -               caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > > > > > -       if (HAS_4TILE(i915))
> > > > > > > > -               caps |= INTEL_PLANE_CAP_TILING_4;
> > > > > > > > -
> > > > > > > > -       if (skl_plane_has_rc_ccs(i915, pipe, plane_id))
> > > > > > > > {
> > > > > > > > -               caps |= INTEL_PLANE_CAP_CCS_RC;
> > > > > > > > -               if (DISPLAY_VER(i915) >= 12)
> > > > > > > > -                       caps |=
> > > > > > > > INTEL_PLANE_CAP_CCS_RC_CC;
> > > > > > > > -       }
> > > > > > > > -
> > > > > > > > -       if (gen12_plane_has_mc_ccs(i915, plane_id))
> > > > > > > > -               caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > > > > > -
> > > > > > > > -       return caps;
> > > > > > > > -}
> > > > > > > > -
> > > > > > > >  struct intel_plane *
> > > > > > > >  skl_universal_plane_create(struct drm_i915_private
> > > > > > > > *dev_priv,
> > > > > > > >                            enum pipe pipe, enum
> > > > > > > > plane_id
> > > > > > > > plane_id)
> > > > > > > > diff --git
> > > > > > > > a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > > > > b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > > > > index ccf83c12b545..425c6e6744a6 100644
> > > > > > > > --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > > > > +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > > > > > > @@ -15,6 +15,7 @@
> > > > > > > >  #include "intel_fb.h"
> > > > > > > >  #include "intel_fb_pin.h"
> > > > > > > >  #include "intel_frontbuffer.h"
> > > > > > > > +#include "intel_plane_caps.h"
> > > > > > > >  #include "intel_plane_initial.h"
> > > > > > > >  
> > > > > > > >  static bool
> > > > > > > > @@ -289,3 +290,25 @@ void
> > > > > > > > intel_crtc_initial_plane_config(struct intel_crtc
> > > > > > > > *crtc)
> > > > > > > >  
> > > > > > > >         plane_config_fini(&plane_config);
> > > > > > > >  }
> > > > > > > > +
> > > > > > > > +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> > > > > > > > +                     enum pipe pipe, enum plane_id
> > > > > > > > plane_id)
> > > > > > > > +{
> > > > > > > > +       u8 caps = INTEL_PLANE_CAP_TILING_X;
> > > > > > > > +
> > > > > > > > +       if (DISPLAY_VER(i915) < 13 ||
> > > > > > > > IS_ALDERLAKE_P(i915))
> > > > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Y;
> > > > > > > > +       if (DISPLAY_VER(i915) < 12)
> > > > > > > > +               caps |= INTEL_PLANE_CAP_TILING_Yf;
> > > > > > > > +       if (HAS_4TILE(i915))
> > > > > > > > +               caps |= INTEL_PLANE_CAP_TILING_4;
> > > > > > > > +
> > > > > > > > +       if (HAS_FLAT_CCS(i915)) {
> > > > > > > > +               caps |= INTEL_PLANE_CAP_CCS_RC |
> > > > > > > > INTEL_PLANE_CAP_CCS_RC_CC;
> > > > > > > > +
> > > > > > > > +               if (plane_id < PLANE_SPRITE4)
> > > > > > > > +                       caps |= INTEL_PLANE_CAP_CCS_MC;
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       return caps;
> > > > > > > > +}
> > > > > > > 
> > > > > > > -- 
> > > > > > > Jani Nikula, Intel
> > > > > 
> > > 
>
Jani Nikula Jan. 16, 2024, 9:01 a.m. UTC | #10
On Mon, 15 Jan 2024, "Hellstrom, Thomas" <thomas.hellstrom@intel.com> wrote:
> I wonder if, moving forward, we should use a topic/display-for-xe
> branch (no force-push) that can be used for display commits that need
> to be in Xe during -next cycles. It could then be merged into xe and
> i915 as needed?

Topic branches have been discouraged lately, but it's really up to the
drm maintainers. Cc: Dave and Sima, thoughts?

It's also an additional point of confusion for committers to apply to a
topic branch. Deciding between drm-xe-next and drm-intel-next can
already go wrong...


BR,
Jani.
Jani Nikula Jan. 23, 2024, 10:49 a.m. UTC | #11
On Tue, 02 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:
> Aux ccs framebuffers don't work on Xe driver hence disable them
> from plane capabilities until they are fixed. Flat ccs framebuffers
> work and they are left enabled. Here is separated plane capabilities
> check on i915 so it can behave differencly depending on the driver.

I still think there's too much going on in this one patch. It refactors
i915 and modifies xe behaviour in one go.

It adds intel_plane_caps.[ch] in i915, but extracts them from skl+
specific functions and files. xe uses the .h but adds the code in
existing xe_plane_initial.c. There's also intel_plane_initial.c i915
side, but that's not where the functions get moved in i915 side.

I'm trying to look at the actual functional change, and I'm wondering if
it isn't just this:

index 511dc1544854..8bba6c2e5098 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -2290,6 +2290,9 @@ static u8 skl_get_plane_caps(struct drm_i915_private *i915,
 	if (HAS_4TILE(i915))
 		caps |= INTEL_PLANE_CAP_TILING_4;
 
+	if (!IS_ENABLED(I915) && !HAS_FLAT_CCS(i915))
+		return caps;
+
 	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
 		caps |= INTEL_PLANE_CAP_CCS_RC;
 		if (DISPLAY_VER(i915) >= 12)

I'm not saying that's exactly pretty, either, but IIUC this is supposed
to be a temporary measure ("until they are fixed"), I'd rather take this
small thing instead.

BR,
Jani.




>
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  drivers/gpu/drm/i915/Makefile                 |  1 +
>  .../gpu/drm/i915/display/intel_plane_caps.c   | 68 +++++++++++++++++++
>  .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
>  .../drm/i915/display/skl_universal_plane.c    | 61 +----------------
>  drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
>  5 files changed, 107 insertions(+), 60 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e777686190ca..c5e3c2dd0a01 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -302,6 +302,7 @@ i915-y += \
>  	display/intel_overlay.o \
>  	display/intel_pch_display.o \
>  	display/intel_pch_refclk.o \
> +	display/intel_plane_caps.o \
>  	display/intel_plane_initial.o \
>  	display/intel_pmdemand.o \
>  	display/intel_psr.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> new file mode 100644
> index 000000000000..6206ae11f296
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_fb.h"
> +#include "intel_plane_caps.h"
> +
> +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> +				 enum pipe pipe, enum plane_id plane_id)
> +{
> +	/* Wa_22011186057 */
> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> +		return false;
> +
> +	if (DISPLAY_VER(i915) >= 11)
> +		return true;
> +
> +	if (IS_GEMINILAKE(i915))
> +		return pipe != PIPE_C;
> +
> +	return pipe != PIPE_C &&
> +		(plane_id == PLANE_PRIMARY ||
> +		 plane_id == PLANE_SPRITE0);
> +}
> +
> +static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> +				   enum plane_id plane_id)
> +{
> +	if (DISPLAY_VER(i915) < 12)
> +		return false;
> +
> +	/* Wa_14010477008 */
> +	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> +	    (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> +		return false;
> +
> +	/* Wa_22011186057 */
> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> +		return false;
> +
> +	return plane_id < PLANE_SPRITE4;
> +}
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id)
> +{
> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> +
> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_Y;
> +	if (DISPLAY_VER(i915) < 12)
> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> +	if (HAS_4TILE(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_4;
> +
> +	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> +		caps |= INTEL_PLANE_CAP_CCS_RC;
> +		if (DISPLAY_VER(i915) >= 12)
> +			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> +	}
> +
> +	if (gen12_plane_has_mc_ccs(i915, plane_id))
> +		caps |= INTEL_PLANE_CAP_CCS_MC;
> +
> +	return caps;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> new file mode 100644
> index 000000000000..60a941c76f23
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef __INTEL_PLANE_CAPS_H__
> +#define __INTEL_PLANE_CAPS_H__
> +
> +#include "intel_display_types.h"
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id);
> +
> +#endif /* __INTEL_PLANE_CAPS_H__ */
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 511dc1544854..f2fd3833c61d 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -17,6 +17,7 @@
>  #include "intel_fb.h"
>  #include "intel_fbc.h"
>  #include "intel_frontbuffer.h"
> +#include "intel_plane_caps.h"
>  #include "intel_psr.h"
>  #include "intel_psr_regs.h"
>  #include "skl_scaler.h"
> @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct intel_plane *plane)
>  	spin_unlock_irq(&i915->irq_lock);
>  }
>  
> -static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> -				 enum pipe pipe, enum plane_id plane_id)
> -{
> -	/* Wa_22011186057 */
> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> -		return false;
> -
> -	if (DISPLAY_VER(i915) >= 11)
> -		return true;
> -
> -	if (IS_GEMINILAKE(i915))
> -		return pipe != PIPE_C;
> -
> -	return pipe != PIPE_C &&
> -		(plane_id == PLANE_PRIMARY ||
> -		 plane_id == PLANE_SPRITE0);
> -}
> -
> -static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> -				   enum plane_id plane_id)
> -{
> -	if (DISPLAY_VER(i915) < 12)
> -		return false;
> -
> -	/* Wa_14010477008 */
> -	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> -		(IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> -		return false;
> -
> -	/* Wa_22011186057 */
> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> -		return false;
> -
> -	return plane_id < PLANE_SPRITE4;
> -}
> -
> -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
> -			     enum pipe pipe, enum plane_id plane_id)
> -{
> -	u8 caps = INTEL_PLANE_CAP_TILING_X;
> -
> -	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> -		caps |= INTEL_PLANE_CAP_TILING_Y;
> -	if (DISPLAY_VER(i915) < 12)
> -		caps |= INTEL_PLANE_CAP_TILING_Yf;
> -	if (HAS_4TILE(i915))
> -		caps |= INTEL_PLANE_CAP_TILING_4;
> -
> -	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> -		caps |= INTEL_PLANE_CAP_CCS_RC;
> -		if (DISPLAY_VER(i915) >= 12)
> -			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> -	}
> -
> -	if (gen12_plane_has_mc_ccs(i915, plane_id))
> -		caps |= INTEL_PLANE_CAP_CCS_MC;
> -
> -	return caps;
> -}
> -
>  struct intel_plane *
>  skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  			   enum pipe pipe, enum plane_id plane_id)
> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> index ccf83c12b545..425c6e6744a6 100644
> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> @@ -15,6 +15,7 @@
>  #include "intel_fb.h"
>  #include "intel_fb_pin.h"
>  #include "intel_frontbuffer.h"
> +#include "intel_plane_caps.h"
>  #include "intel_plane_initial.h"
>  
>  static bool
> @@ -289,3 +290,25 @@ void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
>  
>  	plane_config_fini(&plane_config);
>  }
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id)
> +{
> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> +
> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_Y;
> +	if (DISPLAY_VER(i915) < 12)
> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> +	if (HAS_4TILE(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_4;
> +
> +	if (HAS_FLAT_CCS(i915)) {
> +		caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
> +
> +		if (plane_id < PLANE_SPRITE4)
> +			caps |= INTEL_PLANE_CAP_CCS_MC;
> +	}
> +
> +	return caps;
> +}
Ville Syrjala Jan. 23, 2024, 12:50 p.m. UTC | #12
On Tue, Jan 02, 2024 at 08:24:22PM +0200, Juha-Pekka Heikkila wrote:
> Aux ccs framebuffers don't work on Xe driver hence disable them
> from plane capabilities until they are fixed. Flat ccs framebuffers
> work and they are left enabled. Here is separated plane capabilities
> check on i915 so it can behave differencly depending on the driver.
> 
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  drivers/gpu/drm/i915/Makefile                 |  1 +
>  .../gpu/drm/i915/display/intel_plane_caps.c   | 68 +++++++++++++++++++
>  .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
>  .../drm/i915/display/skl_universal_plane.c    | 61 +----------------
>  drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
>  5 files changed, 107 insertions(+), 60 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e777686190ca..c5e3c2dd0a01 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -302,6 +302,7 @@ i915-y += \
>  	display/intel_overlay.o \
>  	display/intel_pch_display.o \
>  	display/intel_pch_refclk.o \
> +	display/intel_plane_caps.o \
>  	display/intel_plane_initial.o \
>  	display/intel_pmdemand.o \
>  	display/intel_psr.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> new file mode 100644
> index 000000000000..6206ae11f296
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_fb.h"
> +#include "intel_plane_caps.h"
> +
> +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> +				 enum pipe pipe, enum plane_id plane_id)
> +{
> +	/* Wa_22011186057 */
> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> +		return false;
> +
> +	if (DISPLAY_VER(i915) >= 11)
> +		return true;
> +
> +	if (IS_GEMINILAKE(i915))
> +		return pipe != PIPE_C;
> +
> +	return pipe != PIPE_C &&
> +		(plane_id == PLANE_PRIMARY ||
> +		 plane_id == PLANE_SPRITE0);
> +}

These are about the *hardware* capabilities of the skl+
univeral planes. Thus IMO they belong in
skl_universal_plane.c and nowhere else.

> +
> +static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> +				   enum plane_id plane_id)
> +{
> +	if (DISPLAY_VER(i915) < 12)
> +		return false;
> +
> +	/* Wa_14010477008 */
> +	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> +	    (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> +		return false;
> +
> +	/* Wa_22011186057 */
> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> +		return false;
> +
> +	return plane_id < PLANE_SPRITE4;
> +}
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id)
> +{
> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> +
> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_Y;
> +	if (DISPLAY_VER(i915) < 12)
> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> +	if (HAS_4TILE(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_4;
> +
> +	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> +		caps |= INTEL_PLANE_CAP_CCS_RC;
> +		if (DISPLAY_VER(i915) >= 12)
> +			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> +	}
> +
> +	if (gen12_plane_has_mc_ccs(i915, plane_id))
> +		caps |= INTEL_PLANE_CAP_CCS_MC;
> +
> +	return caps;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> new file mode 100644
> index 000000000000..60a941c76f23
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef __INTEL_PLANE_CAPS_H__
> +#define __INTEL_PLANE_CAPS_H__
> +
> +#include "intel_display_types.h"
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id);
> +
> +#endif /* __INTEL_PLANE_CAPS_H__ */
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 511dc1544854..f2fd3833c61d 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -17,6 +17,7 @@
>  #include "intel_fb.h"
>  #include "intel_fbc.h"
>  #include "intel_frontbuffer.h"
> +#include "intel_plane_caps.h"
>  #include "intel_psr.h"
>  #include "intel_psr_regs.h"
>  #include "skl_scaler.h"
> @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct intel_plane *plane)
>  	spin_unlock_irq(&i915->irq_lock);
>  }
>  
> -static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
> -				 enum pipe pipe, enum plane_id plane_id)
> -{
> -	/* Wa_22011186057 */
> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> -		return false;
> -
> -	if (DISPLAY_VER(i915) >= 11)
> -		return true;
> -
> -	if (IS_GEMINILAKE(i915))
> -		return pipe != PIPE_C;
> -
> -	return pipe != PIPE_C &&
> -		(plane_id == PLANE_PRIMARY ||
> -		 plane_id == PLANE_SPRITE0);
> -}
> -
> -static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
> -				   enum plane_id plane_id)
> -{
> -	if (DISPLAY_VER(i915) < 12)
> -		return false;
> -
> -	/* Wa_14010477008 */
> -	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
> -		(IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
> -		return false;
> -
> -	/* Wa_22011186057 */
> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> -		return false;
> -
> -	return plane_id < PLANE_SPRITE4;
> -}
> -
> -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
> -			     enum pipe pipe, enum plane_id plane_id)
> -{
> -	u8 caps = INTEL_PLANE_CAP_TILING_X;
> -
> -	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> -		caps |= INTEL_PLANE_CAP_TILING_Y;
> -	if (DISPLAY_VER(i915) < 12)
> -		caps |= INTEL_PLANE_CAP_TILING_Yf;
> -	if (HAS_4TILE(i915))
> -		caps |= INTEL_PLANE_CAP_TILING_4;
> -
> -	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
> -		caps |= INTEL_PLANE_CAP_CCS_RC;
> -		if (DISPLAY_VER(i915) >= 12)
> -			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
> -	}
> -
> -	if (gen12_plane_has_mc_ccs(i915, plane_id))
> -		caps |= INTEL_PLANE_CAP_CCS_MC;
> -
> -	return caps;
> -}
> -
>  struct intel_plane *
>  skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  			   enum pipe pipe, enum plane_id plane_id)
> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> index ccf83c12b545..425c6e6744a6 100644
> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> @@ -15,6 +15,7 @@
>  #include "intel_fb.h"
>  #include "intel_fb_pin.h"
>  #include "intel_frontbuffer.h"
> +#include "intel_plane_caps.h"
>  #include "intel_plane_initial.h"
>  
>  static bool
> @@ -289,3 +290,25 @@ void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
>  
>  	plane_config_fini(&plane_config);
>  }
> +
> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
> +		      enum pipe pipe, enum plane_id plane_id)
> +{
> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
> +
> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_Y;
> +	if (DISPLAY_VER(i915) < 12)
> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
> +	if (HAS_4TILE(i915))
> +		caps |= INTEL_PLANE_CAP_TILING_4;
> +
> +	if (HAS_FLAT_CCS(i915)) {
> +		caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
> +
> +		if (plane_id < PLANE_SPRITE4)
> +			caps |= INTEL_PLANE_CAP_CCS_MC;
> +	}
> +
> +	return caps;
> +}
> -- 
> 2.25.1
Juha-Pekka Heikkila Jan. 23, 2024, 5:49 p.m. UTC | #13
On 23.1.2024 14.50, Ville Syrjälä wrote:
> On Tue, Jan 02, 2024 at 08:24:22PM +0200, Juha-Pekka Heikkila wrote:
>> Aux ccs framebuffers don't work on Xe driver hence disable them
>> from plane capabilities until they are fixed. Flat ccs framebuffers
>> work and they are left enabled. Here is separated plane capabilities
>> check on i915 so it can behave differencly depending on the driver.
>>
>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/Makefile                 |  1 +
>>   .../gpu/drm/i915/display/intel_plane_caps.c   | 68 +++++++++++++++++++
>>   .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
>>   .../drm/i915/display/skl_universal_plane.c    | 61 +----------------
>>   drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
>>   5 files changed, 107 insertions(+), 60 deletions(-)
>>   create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
>>   create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index e777686190ca..c5e3c2dd0a01 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -302,6 +302,7 @@ i915-y += \
>>   	display/intel_overlay.o \
>>   	display/intel_pch_display.o \
>>   	display/intel_pch_refclk.o \
>> +	display/intel_plane_caps.o \
>>   	display/intel_plane_initial.o \
>>   	display/intel_pmdemand.o \
>>   	display/intel_psr.o \
>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
>> new file mode 100644
>> index 000000000000..6206ae11f296
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
>> @@ -0,0 +1,68 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#include "i915_drv.h"
>> +#include "intel_fb.h"
>> +#include "intel_plane_caps.h"
>> +
>> +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
>> +				 enum pipe pipe, enum plane_id plane_id)
>> +{
>> +	/* Wa_22011186057 */
>> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>> +		return false;
>> +
>> +	if (DISPLAY_VER(i915) >= 11)
>> +		return true;
>> +
>> +	if (IS_GEMINILAKE(i915))
>> +		return pipe != PIPE_C;
>> +
>> +	return pipe != PIPE_C &&
>> +		(plane_id == PLANE_PRIMARY ||
>> +		 plane_id == PLANE_SPRITE0);
>> +}
> 
> These are about the *hardware* capabilities of the skl+
> univeral planes. Thus IMO they belong in
> skl_universal_plane.c and nowhere else.

Problem with this is we'd need with same display code allow and deny 
usage of ccs framebuffers. Since doing it via ifdefs was not allowed 
there was left limited options. All those other functionality separation 
is done in similar way, say for example dpt code.

> 
>> +
>> +static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
>> +				   enum plane_id plane_id)
>> +{
>> +	if (DISPLAY_VER(i915) < 12)
>> +		return false;
>> +
>> +	/* Wa_14010477008 */
>> +	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
>> +	    (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
>> +		return false;
>> +
>> +	/* Wa_22011186057 */
>> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>> +		return false;
>> +
>> +	return plane_id < PLANE_SPRITE4;
>> +}
>> +
>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> +		      enum pipe pipe, enum plane_id plane_id)
>> +{
>> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
>> +
>> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>> +		caps |= INTEL_PLANE_CAP_TILING_Y;
>> +	if (DISPLAY_VER(i915) < 12)
>> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
>> +	if (HAS_4TILE(i915))
>> +		caps |= INTEL_PLANE_CAP_TILING_4;
>> +
>> +	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>> +		caps |= INTEL_PLANE_CAP_CCS_RC;
>> +		if (DISPLAY_VER(i915) >= 12)
>> +			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
>> +	}
>> +
>> +	if (gen12_plane_has_mc_ccs(i915, plane_id))
>> +		caps |= INTEL_PLANE_CAP_CCS_MC;
>> +
>> +	return caps;
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
>> new file mode 100644
>> index 000000000000..60a941c76f23
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#ifndef __INTEL_PLANE_CAPS_H__
>> +#define __INTEL_PLANE_CAPS_H__
>> +
>> +#include "intel_display_types.h"
>> +
>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> +		      enum pipe pipe, enum plane_id plane_id);
>> +
>> +#endif /* __INTEL_PLANE_CAPS_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> index 511dc1544854..f2fd3833c61d 100644
>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> @@ -17,6 +17,7 @@
>>   #include "intel_fb.h"
>>   #include "intel_fbc.h"
>>   #include "intel_frontbuffer.h"
>> +#include "intel_plane_caps.h"
>>   #include "intel_psr.h"
>>   #include "intel_psr_regs.h"
>>   #include "skl_scaler.h"
>> @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct intel_plane *plane)
>>   	spin_unlock_irq(&i915->irq_lock);
>>   }
>>   
>> -static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
>> -				 enum pipe pipe, enum plane_id plane_id)
>> -{
>> -	/* Wa_22011186057 */
>> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>> -		return false;
>> -
>> -	if (DISPLAY_VER(i915) >= 11)
>> -		return true;
>> -
>> -	if (IS_GEMINILAKE(i915))
>> -		return pipe != PIPE_C;
>> -
>> -	return pipe != PIPE_C &&
>> -		(plane_id == PLANE_PRIMARY ||
>> -		 plane_id == PLANE_SPRITE0);
>> -}
>> -
>> -static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
>> -				   enum plane_id plane_id)
>> -{
>> -	if (DISPLAY_VER(i915) < 12)
>> -		return false;
>> -
>> -	/* Wa_14010477008 */
>> -	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
>> -		(IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
>> -		return false;
>> -
>> -	/* Wa_22011186057 */
>> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>> -		return false;
>> -
>> -	return plane_id < PLANE_SPRITE4;
>> -}
>> -
>> -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> -			     enum pipe pipe, enum plane_id plane_id)
>> -{
>> -	u8 caps = INTEL_PLANE_CAP_TILING_X;
>> -
>> -	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>> -		caps |= INTEL_PLANE_CAP_TILING_Y;
>> -	if (DISPLAY_VER(i915) < 12)
>> -		caps |= INTEL_PLANE_CAP_TILING_Yf;
>> -	if (HAS_4TILE(i915))
>> -		caps |= INTEL_PLANE_CAP_TILING_4;
>> -
>> -	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>> -		caps |= INTEL_PLANE_CAP_CCS_RC;
>> -		if (DISPLAY_VER(i915) >= 12)
>> -			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
>> -	}
>> -
>> -	if (gen12_plane_has_mc_ccs(i915, plane_id))
>> -		caps |= INTEL_PLANE_CAP_CCS_MC;
>> -
>> -	return caps;
>> -}
>> -
>>   struct intel_plane *
>>   skl_universal_plane_create(struct drm_i915_private *dev_priv,
>>   			   enum pipe pipe, enum plane_id plane_id)
>> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> index ccf83c12b545..425c6e6744a6 100644
>> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> @@ -15,6 +15,7 @@
>>   #include "intel_fb.h"
>>   #include "intel_fb_pin.h"
>>   #include "intel_frontbuffer.h"
>> +#include "intel_plane_caps.h"
>>   #include "intel_plane_initial.h"
>>   
>>   static bool
>> @@ -289,3 +290,25 @@ void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
>>   
>>   	plane_config_fini(&plane_config);
>>   }
>> +
>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> +		      enum pipe pipe, enum plane_id plane_id)
>> +{
>> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
>> +
>> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>> +		caps |= INTEL_PLANE_CAP_TILING_Y;
>> +	if (DISPLAY_VER(i915) < 12)
>> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
>> +	if (HAS_4TILE(i915))
>> +		caps |= INTEL_PLANE_CAP_TILING_4;
>> +
>> +	if (HAS_FLAT_CCS(i915)) {
>> +		caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
>> +
>> +		if (plane_id < PLANE_SPRITE4)
>> +			caps |= INTEL_PLANE_CAP_CCS_MC;
>> +	}
>> +
>> +	return caps;
>> +}
>> -- 
>> 2.25.1
>
Juha-Pekka Heikkila Jan. 23, 2024, 5:53 p.m. UTC | #14
On 23.1.2024 12.49, Jani Nikula wrote:
> On Tue, 02 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:
>> Aux ccs framebuffers don't work on Xe driver hence disable them
>> from plane capabilities until they are fixed. Flat ccs framebuffers
>> work and they are left enabled. Here is separated plane capabilities
>> check on i915 so it can behave differencly depending on the driver.
> 
> I still think there's too much going on in this one patch. It refactors
> i915 and modifies xe behaviour in one go.
> 
> It adds intel_plane_caps.[ch] in i915, but extracts them from skl+
> specific functions and files. xe uses the .h but adds the code in
> existing xe_plane_initial.c. There's also intel_plane_initial.c i915
> side, but that's not where the functions get moved in i915 side.

I was never against splitting it, I can do that.

> 
> I'm trying to look at the actual functional change, and I'm wondering if
> it isn't just this:
> 
> index 511dc1544854..8bba6c2e5098 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -2290,6 +2290,9 @@ static u8 skl_get_plane_caps(struct drm_i915_private *i915,
>   	if (HAS_4TILE(i915))
>   		caps |= INTEL_PLANE_CAP_TILING_4;
>   
> +	if (!IS_ENABLED(I915) && !HAS_FLAT_CCS(i915))
> +		return caps;
> +
>   	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>   		caps |= INTEL_PLANE_CAP_CCS_RC;
>   		if (DISPLAY_VER(i915) >= 12)
> 
> I'm not saying that's exactly pretty, either, but IIUC this is supposed
> to be a temporary measure ("until they are fixed"), I'd rather take this
> small thing instead.
> 

Would that work when both i915 and Xe are being built?

> 
> 
> 
>>
>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/Makefile                 |  1 +
>>   .../gpu/drm/i915/display/intel_plane_caps.c   | 68 +++++++++++++++++++
>>   .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
>>   .../drm/i915/display/skl_universal_plane.c    | 61 +----------------
>>   drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
>>   5 files changed, 107 insertions(+), 60 deletions(-)
>>   create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
>>   create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index e777686190ca..c5e3c2dd0a01 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -302,6 +302,7 @@ i915-y += \
>>   	display/intel_overlay.o \
>>   	display/intel_pch_display.o \
>>   	display/intel_pch_refclk.o \
>> +	display/intel_plane_caps.o \
>>   	display/intel_plane_initial.o \
>>   	display/intel_pmdemand.o \
>>   	display/intel_psr.o \
>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
>> new file mode 100644
>> index 000000000000..6206ae11f296
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
>> @@ -0,0 +1,68 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#include "i915_drv.h"
>> +#include "intel_fb.h"
>> +#include "intel_plane_caps.h"
>> +
>> +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
>> +				 enum pipe pipe, enum plane_id plane_id)
>> +{
>> +	/* Wa_22011186057 */
>> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>> +		return false;
>> +
>> +	if (DISPLAY_VER(i915) >= 11)
>> +		return true;
>> +
>> +	if (IS_GEMINILAKE(i915))
>> +		return pipe != PIPE_C;
>> +
>> +	return pipe != PIPE_C &&
>> +		(plane_id == PLANE_PRIMARY ||
>> +		 plane_id == PLANE_SPRITE0);
>> +}
>> +
>> +static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
>> +				   enum plane_id plane_id)
>> +{
>> +	if (DISPLAY_VER(i915) < 12)
>> +		return false;
>> +
>> +	/* Wa_14010477008 */
>> +	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
>> +	    (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
>> +		return false;
>> +
>> +	/* Wa_22011186057 */
>> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>> +		return false;
>> +
>> +	return plane_id < PLANE_SPRITE4;
>> +}
>> +
>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> +		      enum pipe pipe, enum plane_id plane_id)
>> +{
>> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
>> +
>> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>> +		caps |= INTEL_PLANE_CAP_TILING_Y;
>> +	if (DISPLAY_VER(i915) < 12)
>> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
>> +	if (HAS_4TILE(i915))
>> +		caps |= INTEL_PLANE_CAP_TILING_4;
>> +
>> +	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>> +		caps |= INTEL_PLANE_CAP_CCS_RC;
>> +		if (DISPLAY_VER(i915) >= 12)
>> +			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
>> +	}
>> +
>> +	if (gen12_plane_has_mc_ccs(i915, plane_id))
>> +		caps |= INTEL_PLANE_CAP_CCS_MC;
>> +
>> +	return caps;
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
>> new file mode 100644
>> index 000000000000..60a941c76f23
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#ifndef __INTEL_PLANE_CAPS_H__
>> +#define __INTEL_PLANE_CAPS_H__
>> +
>> +#include "intel_display_types.h"
>> +
>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> +		      enum pipe pipe, enum plane_id plane_id);
>> +
>> +#endif /* __INTEL_PLANE_CAPS_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> index 511dc1544854..f2fd3833c61d 100644
>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> @@ -17,6 +17,7 @@
>>   #include "intel_fb.h"
>>   #include "intel_fbc.h"
>>   #include "intel_frontbuffer.h"
>> +#include "intel_plane_caps.h"
>>   #include "intel_psr.h"
>>   #include "intel_psr_regs.h"
>>   #include "skl_scaler.h"
>> @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct intel_plane *plane)
>>   	spin_unlock_irq(&i915->irq_lock);
>>   }
>>   
>> -static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
>> -				 enum pipe pipe, enum plane_id plane_id)
>> -{
>> -	/* Wa_22011186057 */
>> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>> -		return false;
>> -
>> -	if (DISPLAY_VER(i915) >= 11)
>> -		return true;
>> -
>> -	if (IS_GEMINILAKE(i915))
>> -		return pipe != PIPE_C;
>> -
>> -	return pipe != PIPE_C &&
>> -		(plane_id == PLANE_PRIMARY ||
>> -		 plane_id == PLANE_SPRITE0);
>> -}
>> -
>> -static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
>> -				   enum plane_id plane_id)
>> -{
>> -	if (DISPLAY_VER(i915) < 12)
>> -		return false;
>> -
>> -	/* Wa_14010477008 */
>> -	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
>> -		(IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
>> -		return false;
>> -
>> -	/* Wa_22011186057 */
>> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>> -		return false;
>> -
>> -	return plane_id < PLANE_SPRITE4;
>> -}
>> -
>> -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> -			     enum pipe pipe, enum plane_id plane_id)
>> -{
>> -	u8 caps = INTEL_PLANE_CAP_TILING_X;
>> -
>> -	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>> -		caps |= INTEL_PLANE_CAP_TILING_Y;
>> -	if (DISPLAY_VER(i915) < 12)
>> -		caps |= INTEL_PLANE_CAP_TILING_Yf;
>> -	if (HAS_4TILE(i915))
>> -		caps |= INTEL_PLANE_CAP_TILING_4;
>> -
>> -	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>> -		caps |= INTEL_PLANE_CAP_CCS_RC;
>> -		if (DISPLAY_VER(i915) >= 12)
>> -			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
>> -	}
>> -
>> -	if (gen12_plane_has_mc_ccs(i915, plane_id))
>> -		caps |= INTEL_PLANE_CAP_CCS_MC;
>> -
>> -	return caps;
>> -}
>> -
>>   struct intel_plane *
>>   skl_universal_plane_create(struct drm_i915_private *dev_priv,
>>   			   enum pipe pipe, enum plane_id plane_id)
>> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> index ccf83c12b545..425c6e6744a6 100644
>> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> @@ -15,6 +15,7 @@
>>   #include "intel_fb.h"
>>   #include "intel_fb_pin.h"
>>   #include "intel_frontbuffer.h"
>> +#include "intel_plane_caps.h"
>>   #include "intel_plane_initial.h"
>>   
>>   static bool
>> @@ -289,3 +290,25 @@ void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
>>   
>>   	plane_config_fini(&plane_config);
>>   }
>> +
>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> +		      enum pipe pipe, enum plane_id plane_id)
>> +{
>> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
>> +
>> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>> +		caps |= INTEL_PLANE_CAP_TILING_Y;
>> +	if (DISPLAY_VER(i915) < 12)
>> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
>> +	if (HAS_4TILE(i915))
>> +		caps |= INTEL_PLANE_CAP_TILING_4;
>> +
>> +	if (HAS_FLAT_CCS(i915)) {
>> +		caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
>> +
>> +		if (plane_id < PLANE_SPRITE4)
>> +			caps |= INTEL_PLANE_CAP_CCS_MC;
>> +	}
>> +
>> +	return caps;
>> +}
>
Jani Nikula Jan. 23, 2024, 5:57 p.m. UTC | #15
On Tue, 23 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:
> On 23.1.2024 12.49, Jani Nikula wrote:
>> On Tue, 02 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:
>>> Aux ccs framebuffers don't work on Xe driver hence disable them
>>> from plane capabilities until they are fixed. Flat ccs framebuffers
>>> work and they are left enabled. Here is separated plane capabilities
>>> check on i915 so it can behave differencly depending on the driver.
>> 
>> I still think there's too much going on in this one patch. It refactors
>> i915 and modifies xe behaviour in one go.
>> 
>> It adds intel_plane_caps.[ch] in i915, but extracts them from skl+
>> specific functions and files. xe uses the .h but adds the code in
>> existing xe_plane_initial.c. There's also intel_plane_initial.c i915
>> side, but that's not where the functions get moved in i915 side.
>
> I was never against splitting it, I can do that.
>
>> 
>> I'm trying to look at the actual functional change, and I'm wondering if
>> it isn't just this:
>> 
>> index 511dc1544854..8bba6c2e5098 100644
>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> @@ -2290,6 +2290,9 @@ static u8 skl_get_plane_caps(struct drm_i915_private *i915,
>>   	if (HAS_4TILE(i915))
>>   		caps |= INTEL_PLANE_CAP_TILING_4;
>>   
>> +	if (!IS_ENABLED(I915) && !HAS_FLAT_CCS(i915))
>> +		return caps;
>> +
>>   	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>>   		caps |= INTEL_PLANE_CAP_CCS_RC;
>>   		if (DISPLAY_VER(i915) >= 12)
>> 
>> I'm not saying that's exactly pretty, either, but IIUC this is supposed
>> to be a temporary measure ("until they are fixed"), I'd rather take this
>> small thing instead.
>> 
>
> Would that work when both i915 and Xe are being built?

IS_ENABLED(I915) will be false for xe build, true for i915 build. And
HAS_FLAT_CCS() is defined for both, in different ways.

It's essentially the same as #ifdef but much less of an eyesore.

BR,
Jani.




>
>> 
>> 
>> 
>>>
>>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>> ---
>>>   drivers/gpu/drm/i915/Makefile                 |  1 +
>>>   .../gpu/drm/i915/display/intel_plane_caps.c   | 68 +++++++++++++++++++
>>>   .../gpu/drm/i915/display/intel_plane_caps.h   | 14 ++++
>>>   .../drm/i915/display/skl_universal_plane.c    | 61 +----------------
>>>   drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
>>>   5 files changed, 107 insertions(+), 60 deletions(-)
>>>   create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
>>>   create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h
>>>
>>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>>> index e777686190ca..c5e3c2dd0a01 100644
>>> --- a/drivers/gpu/drm/i915/Makefile
>>> +++ b/drivers/gpu/drm/i915/Makefile
>>> @@ -302,6 +302,7 @@ i915-y += \
>>>   	display/intel_overlay.o \
>>>   	display/intel_pch_display.o \
>>>   	display/intel_pch_refclk.o \
>>> +	display/intel_plane_caps.o \
>>>   	display/intel_plane_initial.o \
>>>   	display/intel_pmdemand.o \
>>>   	display/intel_psr.o \
>>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
>>> new file mode 100644
>>> index 000000000000..6206ae11f296
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
>>> @@ -0,0 +1,68 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +
>>> +#include "i915_drv.h"
>>> +#include "intel_fb.h"
>>> +#include "intel_plane_caps.h"
>>> +
>>> +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
>>> +				 enum pipe pipe, enum plane_id plane_id)
>>> +{
>>> +	/* Wa_22011186057 */
>>> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>>> +		return false;
>>> +
>>> +	if (DISPLAY_VER(i915) >= 11)
>>> +		return true;
>>> +
>>> +	if (IS_GEMINILAKE(i915))
>>> +		return pipe != PIPE_C;
>>> +
>>> +	return pipe != PIPE_C &&
>>> +		(plane_id == PLANE_PRIMARY ||
>>> +		 plane_id == PLANE_SPRITE0);
>>> +}
>>> +
>>> +static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
>>> +				   enum plane_id plane_id)
>>> +{
>>> +	if (DISPLAY_VER(i915) < 12)
>>> +		return false;
>>> +
>>> +	/* Wa_14010477008 */
>>> +	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
>>> +	    (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
>>> +		return false;
>>> +
>>> +	/* Wa_22011186057 */
>>> +	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>>> +		return false;
>>> +
>>> +	return plane_id < PLANE_SPRITE4;
>>> +}
>>> +
>>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>>> +		      enum pipe pipe, enum plane_id plane_id)
>>> +{
>>> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
>>> +
>>> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>>> +		caps |= INTEL_PLANE_CAP_TILING_Y;
>>> +	if (DISPLAY_VER(i915) < 12)
>>> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
>>> +	if (HAS_4TILE(i915))
>>> +		caps |= INTEL_PLANE_CAP_TILING_4;
>>> +
>>> +	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>>> +		caps |= INTEL_PLANE_CAP_CCS_RC;
>>> +		if (DISPLAY_VER(i915) >= 12)
>>> +			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
>>> +	}
>>> +
>>> +	if (gen12_plane_has_mc_ccs(i915, plane_id))
>>> +		caps |= INTEL_PLANE_CAP_CCS_MC;
>>> +
>>> +	return caps;
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
>>> new file mode 100644
>>> index 000000000000..60a941c76f23
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
>>> @@ -0,0 +1,14 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +
>>> +#ifndef __INTEL_PLANE_CAPS_H__
>>> +#define __INTEL_PLANE_CAPS_H__
>>> +
>>> +#include "intel_display_types.h"
>>> +
>>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>>> +		      enum pipe pipe, enum plane_id plane_id);
>>> +
>>> +#endif /* __INTEL_PLANE_CAPS_H__ */
>>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>>> index 511dc1544854..f2fd3833c61d 100644
>>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>>> @@ -17,6 +17,7 @@
>>>   #include "intel_fb.h"
>>>   #include "intel_fbc.h"
>>>   #include "intel_frontbuffer.h"
>>> +#include "intel_plane_caps.h"
>>>   #include "intel_psr.h"
>>>   #include "intel_psr_regs.h"
>>>   #include "skl_scaler.h"
>>> @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct intel_plane *plane)
>>>   	spin_unlock_irq(&i915->irq_lock);
>>>   }
>>>   
>>> -static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
>>> -				 enum pipe pipe, enum plane_id plane_id)
>>> -{
>>> -	/* Wa_22011186057 */
>>> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>>> -		return false;
>>> -
>>> -	if (DISPLAY_VER(i915) >= 11)
>>> -		return true;
>>> -
>>> -	if (IS_GEMINILAKE(i915))
>>> -		return pipe != PIPE_C;
>>> -
>>> -	return pipe != PIPE_C &&
>>> -		(plane_id == PLANE_PRIMARY ||
>>> -		 plane_id == PLANE_SPRITE0);
>>> -}
>>> -
>>> -static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
>>> -				   enum plane_id plane_id)
>>> -{
>>> -	if (DISPLAY_VER(i915) < 12)
>>> -		return false;
>>> -
>>> -	/* Wa_14010477008 */
>>> -	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
>>> -		(IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
>>> -		return false;
>>> -
>>> -	/* Wa_22011186057 */
>>> -	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>>> -		return false;
>>> -
>>> -	return plane_id < PLANE_SPRITE4;
>>> -}
>>> -
>>> -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
>>> -			     enum pipe pipe, enum plane_id plane_id)
>>> -{
>>> -	u8 caps = INTEL_PLANE_CAP_TILING_X;
>>> -
>>> -	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>>> -		caps |= INTEL_PLANE_CAP_TILING_Y;
>>> -	if (DISPLAY_VER(i915) < 12)
>>> -		caps |= INTEL_PLANE_CAP_TILING_Yf;
>>> -	if (HAS_4TILE(i915))
>>> -		caps |= INTEL_PLANE_CAP_TILING_4;
>>> -
>>> -	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>>> -		caps |= INTEL_PLANE_CAP_CCS_RC;
>>> -		if (DISPLAY_VER(i915) >= 12)
>>> -			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
>>> -	}
>>> -
>>> -	if (gen12_plane_has_mc_ccs(i915, plane_id))
>>> -		caps |= INTEL_PLANE_CAP_CCS_MC;
>>> -
>>> -	return caps;
>>> -}
>>> -
>>>   struct intel_plane *
>>>   skl_universal_plane_create(struct drm_i915_private *dev_priv,
>>>   			   enum pipe pipe, enum plane_id plane_id)
>>> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>>> index ccf83c12b545..425c6e6744a6 100644
>>> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
>>> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>>> @@ -15,6 +15,7 @@
>>>   #include "intel_fb.h"
>>>   #include "intel_fb_pin.h"
>>>   #include "intel_frontbuffer.h"
>>> +#include "intel_plane_caps.h"
>>>   #include "intel_plane_initial.h"
>>>   
>>>   static bool
>>> @@ -289,3 +290,25 @@ void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
>>>   
>>>   	plane_config_fini(&plane_config);
>>>   }
>>> +
>>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>>> +		      enum pipe pipe, enum plane_id plane_id)
>>> +{
>>> +	u8 caps = INTEL_PLANE_CAP_TILING_X;
>>> +
>>> +	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>>> +		caps |= INTEL_PLANE_CAP_TILING_Y;
>>> +	if (DISPLAY_VER(i915) < 12)
>>> +		caps |= INTEL_PLANE_CAP_TILING_Yf;
>>> +	if (HAS_4TILE(i915))
>>> +		caps |= INTEL_PLANE_CAP_TILING_4;
>>> +
>>> +	if (HAS_FLAT_CCS(i915)) {
>>> +		caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
>>> +
>>> +		if (plane_id < PLANE_SPRITE4)
>>> +			caps |= INTEL_PLANE_CAP_CCS_MC;
>>> +	}
>>> +
>>> +	return caps;
>>> +}
>> 
>
Juha-Pekka Heikkila Jan. 23, 2024, 6:33 p.m. UTC | #16
On 23.1.2024 19.57, Jani Nikula wrote:
> On Tue, 23 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:
>> On 23.1.2024 12.49, Jani Nikula wrote:
>>> On Tue, 02 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:
>>>> Aux ccs framebuffers don't work on Xe driver hence disable them
>>>> from plane capabilities until they are fixed. Flat ccs framebuffers
>>>> work and they are left enabled. Here is separated plane capabilities
>>>> check on i915 so it can behave differencly depending on the driver.
>>>
>>> I still think there's too much going on in this one patch. It refactors
>>> i915 and modifies xe behaviour in one go.
>>>
>>> It adds intel_plane_caps.[ch] in i915, but extracts them from skl+
>>> specific functions and files. xe uses the .h but adds the code in
>>> existing xe_plane_initial.c. There's also intel_plane_initial.c i915
>>> side, but that's not where the functions get moved in i915 side.
>>
>> I was never against splitting it, I can do that.
>>
>>>
>>> I'm trying to look at the actual functional change, and I'm wondering if
>>> it isn't just this:
>>>
>>> index 511dc1544854..8bba6c2e5098 100644
>>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>>> @@ -2290,6 +2290,9 @@ static u8 skl_get_plane_caps(struct drm_i915_private *i915,
>>>    	if (HAS_4TILE(i915))
>>>    		caps |= INTEL_PLANE_CAP_TILING_4;
>>>    
>>> +	if (!IS_ENABLED(I915) && !HAS_FLAT_CCS(i915))
>>> +		return caps;
>>> +
>>>    	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>>>    		caps |= INTEL_PLANE_CAP_CCS_RC;
>>>    		if (DISPLAY_VER(i915) >= 12)
>>>
>>> I'm not saying that's exactly pretty, either, but IIUC this is supposed
>>> to be a temporary measure ("until they are fixed"), I'd rather take this
>>> small thing instead.
>>>
>>
>> Would that work when both i915 and Xe are being built?
> 
> IS_ENABLED(I915) will be false for xe build, true for i915 build. And
> HAS_FLAT_CCS() is defined for both, in different ways.
> 
> It's essentially the same as #ifdef but much less of an eyesore.
> 

ack. I'll put that into a patch that will replace this patch.

/Juha-Pekka
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index e777686190ca..c5e3c2dd0a01 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -302,6 +302,7 @@  i915-y += \
 	display/intel_overlay.o \
 	display/intel_pch_display.o \
 	display/intel_pch_refclk.o \
+	display/intel_plane_caps.o \
 	display/intel_plane_initial.o \
 	display/intel_pmdemand.o \
 	display/intel_psr.o \
diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
new file mode 100644
index 000000000000..6206ae11f296
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
@@ -0,0 +1,68 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#include "i915_drv.h"
+#include "intel_fb.h"
+#include "intel_plane_caps.h"
+
+static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
+				 enum pipe pipe, enum plane_id plane_id)
+{
+	/* Wa_22011186057 */
+	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
+		return false;
+
+	if (DISPLAY_VER(i915) >= 11)
+		return true;
+
+	if (IS_GEMINILAKE(i915))
+		return pipe != PIPE_C;
+
+	return pipe != PIPE_C &&
+		(plane_id == PLANE_PRIMARY ||
+		 plane_id == PLANE_SPRITE0);
+}
+
+static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
+				   enum plane_id plane_id)
+{
+	if (DISPLAY_VER(i915) < 12)
+		return false;
+
+	/* Wa_14010477008 */
+	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
+	    (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
+		return false;
+
+	/* Wa_22011186057 */
+	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
+		return false;
+
+	return plane_id < PLANE_SPRITE4;
+}
+
+u8 skl_get_plane_caps(struct drm_i915_private *i915,
+		      enum pipe pipe, enum plane_id plane_id)
+{
+	u8 caps = INTEL_PLANE_CAP_TILING_X;
+
+	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
+		caps |= INTEL_PLANE_CAP_TILING_Y;
+	if (DISPLAY_VER(i915) < 12)
+		caps |= INTEL_PLANE_CAP_TILING_Yf;
+	if (HAS_4TILE(i915))
+		caps |= INTEL_PLANE_CAP_TILING_4;
+
+	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
+		caps |= INTEL_PLANE_CAP_CCS_RC;
+		if (DISPLAY_VER(i915) >= 12)
+			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
+	}
+
+	if (gen12_plane_has_mc_ccs(i915, plane_id))
+		caps |= INTEL_PLANE_CAP_CCS_MC;
+
+	return caps;
+}
diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
new file mode 100644
index 000000000000..60a941c76f23
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
@@ -0,0 +1,14 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#ifndef __INTEL_PLANE_CAPS_H__
+#define __INTEL_PLANE_CAPS_H__
+
+#include "intel_display_types.h"
+
+u8 skl_get_plane_caps(struct drm_i915_private *i915,
+		      enum pipe pipe, enum plane_id plane_id);
+
+#endif /* __INTEL_PLANE_CAPS_H__ */
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 511dc1544854..f2fd3833c61d 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -17,6 +17,7 @@ 
 #include "intel_fb.h"
 #include "intel_fbc.h"
 #include "intel_frontbuffer.h"
+#include "intel_plane_caps.h"
 #include "intel_psr.h"
 #include "intel_psr_regs.h"
 #include "skl_scaler.h"
@@ -2242,66 +2243,6 @@  skl_plane_disable_flip_done(struct intel_plane *plane)
 	spin_unlock_irq(&i915->irq_lock);
 }
 
-static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
-				 enum pipe pipe, enum plane_id plane_id)
-{
-	/* Wa_22011186057 */
-	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
-		return false;
-
-	if (DISPLAY_VER(i915) >= 11)
-		return true;
-
-	if (IS_GEMINILAKE(i915))
-		return pipe != PIPE_C;
-
-	return pipe != PIPE_C &&
-		(plane_id == PLANE_PRIMARY ||
-		 plane_id == PLANE_SPRITE0);
-}
-
-static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
-				   enum plane_id plane_id)
-{
-	if (DISPLAY_VER(i915) < 12)
-		return false;
-
-	/* Wa_14010477008 */
-	if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
-		(IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
-		return false;
-
-	/* Wa_22011186057 */
-	if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
-		return false;
-
-	return plane_id < PLANE_SPRITE4;
-}
-
-static u8 skl_get_plane_caps(struct drm_i915_private *i915,
-			     enum pipe pipe, enum plane_id plane_id)
-{
-	u8 caps = INTEL_PLANE_CAP_TILING_X;
-
-	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
-		caps |= INTEL_PLANE_CAP_TILING_Y;
-	if (DISPLAY_VER(i915) < 12)
-		caps |= INTEL_PLANE_CAP_TILING_Yf;
-	if (HAS_4TILE(i915))
-		caps |= INTEL_PLANE_CAP_TILING_4;
-
-	if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
-		caps |= INTEL_PLANE_CAP_CCS_RC;
-		if (DISPLAY_VER(i915) >= 12)
-			caps |= INTEL_PLANE_CAP_CCS_RC_CC;
-	}
-
-	if (gen12_plane_has_mc_ccs(i915, plane_id))
-		caps |= INTEL_PLANE_CAP_CCS_MC;
-
-	return caps;
-}
-
 struct intel_plane *
 skl_universal_plane_create(struct drm_i915_private *dev_priv,
 			   enum pipe pipe, enum plane_id plane_id)
diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
index ccf83c12b545..425c6e6744a6 100644
--- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
+++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
@@ -15,6 +15,7 @@ 
 #include "intel_fb.h"
 #include "intel_fb_pin.h"
 #include "intel_frontbuffer.h"
+#include "intel_plane_caps.h"
 #include "intel_plane_initial.h"
 
 static bool
@@ -289,3 +290,25 @@  void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
 
 	plane_config_fini(&plane_config);
 }
+
+u8 skl_get_plane_caps(struct drm_i915_private *i915,
+		      enum pipe pipe, enum plane_id plane_id)
+{
+	u8 caps = INTEL_PLANE_CAP_TILING_X;
+
+	if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
+		caps |= INTEL_PLANE_CAP_TILING_Y;
+	if (DISPLAY_VER(i915) < 12)
+		caps |= INTEL_PLANE_CAP_TILING_Yf;
+	if (HAS_4TILE(i915))
+		caps |= INTEL_PLANE_CAP_TILING_4;
+
+	if (HAS_FLAT_CCS(i915)) {
+		caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
+
+		if (plane_id < PLANE_SPRITE4)
+			caps |= INTEL_PLANE_CAP_CCS_MC;
+	}
+
+	return caps;
+}