Re: [PATCH 01/13] drm/i915: Add skl+ plane name aliases to enum plane_id

2024-05-17 Thread Ville Syrjälä 
On Fri, May 17, 2024 at 06:33:46PM +0300, Jani Nikula wrote:
> On Thu, 16 May 2024, Ville Syrjala  wrote:
> > From: Ville Syrjälä 
> >
> > Using PLANE_PRIMARY + PLANE_SPRITE? on skl+ results in a bunch
> > of unnecessary head scratching. Add aliases using the skl+ plane
> > names.
> > And for pre-skl we only need to keep PRIMARY,SPRITE0,SPRITE1
> > as we only ever have 0-2 sprites per pipe on those platforms.
> 
> Should these be changed too?
> 
> - intel_plane_set_ckey()

I suppose one could consider splitting this to pre-skl
vs. skl+ variants and using the appropriate names
in each. But the whole ckey uapi is really designed
around the pre-skl single primary + single sprite world
view, so using the PLANE_PRIMARY name there seems OK.

> - for_each_plane_id_on_crtc()

There's not really a right answer here I guess. As 
long as it's 0 where we start this will work.


> I'm not sure. But there's one real issue:
> 
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c 
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 32d10e62b2b9..d0bfee2ca643 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -238,9 +238,9 @@ int skl_format_to_fourcc(int format, bool rgb_order, 
> > bool alpha)
> >  static u8 icl_nv12_y_plane_mask(struct drm_i915_private *i915)
> >  {
> > if (DISPLAY_VER(i915) >= 13 || HAS_D12_PLANE_MINIMIZATION(i915))
> > -   return BIT(PLANE_SPRITE2) | BIT(PLANE_SPRITE3);
> > +   return BIT(PLANE_6) | BIT(PLANE_7);
> > else
> > -   return BIT(PLANE_SPRITE4) | BIT(PLANE_SPRITE5);
> > +   return BIT(PLANE_4) | BIT(PLANE_5);
> 
> The if branches got swapped?

Yeah. Good catch. I suspect my brain was in the
"newer platforms surely have more things" mindset.

-- 
Ville Syrjälä
Intel

Re: [PATCH 01/13] drm/i915: Add skl+ plane name aliases to enum plane_id

2024-05-17 Thread Jani Nikula 
On Thu, 16 May 2024, Ville Syrjala  wrote:
> From: Ville Syrjälä 
>
> Using PLANE_PRIMARY + PLANE_SPRITE? on skl+ results in a bunch
> of unnecessary head scratching. Add aliases using the skl+ plane
> names.
> And for pre-skl we only need to keep PRIMARY,SPRITE0,SPRITE1
> as we only ever have 0-2 sprites per pipe on those platforms.

Should these be changed too?

- intel_plane_set_ckey()
- for_each_plane_id_on_crtc()

I'm not sure. But there's one real issue:

> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c 
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 32d10e62b2b9..d0bfee2ca643 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -238,9 +238,9 @@ int skl_format_to_fourcc(int format, bool rgb_order, bool 
> alpha)
>  static u8 icl_nv12_y_plane_mask(struct drm_i915_private *i915)
>  {
>   if (DISPLAY_VER(i915) >= 13 || HAS_D12_PLANE_MINIMIZATION(i915))
> - return BIT(PLANE_SPRITE2) | BIT(PLANE_SPRITE3);
> + return BIT(PLANE_6) | BIT(PLANE_7);
>   else
> - return BIT(PLANE_SPRITE4) | BIT(PLANE_SPRITE5);
> + return BIT(PLANE_4) | BIT(PLANE_5);

The if branches got swapped?

BR,
Jani.


-- 
Jani Nikula, Intel

[PATCH 01/13] drm/i915: Add skl+ plane name aliases to enum plane_id

2024-05-16 Thread Ville Syrjala 
From: Ville Syrjälä 

Using PLANE_PRIMARY + PLANE_SPRITE? on skl+ results in a bunch
of unnecessary head scratching. Add aliases using the skl+ plane
names.
And for pre-skl we only need to keep PRIMARY,SPRITE0,SPRITE1
as we only ever have 0-2 sprites per pipe on those platforms.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_crtc.c |  6 ++
 drivers/gpu/drm/i915/display/intel_display.c  |  8 +++
 .../drm/i915/display/intel_display_limits.h   | 21 ---
 .../gpu/drm/i915/display/intel_sprite_uapi.c  |  2 +-
 .../drm/i915/display/skl_universal_plane.c| 19 -
 5 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c 
b/drivers/gpu/drm/i915/display/intel_crtc.c
index 339010384b86..ca6dc1dc56c8 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -310,8 +310,7 @@ int intel_crtc_init(struct drm_i915_private *dev_priv, enum 
pipe pipe)
crtc->num_scalers = DISPLAY_RUNTIME_INFO(dev_priv)->num_scalers[pipe];
 
if (DISPLAY_VER(dev_priv) >= 9)
-   primary = skl_universal_plane_create(dev_priv, pipe,
-PLANE_PRIMARY);
+   primary = skl_universal_plane_create(dev_priv, pipe, PLANE_1);
else
primary = intel_primary_plane_create(dev_priv, pipe);
if (IS_ERR(primary)) {
@@ -326,8 +325,7 @@ int intel_crtc_init(struct drm_i915_private *dev_priv, enum 
pipe pipe)
struct intel_plane *plane;
 
if (DISPLAY_VER(dev_priv) >= 9)
-   plane = skl_universal_plane_create(dev_priv, pipe,
-  PLANE_SPRITE0 + 
sprite);
+   plane = skl_universal_plane_create(dev_priv, pipe, 
PLANE_2 + sprite);
else
plane = intel_sprite_plane_create(dev_priv, pipe, 
sprite);
if (IS_ERR(plane)) {
diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index cce1420fb541..ee2df655b0ab 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -4121,13 +4121,13 @@ static int icl_check_nv12_planes(struct 
intel_crtc_state *crtc_state)
linked_state->uapi.dst = plane_state->uapi.dst;
 
if (icl_is_hdr_plane(dev_priv, plane->id)) {
-   if (linked->id == PLANE_SPRITE5)
+   if (linked->id == PLANE_7)
plane_state->cus_ctl |= PLANE_CUS_Y_PLANE_7_ICL;
-   else if (linked->id == PLANE_SPRITE4)
+   else if (linked->id == PLANE_6)
plane_state->cus_ctl |= PLANE_CUS_Y_PLANE_6_ICL;
-   else if (linked->id == PLANE_SPRITE3)
+   else if (linked->id == PLANE_5)
plane_state->cus_ctl |= PLANE_CUS_Y_PLANE_5_RKL;
-   else if (linked->id == PLANE_SPRITE2)
+   else if (linked->id == PLANE_4)
plane_state->cus_ctl |= PLANE_CUS_Y_PLANE_4_RKL;
else
MISSING_CASE(linked->id);
diff --git a/drivers/gpu/drm/i915/display/intel_display_limits.h 
b/drivers/gpu/drm/i915/display/intel_display_limits.h
index 5126d0b5ae5d..c4775c99dc83 100644
--- a/drivers/gpu/drm/i915/display/intel_display_limits.h
+++ b/drivers/gpu/drm/i915/display/intel_display_limits.h
@@ -60,16 +60,23 @@ enum transcoder {
  * (eg. PLANE_CTL(), PS_PLANE_SEL(), etc.) so adjust with care.
  */
 enum plane_id {
-   PLANE_PRIMARY,
-   PLANE_SPRITE0,
-   PLANE_SPRITE1,
-   PLANE_SPRITE2,
-   PLANE_SPRITE3,
-   PLANE_SPRITE4,
-   PLANE_SPRITE5,
+   /* skl+ universal plane names */
+   PLANE_1,
+   PLANE_2,
+   PLANE_3,
+   PLANE_4,
+   PLANE_5,
+   PLANE_6,
+   PLANE_7,
+
PLANE_CURSOR,
 
I915_MAX_PLANES,
+
+   /* pre-skl plane names */
+   PLANE_PRIMARY = PLANE_1,
+   PLANE_SPRITE0,
+   PLANE_SPRITE1,
 };
 
 enum port {
diff --git a/drivers/gpu/drm/i915/display/intel_sprite_uapi.c 
b/drivers/gpu/drm/i915/display/intel_sprite_uapi.c
index a76b48ebc2d3..4853c4806004 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite_uapi.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite_uapi.c
@@ -74,7 +74,7 @@ int intel_sprite_set_colorkey_ioctl(struct drm_device *dev, 
void *data,
 * pipe simultaneously.
 */
if (DISPLAY_VER(dev_priv) >= 9 &&
-   to_intel_plane(plane)->id >= PLANE_SPRITE1 &&
+   to_intel_plane(plane)->id >= PLANE_3 &&
set->flags & I915_SET_COLORKEY_DESTINATION)
return -EINVAL;
 
diff --git