[PATCH v2 0/8] drm/i915: Fix up the CCS code

[PATCH v2 0/8] drm/i915: Fix up the CCS code

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Here's a rebased series of the remaining CCS patches. Apart from the rebase
and some commit message changes nothing has changed.

Ville Syrjälä (8):
  drm/i915: Add a comment exlaining CCS hsub/vsub
  drm/i915: Nuke a pointless unreachable()
  drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites
  drm/i915: Clean up the sprite modifier checks
  drm/i915: Add CCS capability for sprites
  drm/i915: Allow up to 32KB stride on SKL+ "sprites"
  drm: Check that the plane supports the request format+modifier combo
  drm/i915: Remove the pipe/plane ID checks from
    skl_check_ccs_aux_surface()

 drivers/gpu/drm/drm_atomic.c         |  10 ++--
 drivers/gpu/drm/drm_crtc.c           |  10 ++--
 drivers/gpu/drm/drm_crtc_internal.h  |   4 +-
 drivers/gpu/drm/drm_plane.c          |  33 ++++++++---
 drivers/gpu/drm/i915/intel_display.c |  36 +++---------
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  | 103 ++++++++++++++++++++++++-----------
 7 files changed, 121 insertions(+), 77 deletions(-)

-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Let's document why we claim hsub==8,vsub==16 for CCS even though the
memory layout would suggest that we use 16x8 instead.

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0cd355978ab4..83aec68537b4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2387,6 +2387,13 @@ static unsigned int intel_fb_modifier_to_tiling(uint64_t fb_modifier)
 	}
 }
 
+/*
+ * 1 byte of CCS actually corresponds to 16x8 pixels on the main
+ * surface, and the memory layout for the CCS tile is 64x64 bytes.
+ * But since we're pretending the CCS tile is 128 bytes wide we
+ * adjust hsub/vsub here accordingly to 8x16 so that the
+ * bytes<->x/y conversions come out correct.
+ */
 static const struct drm_format_info ccs_formats[] = {
 	{ .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
 	{ .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub

[Daniel Vetter]

On Fri, Dec 22, 2017 at 09:22:24PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Let's document why we claim hsub==8,vsub==16 for CCS even though the
> memory layout would suggest that we use 16x8 instead.
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Imo this needs an ack from Jason (or someone else who groks the CCS
layout), passsing on this one.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0cd355978ab4..83aec68537b4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2387,6 +2387,13 @@ static unsigned int intel_fb_modifier_to_tiling(uint64_t fb_modifier)
>  	}
>  }
>  
> +/*
> + * 1 byte of CCS actually corresponds to 16x8 pixels on the main
> + * surface, and the memory layout for the CCS tile is 64x64 bytes.
> + * But since we're pretending the CCS tile is 128 bytes wide we
> + * adjust hsub/vsub here accordingly to 8x16 so that the
> + * bytes<->x/y conversions come out correct.
> + */
>  static const struct drm_format_info ccs_formats[] = {
>  	{ .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
>  	{ .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
> -- 
> 2.13.6
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 2782 bytes --]

On Fri, Dec 22, 2017 at 11:22 AM, Ville Syrjala <
ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Let's document why we claim hsub==8,vsub==16 for CCS even though the
> memory layout would suggest that we use 16x8 instead.
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 0cd355978ab4..83aec68537b4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2387,6 +2387,13 @@ static unsigned int intel_fb_modifier_to_tiling(uint64_t
> fb_modifier)
>         }
>  }
>
> +/*
> + * 1 byte of CCS actually corresponds to 16x8 pixels on the main
> + * surface, and the memory layout for the CCS tile is 64x64 bytes.
> + * But since we're pretending the CCS tile is 128 bytes wide we
> + * adjust hsub/vsub here accordingly to 8x16 so that the
> + * bytes<->x/y conversions come out correct.
>

I'm not particularly happy with this comment as I think it pushes the
mental model for these calculations in the wrong direction.  The PRM says:

The Color Control Surface (CCS) contains the compression status of the
cache-line pairs. The
compression state of the cache-line pair is specified by 2 bits in the CCS.
Each CCS cache-line represents
an area on the main surface of 16 x16 sets of 128 byte Y-tiled
cache-line-pairs. CCS is always Y tiled.

If you understand that a "cache line pair" in the main surface is a
horizontally adjacent cache line pair (cl1_addr = cl0_addr + 512) and you
just accept the statement about Y-tiling, this is the correct calculation.
Calculating these things in terms of pixels is occasionally useful but is
the wrong mental model.  The cache line statement above both accurately
describes the layout of the CCS (at the cache line granularity) and scales
to other pixel formats which are not 32-bit.

I know that Ville and I have disagreed on this in the past but I don't
think adding comments about how we're "pretending the CCS tile is 128 bytes
wide" is making anything more clear.

--Jason


> + */
>  static const struct drm_format_info ccs_formats[] = {
>         { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2,
> .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
>         { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2,
> .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
> --
> 2.13.6
>
>

[-- Attachment #1.2: Type: text/html, Size: 3872 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub

[Ville Syrjälä]

On Wed, Jan 10, 2018 at 09:03:14AM -0800, Jason Ekstrand wrote:
> On Fri, Dec 22, 2017 at 11:22 AM, Ville Syrjala <
> ville.syrjala@linux.intel.com> wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Let's document why we claim hsub==8,vsub==16 for CCS even though the
> > memory layout would suggest that we use 16x8 instead.
> >
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > Cc: Daniel Stone <daniels@collabora.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 0cd355978ab4..83aec68537b4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2387,6 +2387,13 @@ static unsigned int intel_fb_modifier_to_tiling(uint64_t
> > fb_modifier)
> >         }
> >  }
> >
> > +/*
> > + * 1 byte of CCS actually corresponds to 16x8 pixels on the main
> > + * surface, and the memory layout for the CCS tile is 64x64 bytes.
> > + * But since we're pretending the CCS tile is 128 bytes wide we
> > + * adjust hsub/vsub here accordingly to 8x16 so that the
> > + * bytes<->x/y conversions come out correct.
> >
> 
> I'm not particularly happy with this comment as I think it pushes the
> mental model for these calculations in the wrong direction.  The PRM says:
> 
> The Color Control Surface (CCS) contains the compression status of the
> cache-line pairs. The
> compression state of the cache-line pair is specified by 2 bits in the CCS.
> Each CCS cache-line represents
> an area on the main surface of 16 x16 sets of 128 byte Y-tiled
> cache-line-pairs. CCS is always Y tiled.
> 
> If you understand that a "cache line pair" in the main surface is a
> horizontally adjacent cache line pair (cl1_addr = cl0_addr + 512) and you
> just accept the statement about Y-tiling, this is the correct calculation.
> Calculating these things in terms of pixels is occasionally useful but is
> the wrong mental model.  The cache line statement above both accurately
> describes the layout of the CCS (at the cache line granularity) and scales
> to other pixel formats which are not 32-bit.
> 
> I know that Ville and I have disagreed on this in the past but I don't
> think adding comments about how we're "pretending the CCS tile is 128 bytes
> wide" is making anything more clear.

I don't really see how talk about cachelines is going to help explain
the hsub/vsub values we use here.

But I don't really care that much. We could just leave them as magic
numbers if no one can come up with a clear explanation for them.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 3789 bytes --]

On Wed, Jan 10, 2018 at 9:48 AM, Ville Syrjälä <
ville.syrjala@linux.intel.com> wrote:

> On Wed, Jan 10, 2018 at 09:03:14AM -0800, Jason Ekstrand wrote:
> > On Fri, Dec 22, 2017 at 11:22 AM, Ville Syrjala <
> > ville.syrjala@linux.intel.com> wrote:
> >
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Let's document why we claim hsub==8,vsub==16 for CCS even though the
> > > memory layout would suggest that we use 16x8 instead.
> > >
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: Ben Widawsky <ben@bwidawsk.net>
> > > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > > Cc: Daniel Stone <daniels@collabora.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 0cd355978ab4..83aec68537b4 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2387,6 +2387,13 @@ static unsigned int intel_fb_modifier_to_tiling(
> uint64_t
> > > fb_modifier)
> > >         }
> > >  }
> > >
> > > +/*
> > > + * 1 byte of CCS actually corresponds to 16x8 pixels on the main
> > > + * surface, and the memory layout for the CCS tile is 64x64 bytes.
> > > + * But since we're pretending the CCS tile is 128 bytes wide we
> > > + * adjust hsub/vsub here accordingly to 8x16 so that the
> > > + * bytes<->x/y conversions come out correct.
> > >
> >
> > I'm not particularly happy with this comment as I think it pushes the
> > mental model for these calculations in the wrong direction.  The PRM
> says:
> >
> > The Color Control Surface (CCS) contains the compression status of the
> > cache-line pairs. The
> > compression state of the cache-line pair is specified by 2 bits in the
> CCS.
> > Each CCS cache-line represents
> > an area on the main surface of 16 x16 sets of 128 byte Y-tiled
> > cache-line-pairs. CCS is always Y tiled.
> >
> > If you understand that a "cache line pair" in the main surface is a
> > horizontally adjacent cache line pair (cl1_addr = cl0_addr + 512) and you
> > just accept the statement about Y-tiling, this is the correct
> calculation.
> > Calculating these things in terms of pixels is occasionally useful but is
> > the wrong mental model.  The cache line statement above both accurately
> > describes the layout of the CCS (at the cache line granularity) and
> scales
> > to other pixel formats which are not 32-bit.
> >
> > I know that Ville and I have disagreed on this in the past but I don't
> > think adding comments about how we're "pretending the CCS tile is 128
> bytes
> > wide" is making anything more clear.
>
> I don't really see how talk about cachelines is going to help explain
> the hsub/vsub values we use here.
>
> But I don't really care that much. We could just leave them as magic
> numbers if no one can come up with a clear explanation for them.
>

How about a comment like this:

/*From the Sky Lake PRM:
 *
 *    The Color Control Surface (CCS) contains the compression status of
the cache-line pairs. The
 *    compression state of the cache-line pair is specified by 2 bits in
the CCS. Each CCS cache-line represents
 *    an area on the main surface of 16 x16 sets of 128 byte Y-tiled
cache-line-pairs. CCS is always Y tiled.
 *
 * Since cache line pairs refers to horizontally adjacent cache lines, each
cache line in the CCS
 * corresponds to an area of 32x16 cache lines on the main surface.  Since
each pixel is 4 bytes,
 * this gives us a ratio of one byte in the CCS for each 8x16 pixels in the
main surface.
 */

[-- Attachment #1.2: Type: text/html, Size: 5195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub

[Ville Syrjälä]

On Thu, Jan 11, 2018 at 09:25:47PM -0800, Jason Ekstrand wrote:
> On Wed, Jan 10, 2018 at 9:48 AM, Ville Syrjälä <
> ville.syrjala@linux.intel.com> wrote:
> 
> > On Wed, Jan 10, 2018 at 09:03:14AM -0800, Jason Ekstrand wrote:
> > > On Fri, Dec 22, 2017 at 11:22 AM, Ville Syrjala <
> > > ville.syrjala@linux.intel.com> wrote:
> > >
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > Let's document why we claim hsub==8,vsub==16 for CCS even though the
> > > > memory layout would suggest that we use 16x8 instead.
> > > >
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Cc: Ben Widawsky <ben@bwidawsk.net>
> > > > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > > > Cc: Daniel Stone <daniels@collabora.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 0cd355978ab4..83aec68537b4 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -2387,6 +2387,13 @@ static unsigned int intel_fb_modifier_to_tiling(
> > uint64_t
> > > > fb_modifier)
> > > >         }
> > > >  }
> > > >
> > > > +/*
> > > > + * 1 byte of CCS actually corresponds to 16x8 pixels on the main
> > > > + * surface, and the memory layout for the CCS tile is 64x64 bytes.
> > > > + * But since we're pretending the CCS tile is 128 bytes wide we
> > > > + * adjust hsub/vsub here accordingly to 8x16 so that the
> > > > + * bytes<->x/y conversions come out correct.
> > > >
> > >
> > > I'm not particularly happy with this comment as I think it pushes the
> > > mental model for these calculations in the wrong direction.  The PRM
> > says:
> > >
> > > The Color Control Surface (CCS) contains the compression status of the
> > > cache-line pairs. The
> > > compression state of the cache-line pair is specified by 2 bits in the
> > CCS.
> > > Each CCS cache-line represents
> > > an area on the main surface of 16 x16 sets of 128 byte Y-tiled
> > > cache-line-pairs. CCS is always Y tiled.
> > >
> > > If you understand that a "cache line pair" in the main surface is a
> > > horizontally adjacent cache line pair (cl1_addr = cl0_addr + 512) and you
> > > just accept the statement about Y-tiling, this is the correct
> > calculation.
> > > Calculating these things in terms of pixels is occasionally useful but is
> > > the wrong mental model.  The cache line statement above both accurately
> > > describes the layout of the CCS (at the cache line granularity) and
> > scales
> > > to other pixel formats which are not 32-bit.
> > >
> > > I know that Ville and I have disagreed on this in the past but I don't
> > > think adding comments about how we're "pretending the CCS tile is 128
> > bytes
> > > wide" is making anything more clear.
> >
> > I don't really see how talk about cachelines is going to help explain
> > the hsub/vsub values we use here.
> >
> > But I don't really care that much. We could just leave them as magic
> > numbers if no one can come up with a clear explanation for them.
> >
> 
> How about a comment like this:
> 
> /*From the Sky Lake PRM:
>  *
>  *    The Color Control Surface (CCS) contains the compression status of
> the cache-line pairs. The
>  *    compression state of the cache-line pair is specified by 2 bits in
> the CCS. Each CCS cache-line represents
>  *    an area on the main surface of 16 x16 sets of 128 byte Y-tiled
> cache-line-pairs. CCS is always Y tiled.
>  *
>  * Since cache line pairs refers to horizontally adjacent cache lines, each
> cache line in the CCS
>  * corresponds to an area of 32x16 cache lines on the main surface.  Since
> each pixel is 4 bytes,
>  * this gives us a ratio of one byte in the CCS for each 8x16 pixels in the
> main surface.
>  */

Works for me. Should I just suck that into my patch, or you want to
submit it as a proper patch?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 4484 bytes --]

On Wed, Jan 17, 2018 at 12:20 PM, Ville Syrjälä <
ville.syrjala@linux.intel.com> wrote:

> On Thu, Jan 11, 2018 at 09:25:47PM -0800, Jason Ekstrand wrote:
> > On Wed, Jan 10, 2018 at 9:48 AM, Ville Syrjälä <
> > ville.syrjala@linux.intel.com> wrote:
> >
> > > On Wed, Jan 10, 2018 at 09:03:14AM -0800, Jason Ekstrand wrote:
> > > > On Fri, Dec 22, 2017 at 11:22 AM, Ville Syrjala <
> > > > ville.syrjala@linux.intel.com> wrote:
> > > >
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > Let's document why we claim hsub==8,vsub==16 for CCS even though
> the
> > > > > memory layout would suggest that we use 16x8 instead.
> > > > >
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > Cc: Ben Widawsky <ben@bwidawsk.net>
> > > > > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > > > > Cc: Daniel Stone <daniels@collabora.com>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 0cd355978ab4..83aec68537b4 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -2387,6 +2387,13 @@ static unsigned int
> intel_fb_modifier_to_tiling(
> > > uint64_t
> > > > > fb_modifier)
> > > > >         }
> > > > >  }
> > > > >
> > > > > +/*
> > > > > + * 1 byte of CCS actually corresponds to 16x8 pixels on the main
> > > > > + * surface, and the memory layout for the CCS tile is 64x64 bytes.
> > > > > + * But since we're pretending the CCS tile is 128 bytes wide we
> > > > > + * adjust hsub/vsub here accordingly to 8x16 so that the
> > > > > + * bytes<->x/y conversions come out correct.
> > > > >
> > > >
> > > > I'm not particularly happy with this comment as I think it pushes the
> > > > mental model for these calculations in the wrong direction.  The PRM
> > > says:
> > > >
> > > > The Color Control Surface (CCS) contains the compression status of
> the
> > > > cache-line pairs. The
> > > > compression state of the cache-line pair is specified by 2 bits in
> the
> > > CCS.
> > > > Each CCS cache-line represents
> > > > an area on the main surface of 16 x16 sets of 128 byte Y-tiled
> > > > cache-line-pairs. CCS is always Y tiled.
> > > >
> > > > If you understand that a "cache line pair" in the main surface is a
> > > > horizontally adjacent cache line pair (cl1_addr = cl0_addr + 512)
> and you
> > > > just accept the statement about Y-tiling, this is the correct
> > > calculation.
> > > > Calculating these things in terms of pixels is occasionally useful
> but is
> > > > the wrong mental model.  The cache line statement above both
> accurately
> > > > describes the layout of the CCS (at the cache line granularity) and
> > > scales
> > > > to other pixel formats which are not 32-bit.
> > > >
> > > > I know that Ville and I have disagreed on this in the past but I
> don't
> > > > think adding comments about how we're "pretending the CCS tile is 128
> > > bytes
> > > > wide" is making anything more clear.
> > >
> > > I don't really see how talk about cachelines is going to help explain
> > > the hsub/vsub values we use here.
> > >
> > > But I don't really care that much. We could just leave them as magic
> > > numbers if no one can come up with a clear explanation for them.
> > >
> >
> > How about a comment like this:
> >
> > /*From the Sky Lake PRM:
> >  *
> >  *    The Color Control Surface (CCS) contains the compression status of
> > the cache-line pairs. The
> >  *    compression state of the cache-line pair is specified by 2 bits in
> > the CCS. Each CCS cache-line represents
> >  *    an area on the main surface of 16 x16 sets of 128 byte Y-tiled
> > cache-line-pairs. CCS is always Y tiled.
> >  *
> >  * Since cache line pairs refers to horizontally adjacent cache lines,
> each
> > cache line in the CCS
> >  * corresponds to an area of 32x16 cache lines on the main surface.
> Since
> > each pixel is 4 bytes,
> >  * this gives us a ratio of one byte in the CCS for each 8x16 pixels in
> the
> > main surface.
> >  */
>
> Works for me. Should I just suck that into my patch, or you want to
> submit it as a proper patch?
>

Feel free to suck it in.

[-- Attachment #1.2: Type: text/html, Size: 6289 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Let's document why we claim hsub==8,vsub==16 for CCS.

v2: Replace my explanation with Jason's

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 91f3c0a64596..8d0d5d8753c0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2387,6 +2387,20 @@ static unsigned int intel_fb_modifier_to_tiling(uint64_t fb_modifier)
 	}
 }
 
+/*
+ * From the Sky Lake PRM:
+ * "The Color Control Surface (CCS) contains the compression status of
+ *  the cache-line pairs. The compression state of the cache-line pair
+ *  is specified by 2 bits in the CCS. Each CCS cache-line represents
+ *  an area on the main surface of 16 x16 sets of 128 byte Y-tiled
+ *  cache-line-pairs. CCS is always Y tiled."
+ *
+ * Since cache line pairs refers to horizontally adjacent cache lines,
+ * each cache line in the CCS corresponds to an area of 32x16 cache
+ * lines on the main surface. Since each pixel is 4 bytes, this gives
+ * us a ratio of one byte in the CCS for each 8x16 pixels in the
+ * main surface.
+ */
 static const struct drm_format_info ccs_formats[] = {
 	{ .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
 	{ .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 2033 bytes --]

Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

On Fri, Jan 19, 2018 at 6:41 AM, Ville Syrjala <
ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Let's document why we claim hsub==8,vsub==16 for CCS.
>
> v2: Replace my explanation with Jason's
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 91f3c0a64596..8d0d5d8753c0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2387,6 +2387,20 @@ static unsigned int intel_fb_modifier_to_tiling(uint64_t
> fb_modifier)
>         }
>  }
>
> +/*
> + * From the Sky Lake PRM:
> + * "The Color Control Surface (CCS) contains the compression status of
> + *  the cache-line pairs. The compression state of the cache-line pair
> + *  is specified by 2 bits in the CCS. Each CCS cache-line represents
> + *  an area on the main surface of 16 x16 sets of 128 byte Y-tiled
> + *  cache-line-pairs. CCS is always Y tiled."
> + *
> + * Since cache line pairs refers to horizontally adjacent cache lines,
> + * each cache line in the CCS corresponds to an area of 32x16 cache
> + * lines on the main surface. Since each pixel is 4 bytes, this gives
> + * us a ratio of one byte in the CCS for each 8x16 pixels in the
> + * main surface.
> + */
>  static const struct drm_format_info ccs_formats[] = {
>         { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2,
> .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
>         { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2,
> .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
> --
> 2.13.6
>
>

[-- Attachment #1.2: Type: text/html, Size: 2906 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/8] drm/i915: Add a comment exlaining CCS hsub/vsub

[Ville Syrjälä]

On Sat, Jan 20, 2018 at 09:39:13AM -0800, Jason Ekstrand wrote:
> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

Thanks for the comment and review ;) Patches 1-6 have now been pushed to
dinq.

> 
> On Fri, Jan 19, 2018 at 6:41 AM, Ville Syrjala <
> ville.syrjala@linux.intel.com> wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Let's document why we claim hsub==8,vsub==16 for CCS.
> >
> > v2: Replace my explanation with Jason's
> >
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > Cc: Daniel Stone <daniels@collabora.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 91f3c0a64596..8d0d5d8753c0 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2387,6 +2387,20 @@ static unsigned int intel_fb_modifier_to_tiling(uint64_t
> > fb_modifier)
> >         }
> >  }
> >
> > +/*
> > + * From the Sky Lake PRM:
> > + * "The Color Control Surface (CCS) contains the compression status of
> > + *  the cache-line pairs. The compression state of the cache-line pair
> > + *  is specified by 2 bits in the CCS. Each CCS cache-line represents
> > + *  an area on the main surface of 16 x16 sets of 128 byte Y-tiled
> > + *  cache-line-pairs. CCS is always Y tiled."
> > + *
> > + * Since cache line pairs refers to horizontally adjacent cache lines,
> > + * each cache line in the CCS corresponds to an area of 32x16 cache
> > + * lines on the main surface. Since each pixel is 4 bytes, this gives
> > + * us a ratio of one byte in the CCS for each 8x16 pixels in the
> > + * main surface.
> > + */
> >  static const struct drm_format_info ccs_formats[] = {
> >         { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2,
> > .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
> >         { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2,
> > .cpp = { 4, 1, }, .hsub = 8, .vsub = 16, },
> > --
> > 2.13.6
> >
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/8] drm/i915: Nuke a pointless unreachable()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The unreachable() is very much unreachable and the compiler knows
that, so there's no point in having it.

Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 83aec68537b4..0dd37c854820 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12951,8 +12951,6 @@ static bool intel_primary_plane_format_mod_supported(struct drm_plane *plane,
 		return i965_mod_supported(format, modifier);
 	else
 		return i8xx_mod_supported(format, modifier);
-
-	unreachable();
 }
 
 static bool intel_cursor_plane_format_mod_supported(struct drm_plane *plane,
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915: Nuke a pointless unreachable()

[Daniel Vetter]

On Fri, Dec 22, 2017 at 09:22:25PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The unreachable() is very much unreachable and the compiler knows
> that, so there's no point in having it.
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 83aec68537b4..0dd37c854820 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12951,8 +12951,6 @@ static bool intel_primary_plane_format_mod_supported(struct drm_plane *plane,
>  		return i965_mod_supported(format, modifier);
>  	else
>  		return i8xx_mod_supported(format, modifier);
> -
> -	unreachable();
>  }
>  
>  static bool intel_cursor_plane_format_mod_supported(struct drm_plane *plane,
> -- 
> 2.13.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/8] drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Y/Yf were dropped out from the SKL+ sprite modifier list on account
of some watermark issues Daniel Stone was having. My subsequent testing
seemed to indicate that things work better now, so add the modifiers
back in.

v2: Update the commit message with a better explanation

Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index dd485f59eb1d..51bd79ad647a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1162,6 +1162,8 @@ static uint32_t skl_plane_formats[] = {
 };
 
 static const uint64_t skl_plane_format_modifiers[] = {
+	I915_FORMAT_MOD_Yf_TILED,
+	I915_FORMAT_MOD_Y_TILED,
 	I915_FORMAT_MOD_X_TILED,
 	DRM_FORMAT_MOD_LINEAR,
 	DRM_FORMAT_MOD_INVALID
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/8] drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites

[Daniel Stone]

[-- Attachment #1: Type: text/html, Size: 365 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/8] drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites

[Daniel Vetter]

On Fri, Dec 22, 2017 at 09:22:26PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Y/Yf were dropped out from the SKL+ sprite modifier list on account
> of some watermark issues Daniel Stone was having. My subsequent testing
> seemed to indicate that things work better now, so add the modifiers
> back in.
> 
> v2: Update the commit message with a better explanation
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Do we have coverage for this in igt? If yes, then

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index dd485f59eb1d..51bd79ad647a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1162,6 +1162,8 @@ static uint32_t skl_plane_formats[] = {
>  };
>  
>  static const uint64_t skl_plane_format_modifiers[] = {
> +	I915_FORMAT_MOD_Yf_TILED,
> +	I915_FORMAT_MOD_Y_TILED,
>  	I915_FORMAT_MOD_X_TILED,
>  	DRM_FORMAT_MOD_LINEAR,
>  	DRM_FORMAT_MOD_INVALID
> -- 
> 2.13.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/8] drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites

[Ville Syrjälä]

On Wed, Jan 10, 2018 at 01:59:44PM +0100, Daniel Vetter wrote:
> On Fri, Dec 22, 2017 at 09:22:26PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Y/Yf were dropped out from the SKL+ sprite modifier list on account
> > of some watermark issues Daniel Stone was having. My subsequent testing
> > seemed to indicate that things work better now, so add the modifiers
> > back in.
> > 
> > v2: Update the commit message with a better explanation
> > 
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > Cc: Daniel Stone <daniels@collabora.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Do we have coverage for this in igt? If yes, then

Not sure what test you're actually after here. This only changes
what we expose in the blobifier, but it doesn't change what kind
modifiers the user can actually use with the sprites. Ie. Y/Yf
tiled framebuffers are accepted even before this patch. I do
believe we have some Y/Yf tests on arbitrary planes in some
of the plane tests.

We have no specific tests for blobifiers I think. kms_ccs does use
them to check for CCS support though. We should probably add something
to igt_kms to use them for figuring out which format+modifier combos
are supported by each plane.

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index dd485f59eb1d..51bd79ad647a 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1162,6 +1162,8 @@ static uint32_t skl_plane_formats[] = {
> >  };
> >  
> >  static const uint64_t skl_plane_format_modifiers[] = {
> > +	I915_FORMAT_MOD_Yf_TILED,
> > +	I915_FORMAT_MOD_Y_TILED,
> >  	I915_FORMAT_MOD_X_TILED,
> >  	DRM_FORMAT_MOD_LINEAR,
> >  	DRM_FORMAT_MOD_INVALID
> > -- 
> > 2.13.6
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/8] drm/i915: Clean up the sprite modifier checks

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Split the g4x and snb cases into separate functions to match how we deal
with all other platforms. Also sort the switch cases to match the format
lists we've declared earlier, to ease comparisons.

Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 48 ++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 51bd79ad647a..349be8134c76 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1169,12 +1169,9 @@ static const uint64_t skl_plane_format_modifiers[] = {
 	DRM_FORMAT_MOD_INVALID
 };
 
-static bool g4x_sprite_plane_format_mod_supported(struct drm_plane *plane,
-						  uint32_t format,
-						  uint64_t modifier)
+static bool g4x_mod_supported(uint32_t format, uint64_t modifier)
 {
 	switch (format) {
-	case DRM_FORMAT_XBGR8888:
 	case DRM_FORMAT_XRGB8888:
 	case DRM_FORMAT_YUYV:
 	case DRM_FORMAT_YVYU:
@@ -1189,22 +1186,38 @@ static bool g4x_sprite_plane_format_mod_supported(struct drm_plane *plane,
 	}
 }
 
-static bool vlv_sprite_plane_format_mod_supported(struct drm_plane *plane,
-						  uint32_t format,
-						  uint64_t modifier)
+static bool snb_mod_supported(uint32_t format, uint64_t modifier)
 {
 	switch (format) {
+	case DRM_FORMAT_XRGB8888:
+	case DRM_FORMAT_XBGR8888:
 	case DRM_FORMAT_YUYV:
 	case DRM_FORMAT_YVYU:
 	case DRM_FORMAT_UYVY:
 	case DRM_FORMAT_VYUY:
+		if (modifier == DRM_FORMAT_MOD_LINEAR ||
+		    modifier == I915_FORMAT_MOD_X_TILED)
+			return true;
+		/* fall through */
+	default:
+		return false;
+	}
+}
+
+static bool vlv_mod_supported(uint32_t format, uint64_t modifier)
+{
+	switch (format) {
 	case DRM_FORMAT_RGB565:
-	case DRM_FORMAT_XRGB8888:
+	case DRM_FORMAT_ABGR8888:
 	case DRM_FORMAT_ARGB8888:
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_XRGB8888:
 	case DRM_FORMAT_XBGR2101010:
 	case DRM_FORMAT_ABGR2101010:
-	case DRM_FORMAT_XBGR8888:
-	case DRM_FORMAT_ABGR8888:
+	case DRM_FORMAT_YUYV:
+	case DRM_FORMAT_YVYU:
+	case DRM_FORMAT_UYVY:
+	case DRM_FORMAT_VYUY:
 		if (modifier == DRM_FORMAT_MOD_LINEAR ||
 		    modifier == I915_FORMAT_MOD_X_TILED)
 			return true;
@@ -1214,11 +1227,8 @@ static bool vlv_sprite_plane_format_mod_supported(struct drm_plane *plane,
 	}
 }
 
-static bool skl_sprite_plane_format_mod_supported(struct drm_plane *plane,
-						  uint32_t format,
-						  uint64_t modifier)
+static bool skl_mod_supported(uint32_t format, uint64_t modifier)
 {
-	/* This is the same as primary plane since SKL has universal planes */
 	switch (format) {
 	case DRM_FORMAT_XRGB8888:
 	case DRM_FORMAT_XBGR8888:
@@ -1259,13 +1269,13 @@ static bool intel_sprite_plane_format_mod_supported(struct drm_plane *plane,
 		return false;
 
 	if (INTEL_GEN(dev_priv) >= 9)
-		return skl_sprite_plane_format_mod_supported(plane, format, modifier);
+		return skl_mod_supported(format, modifier);
 	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		return vlv_sprite_plane_format_mod_supported(plane, format, modifier);
+		return vlv_mod_supported(format, modifier);
+	else if (INTEL_GEN(dev_priv) >= 6)
+		return snb_mod_supported(format, modifier);
 	else
-		return g4x_sprite_plane_format_mod_supported(plane, format, modifier);
-
-	unreachable();
+		return g4x_mod_supported(format, modifier);
 }
 
 static const struct drm_plane_funcs intel_sprite_plane_funcs = {
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/8] drm/i915: Clean up the sprite modifier checks

[Daniel Vetter]

On Fri, Dec 22, 2017 at 09:22:27PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Split the g4x and snb cases into separate functions to match how we deal
> with all other platforms. Also sort the switch cases to match the format
> lists we've declared earlier, to ease comparisons.
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 48 ++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 51bd79ad647a..349be8134c76 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1169,12 +1169,9 @@ static const uint64_t skl_plane_format_modifiers[] = {
>  	DRM_FORMAT_MOD_INVALID
>  };
>  
> -static bool g4x_sprite_plane_format_mod_supported(struct drm_plane *plane,
> -						  uint32_t format,
> -						  uint64_t modifier)
> +static bool g4x_mod_supported(uint32_t format, uint64_t modifier)
>  {
>  	switch (format) {
> -	case DRM_FORMAT_XBGR8888:
>  	case DRM_FORMAT_XRGB8888:
>  	case DRM_FORMAT_YUYV:
>  	case DRM_FORMAT_YVYU:
> @@ -1189,22 +1186,38 @@ static bool g4x_sprite_plane_format_mod_supported(struct drm_plane *plane,
>  	}
>  }
>  
> -static bool vlv_sprite_plane_format_mod_supported(struct drm_plane *plane,
> -						  uint32_t format,
> -						  uint64_t modifier)
> +static bool snb_mod_supported(uint32_t format, uint64_t modifier)
>  {
>  	switch (format) {
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_XBGR8888:
>  	case DRM_FORMAT_YUYV:
>  	case DRM_FORMAT_YVYU:
>  	case DRM_FORMAT_UYVY:
>  	case DRM_FORMAT_VYUY:
> +		if (modifier == DRM_FORMAT_MOD_LINEAR ||
> +		    modifier == I915_FORMAT_MOD_X_TILED)
> +			return true;
> +		/* fall through */
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool vlv_mod_supported(uint32_t format, uint64_t modifier)
> +{
> +	switch (format) {
>  	case DRM_FORMAT_RGB565:
> -	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ABGR8888:
>  	case DRM_FORMAT_ARGB8888:
> +	case DRM_FORMAT_XBGR8888:
> +	case DRM_FORMAT_XRGB8888:
>  	case DRM_FORMAT_XBGR2101010:
>  	case DRM_FORMAT_ABGR2101010:
> -	case DRM_FORMAT_XBGR8888:
> -	case DRM_FORMAT_ABGR8888:
> +	case DRM_FORMAT_YUYV:
> +	case DRM_FORMAT_YVYU:
> +	case DRM_FORMAT_UYVY:
> +	case DRM_FORMAT_VYUY:
>  		if (modifier == DRM_FORMAT_MOD_LINEAR ||
>  		    modifier == I915_FORMAT_MOD_X_TILED)
>  			return true;
> @@ -1214,11 +1227,8 @@ static bool vlv_sprite_plane_format_mod_supported(struct drm_plane *plane,
>  	}
>  }
>  
> -static bool skl_sprite_plane_format_mod_supported(struct drm_plane *plane,
> -						  uint32_t format,
> -						  uint64_t modifier)
> +static bool skl_mod_supported(uint32_t format, uint64_t modifier)
>  {
> -	/* This is the same as primary plane since SKL has universal planes */
>  	switch (format) {
>  	case DRM_FORMAT_XRGB8888:
>  	case DRM_FORMAT_XBGR8888:
> @@ -1259,13 +1269,13 @@ static bool intel_sprite_plane_format_mod_supported(struct drm_plane *plane,
>  		return false;
>  
>  	if (INTEL_GEN(dev_priv) >= 9)
> -		return skl_sprite_plane_format_mod_supported(plane, format, modifier);
> +		return skl_mod_supported(format, modifier);
>  	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		return vlv_sprite_plane_format_mod_supported(plane, format, modifier);
> +		return vlv_mod_supported(format, modifier);
> +	else if (INTEL_GEN(dev_priv) >= 6)
> +		return snb_mod_supported(format, modifier);
>  	else
> -		return g4x_sprite_plane_format_mod_supported(plane, format, modifier);
> -
> -	unreachable();
> +		return g4x_mod_supported(format, modifier);
>  }
>  
>  static const struct drm_plane_funcs intel_sprite_plane_funcs = {
> -- 
> 2.13.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/8] drm/i915: Add CCS capability for sprites

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Allow sprites to scan out compressed framebuffers.

Since different platforms have a different set of planes that
support CCS let's add a small helper to determine whether a
specific plane supports CCS or not. Currently that information
is spread around in many places, and not all the pieces of
code even agree with each other.

In addition to allowing sprites to scan out compressed fbs,
the other fix here is that we stop rejecting them on pipe C
on CNL.

Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 25 ++++--------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_sprite.c  | 50 ++++++++++++++++++++++++++----------
 3 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0dd37c854820..df29e33a2355 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3034,6 +3034,7 @@ static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state)
 static int skl_check_ccs_aux_surface(struct intel_plane_state *plane_state)
 {
 	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	struct intel_crtc *crtc = to_intel_crtc(plane_state->base.crtc);
 	const struct drm_framebuffer *fb = plane_state->base.fb;
 	int src_x = plane_state->base.src.x1 >> 16;
@@ -3044,17 +3045,8 @@ static int skl_check_ccs_aux_surface(struct intel_plane_state *plane_state)
 	int y = src_y / vsub;
 	u32 offset;
 
-	switch (plane->id) {
-	case PLANE_PRIMARY:
-	case PLANE_SPRITE0:
-		break;
-	default:
-		DRM_DEBUG_KMS("RC support only on plane 1 and 2\n");
-		return -EINVAL;
-	}
-
-	if (crtc->pipe == PIPE_C) {
-		DRM_DEBUG_KMS("No RC support on pipe C\n");
+	if (!skl_plane_has_ccs(dev_priv, crtc->pipe, plane->id)) {
+		DRM_DEBUG_KMS("No RC support on %s\n", plane->base.name);
 		return -EINVAL;
 	}
 
@@ -13161,18 +13153,11 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
 	primary->check_plane = intel_check_primary_plane;
 
-	if (INTEL_GEN(dev_priv) >= 10) {
+	if (INTEL_GEN(dev_priv) >= 9) {
 		intel_primary_formats = skl_primary_formats;
 		num_formats = ARRAY_SIZE(skl_primary_formats);
-		modifiers = skl_format_modifiers_ccs;
 
-		primary->update_plane = skl_update_plane;
-		primary->disable_plane = skl_disable_plane;
-		primary->get_hw_state = skl_plane_get_hw_state;
-	} else if (INTEL_GEN(dev_priv) >= 9) {
-		intel_primary_formats = skl_primary_formats;
-		num_formats = ARRAY_SIZE(skl_primary_formats);
-		if (pipe < PIPE_C)
+		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_PRIMARY))
 			modifiers = skl_format_modifiers_ccs;
 		else
 			modifiers = skl_format_modifiers_noccs;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 30f791f89d64..059cabdcc026 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1932,6 +1932,8 @@ void skl_update_plane(struct intel_plane *plane,
 		      const struct intel_plane_state *plane_state);
 void skl_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc);
 bool skl_plane_get_hw_state(struct intel_plane *plane);
+bool skl_plane_has_ccs(struct drm_i915_private *dev_priv,
+		       enum pipe pipe, enum plane_id plane_id);
 
 /* intel_tv.c */
 void intel_tv_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 349be8134c76..cb06acff283d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1161,7 +1161,17 @@ static uint32_t skl_plane_formats[] = {
 	DRM_FORMAT_VYUY,
 };
 
-static const uint64_t skl_plane_format_modifiers[] = {
+static const uint64_t skl_plane_format_modifiers_noccs[] = {
+	I915_FORMAT_MOD_Yf_TILED,
+	I915_FORMAT_MOD_Y_TILED,
+	I915_FORMAT_MOD_X_TILED,
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_INVALID
+};
+
+static const uint64_t skl_plane_format_modifiers_ccs[] = {
+	I915_FORMAT_MOD_Yf_TILED_CCS,
+	I915_FORMAT_MOD_Y_TILED_CCS,
 	I915_FORMAT_MOD_Yf_TILED,
 	I915_FORMAT_MOD_Y_TILED,
 	I915_FORMAT_MOD_X_TILED,
@@ -1234,6 +1244,10 @@ static bool skl_mod_supported(uint32_t format, uint64_t modifier)
 	case DRM_FORMAT_XBGR8888:
 	case DRM_FORMAT_ARGB8888:
 	case DRM_FORMAT_ABGR8888:
+		if (modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||
+		    modifier == I915_FORMAT_MOD_Y_TILED_CCS)
+			return true;
+		/* fall through */
 	case DRM_FORMAT_RGB565:
 	case DRM_FORMAT_XRGB2101010:
 	case DRM_FORMAT_XBGR2101010:
@@ -1289,6 +1303,23 @@ static const struct drm_plane_funcs intel_sprite_plane_funcs = {
         .format_mod_supported = intel_sprite_plane_format_mod_supported,
 };
 
+bool skl_plane_has_ccs(struct drm_i915_private *dev_priv,
+		       enum pipe pipe, enum plane_id plane_id)
+{
+	if (plane_id == PLANE_CURSOR)
+		return false;
+
+	if (INTEL_GEN(dev_priv) >= 10)
+		return true;
+
+	if (IS_GEMINILAKE(dev_priv))
+		return pipe != PIPE_C;
+
+	return pipe != PIPE_C &&
+		(plane_id == PLANE_PRIMARY ||
+		 plane_id == PLANE_SPRITE0);
+}
+
 struct intel_plane *
 intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 			  enum pipe pipe, int plane)
@@ -1315,7 +1346,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 	}
 	intel_plane->base.state = &state->base;
 
-	if (INTEL_GEN(dev_priv) >= 10) {
+	if (INTEL_GEN(dev_priv) >= 9) {
 		intel_plane->can_scale = true;
 		state->scaler_id = -1;
 
@@ -1325,18 +1356,11 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 
 		plane_formats = skl_plane_formats;
 		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
-		modifiers = skl_plane_format_modifiers;
-	} else if (INTEL_GEN(dev_priv) >= 9) {
-		intel_plane->can_scale = true;
-		state->scaler_id = -1;
 
-		intel_plane->update_plane = skl_update_plane;
-		intel_plane->disable_plane = skl_disable_plane;
-		intel_plane->get_hw_state = skl_plane_get_hw_state;
-
-		plane_formats = skl_plane_formats;
-		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
-		modifiers = skl_plane_format_modifiers;
+		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_SPRITE0 + plane))
+			modifiers = skl_plane_format_modifiers_ccs;
+		else
+			modifiers = skl_plane_format_modifiers_noccs;
 	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		intel_plane->can_scale = false;
 		intel_plane->max_downscale = 1;
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/8] drm/i915: Add CCS capability for sprites

[Mika Kahola]

On Fri, 2017-12-22 at 21:22 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Allow sprites to scan out compressed framebuffers.
> 
> Since different platforms have a different set of planes that
> support CCS let's add a small helper to determine whether a
> specific plane supports CCS or not. Currently that information
> is spread around in many places, and not all the pieces of
> code even agree with each other.
> 
> In addition to allowing sprites to scan out compressed fbs,
> the other fix here is that we stop rejecting them on pipe C
> on CNL.
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Stone <daniels@collabora.com>

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 25 ++++--------------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_sprite.c  | 50
> ++++++++++++++++++++++++++----------
>  3 files changed, 44 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 0dd37c854820..df29e33a2355 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3034,6 +3034,7 @@ static int skl_check_nv12_aux_surface(struct
> intel_plane_state *plane_state)
>  static int skl_check_ccs_aux_surface(struct intel_plane_state
> *plane_state)
>  {
>  	struct intel_plane *plane = to_intel_plane(plane_state-
> >base.plane);
> +	struct drm_i915_private *dev_priv = to_i915(plane-
> >base.dev);
>  	struct intel_crtc *crtc = to_intel_crtc(plane_state-
> >base.crtc);
>  	const struct drm_framebuffer *fb = plane_state->base.fb;
>  	int src_x = plane_state->base.src.x1 >> 16;
> @@ -3044,17 +3045,8 @@ static int skl_check_ccs_aux_surface(struct
> intel_plane_state *plane_state)
>  	int y = src_y / vsub;
>  	u32 offset;
>  
> -	switch (plane->id) {
> -	case PLANE_PRIMARY:
> -	case PLANE_SPRITE0:
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("RC support only on plane 1 and 2\n");
> -		return -EINVAL;
> -	}
> -
> -	if (crtc->pipe == PIPE_C) {
> -		DRM_DEBUG_KMS("No RC support on pipe C\n");
> +	if (!skl_plane_has_ccs(dev_priv, crtc->pipe, plane->id)) {
> +		DRM_DEBUG_KMS("No RC support on %s\n", plane-
> >base.name);
>  		return -EINVAL;
>  	}
>  
> @@ -13161,18 +13153,11 @@ intel_primary_plane_create(struct
> drm_i915_private *dev_priv, enum pipe pipe)
>  	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
>  	primary->check_plane = intel_check_primary_plane;
>  
> -	if (INTEL_GEN(dev_priv) >= 10) {
> +	if (INTEL_GEN(dev_priv) >= 9) {
>  		intel_primary_formats = skl_primary_formats;
>  		num_formats = ARRAY_SIZE(skl_primary_formats);
> -		modifiers = skl_format_modifiers_ccs;
>  
> -		primary->update_plane = skl_update_plane;
> -		primary->disable_plane = skl_disable_plane;
> -		primary->get_hw_state = skl_plane_get_hw_state;
> -	} else if (INTEL_GEN(dev_priv) >= 9) {
> -		intel_primary_formats = skl_primary_formats;
> -		num_formats = ARRAY_SIZE(skl_primary_formats);
> -		if (pipe < PIPE_C)
> +		if (skl_plane_has_ccs(dev_priv, pipe,
> PLANE_PRIMARY))
>  			modifiers = skl_format_modifiers_ccs;
>  		else
>  			modifiers = skl_format_modifiers_noccs;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 30f791f89d64..059cabdcc026 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1932,6 +1932,8 @@ void skl_update_plane(struct intel_plane
> *plane,
>  		      const struct intel_plane_state *plane_state);
>  void skl_disable_plane(struct intel_plane *plane, struct intel_crtc
> *crtc);
>  bool skl_plane_get_hw_state(struct intel_plane *plane);
> +bool skl_plane_has_ccs(struct drm_i915_private *dev_priv,
> +		       enum pipe pipe, enum plane_id plane_id);
>  
>  /* intel_tv.c */
>  void intel_tv_init(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 349be8134c76..cb06acff283d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1161,7 +1161,17 @@ static uint32_t skl_plane_formats[] = {
>  	DRM_FORMAT_VYUY,
>  };
>  
> -static const uint64_t skl_plane_format_modifiers[] = {
> +static const uint64_t skl_plane_format_modifiers_noccs[] = {
> +	I915_FORMAT_MOD_Yf_TILED,
> +	I915_FORMAT_MOD_Y_TILED,
> +	I915_FORMAT_MOD_X_TILED,
> +	DRM_FORMAT_MOD_LINEAR,
> +	DRM_FORMAT_MOD_INVALID
> +};
> +
> +static const uint64_t skl_plane_format_modifiers_ccs[] = {
> +	I915_FORMAT_MOD_Yf_TILED_CCS,
> +	I915_FORMAT_MOD_Y_TILED_CCS,
>  	I915_FORMAT_MOD_Yf_TILED,
>  	I915_FORMAT_MOD_Y_TILED,
>  	I915_FORMAT_MOD_X_TILED,
> @@ -1234,6 +1244,10 @@ static bool skl_mod_supported(uint32_t format,
> uint64_t modifier)
>  	case DRM_FORMAT_XBGR8888:
>  	case DRM_FORMAT_ARGB8888:
>  	case DRM_FORMAT_ABGR8888:
> +		if (modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||
> +		    modifier == I915_FORMAT_MOD_Y_TILED_CCS)
> +			return true;
> +		/* fall through */
>  	case DRM_FORMAT_RGB565:
>  	case DRM_FORMAT_XRGB2101010:
>  	case DRM_FORMAT_XBGR2101010:
> @@ -1289,6 +1303,23 @@ static const struct drm_plane_funcs
> intel_sprite_plane_funcs = {
>          .format_mod_supported =
> intel_sprite_plane_format_mod_supported,
>  };
>  
> +bool skl_plane_has_ccs(struct drm_i915_private *dev_priv,
> +		       enum pipe pipe, enum plane_id plane_id)
> +{
> +	if (plane_id == PLANE_CURSOR)
> +		return false;
> +
> +	if (INTEL_GEN(dev_priv) >= 10)
> +		return true;
> +
> +	if (IS_GEMINILAKE(dev_priv))
> +		return pipe != PIPE_C;
> +
> +	return pipe != PIPE_C &&
> +		(plane_id == PLANE_PRIMARY ||
> +		 plane_id == PLANE_SPRITE0);
> +}
> +
>  struct intel_plane *
>  intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>  			  enum pipe pipe, int plane)
> @@ -1315,7 +1346,7 @@ intel_sprite_plane_create(struct
> drm_i915_private *dev_priv,
>  	}
>  	intel_plane->base.state = &state->base;
>  
> -	if (INTEL_GEN(dev_priv) >= 10) {
> +	if (INTEL_GEN(dev_priv) >= 9) {
>  		intel_plane->can_scale = true;
>  		state->scaler_id = -1;
>  
> @@ -1325,18 +1356,11 @@ intel_sprite_plane_create(struct
> drm_i915_private *dev_priv,
>  
>  		plane_formats = skl_plane_formats;
>  		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> -		modifiers = skl_plane_format_modifiers;
> -	} else if (INTEL_GEN(dev_priv) >= 9) {
> -		intel_plane->can_scale = true;
> -		state->scaler_id = -1;
>  
> -		intel_plane->update_plane = skl_update_plane;
> -		intel_plane->disable_plane = skl_disable_plane;
> -		intel_plane->get_hw_state = skl_plane_get_hw_state;
> -
> -		plane_formats = skl_plane_formats;
> -		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> -		modifiers = skl_plane_format_modifiers;
> +		if (skl_plane_has_ccs(dev_priv, pipe, PLANE_SPRITE0
> + plane))
> +			modifiers = skl_plane_format_modifiers_ccs;
> +		else
> +			modifiers =
> skl_plane_format_modifiers_noccs;
>  	} else if (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv)) {
>  		intel_plane->can_scale = false;
>  		intel_plane->max_downscale = 1;
-- 
Mika Kahola - Intel OTC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/8] drm/i915: Allow up to 32KB stride on SKL+ "sprites"

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

SKL+ "sprites" no longer have 16KB max stride limit that earlier
platforms had. Bump up the limit to 32KB.

Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index cb06acff283d..94188488db05 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -865,6 +865,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
 	struct drm_rect *src = &state->base.src;
 	struct drm_rect *dst = &state->base.dst;
 	const struct drm_rect *clip = &state->clip;
+	int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384;
 	int hscale, vscale;
 	int max_scale, min_scale;
 	bool can_scale;
@@ -885,7 +886,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
 	}
 
 	/* FIXME check all gen limits */
-	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) {
+	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > max_stride) {
 		DRM_DEBUG_KMS("Unsuitable framebuffer for plane\n");
 		return -EINVAL;
 	}
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/8] drm/i915: Allow up to 32KB stride on SKL+ "sprites"

[Daniel Vetter]

On Fri, Dec 22, 2017 at 09:22:29PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> SKL+ "sprites" no longer have 16KB max stride limit that earlier
> platforms had. Bump up the limit to 32KB.
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index cb06acff283d..94188488db05 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -865,6 +865,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
>  	struct drm_rect *src = &state->base.src;
>  	struct drm_rect *dst = &state->base.dst;
>  	const struct drm_rect *clip = &state->clip;
> +	int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384;
>  	int hscale, vscale;
>  	int max_scale, min_scale;
>  	bool can_scale;
> @@ -885,7 +886,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
>  	}
>  
>  	/* FIXME check all gen limits */
> -	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) {
> +	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > max_stride) {
>  		DRM_DEBUG_KMS("Unsuitable framebuffer for plane\n");
>  		return -EINVAL;

Since pre-gen9 sprites are special, and since we're already validating the
overall sprite limits in intel_fb_pitch_limit I'd push this into a new if
condition like

	if (gen < 9 && pichtes[0] > KB(16)) {
  		DRM_DEBUG_KMS("Invalid stride\n");
  		return -EINVAL;
	}

With or without that bikeshed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/8] drm/i915: Allow up to 32KB stride on SKL+ "sprites"

[Ville Syrjälä]

On Wed, Jan 10, 2018 at 02:03:21PM +0100, Daniel Vetter wrote:
> On Fri, Dec 22, 2017 at 09:22:29PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > SKL+ "sprites" no longer have 16KB max stride limit that earlier
> > platforms had. Bump up the limit to 32KB.
> > 
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > Cc: Daniel Stone <daniels@collabora.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index cb06acff283d..94188488db05 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -865,6 +865,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
> >  	struct drm_rect *src = &state->base.src;
> >  	struct drm_rect *dst = &state->base.dst;
> >  	const struct drm_rect *clip = &state->clip;
> > +	int max_stride = INTEL_GEN(dev_priv) >= 9 ? 32768 : 16384;
> >  	int hscale, vscale;
> >  	int max_scale, min_scale;
> >  	bool can_scale;
> > @@ -885,7 +886,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
> >  	}
> >  
> >  	/* FIXME check all gen limits */
> > -	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) {
> > +	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > max_stride) {
> >  		DRM_DEBUG_KMS("Unsuitable framebuffer for plane\n");
> >  		return -EINVAL;
> 
> Since pre-gen9 sprites are special, and since we're already validating the
> overall sprite limits in intel_fb_pitch_limit I'd push this into a new if
> condition like
> 
> 	if (gen < 9 && pichtes[0] > KB(16)) {
>   		DRM_DEBUG_KMS("Invalid stride\n");
>   		return -EINVAL;
> 	}

I think we'll want to clean up this mess by moving a lot of these
checks into platform specific code. We're also missing a bunch of
checks we should be doing (eg. cdclk vs. scaling limits on ilk-ivb).

> 
> With or without that bikeshed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/8] drm: Check that the plane supports the request format+modifier combo

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we only check that the plane supports the pixel format of the
fb we're about to feed to it. Extend it to check also the modifier, and
more specifically that the combination of the format and modifier is
supported.

Cc: dri-devel@lists.freedesktop.org
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 10 ++++++----
 drivers/gpu/drm/drm_crtc.c          | 10 ++++++----
 drivers/gpu/drm/drm_crtc_internal.h |  4 ++--
 drivers/gpu/drm/drm_plane.c         | 33 ++++++++++++++++++++++++++-------
 4 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b76d49218cf1..6e5c3ea1e43b 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -882,12 +882,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
 	}
 
 	/* Check whether this plane supports the fb pixel format. */
-	ret = drm_plane_check_pixel_format(plane, state->fb->format->format);
+	ret = drm_plane_check_pixel_format(plane, state->fb->format->format,
+					   state->fb->modifier);
 	if (ret) {
 		struct drm_format_name_buf format_name;
-		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
-		                 drm_get_format_name(state->fb->format->format,
-		                                     &format_name));
+		DRM_DEBUG_ATOMIC("Invalid pixel format %s, modifier 0x%llx\n",
+				 drm_get_format_name(state->fb->format->format,
+						     &format_name),
+				 state->fb->modifier);
 		return ret;
 	}
 
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f0556e654116..816c30a0f030 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -625,12 +625,14 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		 */
 		if (!crtc->primary->format_default) {
 			ret = drm_plane_check_pixel_format(crtc->primary,
-							   fb->format->format);
+							   fb->format->format,
+							   fb->modifier);
 			if (ret) {
 				struct drm_format_name_buf format_name;
-				DRM_DEBUG_KMS("Invalid pixel format %s\n",
-				              drm_get_format_name(fb->format->format,
-				                                  &format_name));
+				DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
+					      drm_get_format_name(fb->format->format,
+								  &format_name),
+					      fb->modifier);
 				goto out;
 			}
 		}
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index af00f42ba269..860968a64ae7 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -196,8 +196,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 /* drm_plane.c */
 int drm_plane_register_all(struct drm_device *dev);
 void drm_plane_unregister_all(struct drm_device *dev);
-int drm_plane_check_pixel_format(const struct drm_plane *plane,
-				 u32 format);
+int drm_plane_check_pixel_format(struct drm_plane *plane,
+				 u32 format, u64 modifier);
 
 /* drm_bridge.c */
 void drm_bridge_detach(struct drm_bridge *bridge);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 2c90519576a3..8ac19379fe99 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -545,16 +545,33 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 	return 0;
 }
 
-int drm_plane_check_pixel_format(const struct drm_plane *plane, u32 format)
+int drm_plane_check_pixel_format(struct drm_plane *plane,
+				 u32 format, u64 modifier)
 {
 	unsigned int i;
 
 	for (i = 0; i < plane->format_count; i++) {
 		if (format == plane->format_types[i])
-			return 0;
+			break;
+	}
+	if (i == plane->format_count)
+		return -EINVAL;
+
+	if (!plane->modifier_count)
+		return 0;
+
+	for (i = 0; i < plane->modifier_count; i++) {
+		if (modifier == plane->modifiers[i])
+			break;
 	}
+	if (i == plane->modifier_count)
+		return -EINVAL;
 
-	return -EINVAL;
+	if (plane->funcs->format_mod_supported &&
+	    !plane->funcs->format_mod_supported(plane, format, modifier))
+		return -EINVAL;
+
+	return 0;
 }
 
 /*
@@ -598,12 +615,14 @@ static int __setplane_internal(struct drm_plane *plane,
 	}
 
 	/* Check whether this plane supports the fb pixel format. */
-	ret = drm_plane_check_pixel_format(plane, fb->format->format);
+	ret = drm_plane_check_pixel_format(plane, fb->format->format,
+					   fb->modifier);
 	if (ret) {
 		struct drm_format_name_buf format_name;
-		DRM_DEBUG_KMS("Invalid pixel format %s\n",
-		              drm_get_format_name(fb->format->format,
-		                                  &format_name));
+		DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
+			      drm_get_format_name(fb->format->format,
+						  &format_name),
+			      fb->modifier);
 		goto out;
 	}
 
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 7/8] drm: Check that the plane supports the request format+modifier combo

[Daniel Vetter]

On Fri, Dec 22, 2017 at 09:22:30PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we only check that the plane supports the pixel format of the
> fb we're about to feed to it. Extend it to check also the modifier, and
> more specifically that the combination of the format and modifier is
> supported.
> 
> Cc: dri-devel@lists.freedesktop.org
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_atomic.c        | 10 ++++++----
>  drivers/gpu/drm/drm_crtc.c          | 10 ++++++----
>  drivers/gpu/drm/drm_crtc_internal.h |  4 ++--
>  drivers/gpu/drm/drm_plane.c         | 33 ++++++++++++++++++++++++++-------
>  4 files changed, 40 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b76d49218cf1..6e5c3ea1e43b 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -882,12 +882,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>  	}
>  
>  	/* Check whether this plane supports the fb pixel format. */
> -	ret = drm_plane_check_pixel_format(plane, state->fb->format->format);
> +	ret = drm_plane_check_pixel_format(plane, state->fb->format->format,
> +					   state->fb->modifier);
>  	if (ret) {
>  		struct drm_format_name_buf format_name;
> -		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
> -		                 drm_get_format_name(state->fb->format->format,
> -		                                     &format_name));
> +		DRM_DEBUG_ATOMIC("Invalid pixel format %s, modifier 0x%llx\n",
> +				 drm_get_format_name(state->fb->format->format,
> +						     &format_name),
> +				 state->fb->modifier);
>  		return ret;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index f0556e654116..816c30a0f030 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -625,12 +625,14 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  		 */
>  		if (!crtc->primary->format_default) {
>  			ret = drm_plane_check_pixel_format(crtc->primary,
> -							   fb->format->format);
> +							   fb->format->format,
> +							   fb->modifier);
>  			if (ret) {
>  				struct drm_format_name_buf format_name;
> -				DRM_DEBUG_KMS("Invalid pixel format %s\n",
> -				              drm_get_format_name(fb->format->format,
> -				                                  &format_name));
> +				DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
> +					      drm_get_format_name(fb->format->format,
> +								  &format_name),
> +					      fb->modifier);
>  				goto out;
>  			}
>  		}
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index af00f42ba269..860968a64ae7 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -196,8 +196,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  /* drm_plane.c */
>  int drm_plane_register_all(struct drm_device *dev);
>  void drm_plane_unregister_all(struct drm_device *dev);
> -int drm_plane_check_pixel_format(const struct drm_plane *plane,
> -				 u32 format);
> +int drm_plane_check_pixel_format(struct drm_plane *plane,
> +				 u32 format, u64 modifier);
>  
>  /* drm_bridge.c */
>  void drm_bridge_detach(struct drm_bridge *bridge);
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 2c90519576a3..8ac19379fe99 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -545,16 +545,33 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
>  	return 0;
>  }
>  
> -int drm_plane_check_pixel_format(const struct drm_plane *plane, u32 format)
> +int drm_plane_check_pixel_format(struct drm_plane *plane,
> +				 u32 format, u64 modifier)
>  {
>  	unsigned int i;
>  
>  	for (i = 0; i < plane->format_count; i++) {
>  		if (format == plane->format_types[i])
> -			return 0;
> +			break;
> +	}
> +	if (i == plane->format_count)
> +		return -EINVAL;
> +
> +	if (!plane->modifier_count)
> +		return 0;
> +
> +	for (i = 0; i < plane->modifier_count; i++) {
> +		if (modifier == plane->modifiers[i])
> +			break;
>  	}
> +	if (i == plane->modifier_count)
> +		return -EINVAL;
>  
> -	return -EINVAL;
> +	if (plane->funcs->format_mod_supported &&
> +	    !plane->funcs->format_mod_supported(plane, format, modifier))
> +		return -EINVAL;
> +
> +	return 0;
>  }
>  
>  /*
> @@ -598,12 +615,14 @@ static int __setplane_internal(struct drm_plane *plane,
>  	}
>  
>  	/* Check whether this plane supports the fb pixel format. */
> -	ret = drm_plane_check_pixel_format(plane, fb->format->format);
> +	ret = drm_plane_check_pixel_format(plane, fb->format->format,
> +					   fb->modifier);
>  	if (ret) {
>  		struct drm_format_name_buf format_name;
> -		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> -		              drm_get_format_name(fb->format->format,
> -		                                  &format_name));
> +		DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
> +			      drm_get_format_name(fb->format->format,
> +						  &format_name),
> +			      fb->modifier);
>  		goto out;
>  	}
>  
> -- 
> 2.13.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] drm: Check that the plane supports the request format+modifier combo

[Ville Syrjälä]

On Wed, Jan 10, 2018 at 02:04:57PM +0100, Daniel Vetter wrote:
> On Fri, Dec 22, 2017 at 09:22:30PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we only check that the plane supports the pixel format of the
> > fb we're about to feed to it. Extend it to check also the modifier, and
> > more specifically that the combination of the format and modifier is
> > supported.
> > 
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > Cc: Daniel Stone <daniels@collabora.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Finally pushed this (and the followup i915 patch) to drm-misc-next.
Thanks for the reviews.

> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c        | 10 ++++++----
> >  drivers/gpu/drm/drm_crtc.c          | 10 ++++++----
> >  drivers/gpu/drm/drm_crtc_internal.h |  4 ++--
> >  drivers/gpu/drm/drm_plane.c         | 33 ++++++++++++++++++++++++++-------
> >  4 files changed, 40 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index b76d49218cf1..6e5c3ea1e43b 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -882,12 +882,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
> >  	}
> >  
> >  	/* Check whether this plane supports the fb pixel format. */
> > -	ret = drm_plane_check_pixel_format(plane, state->fb->format->format);
> > +	ret = drm_plane_check_pixel_format(plane, state->fb->format->format,
> > +					   state->fb->modifier);
> >  	if (ret) {
> >  		struct drm_format_name_buf format_name;
> > -		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
> > -		                 drm_get_format_name(state->fb->format->format,
> > -		                                     &format_name));
> > +		DRM_DEBUG_ATOMIC("Invalid pixel format %s, modifier 0x%llx\n",
> > +				 drm_get_format_name(state->fb->format->format,
> > +						     &format_name),
> > +				 state->fb->modifier);
> >  		return ret;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index f0556e654116..816c30a0f030 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -625,12 +625,14 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> >  		 */
> >  		if (!crtc->primary->format_default) {
> >  			ret = drm_plane_check_pixel_format(crtc->primary,
> > -							   fb->format->format);
> > +							   fb->format->format,
> > +							   fb->modifier);
> >  			if (ret) {
> >  				struct drm_format_name_buf format_name;
> > -				DRM_DEBUG_KMS("Invalid pixel format %s\n",
> > -				              drm_get_format_name(fb->format->format,
> > -				                                  &format_name));
> > +				DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
> > +					      drm_get_format_name(fb->format->format,
> > +								  &format_name),
> > +					      fb->modifier);
> >  				goto out;
> >  			}
> >  		}
> > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> > index af00f42ba269..860968a64ae7 100644
> > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > @@ -196,8 +196,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  /* drm_plane.c */
> >  int drm_plane_register_all(struct drm_device *dev);
> >  void drm_plane_unregister_all(struct drm_device *dev);
> > -int drm_plane_check_pixel_format(const struct drm_plane *plane,
> > -				 u32 format);
> > +int drm_plane_check_pixel_format(struct drm_plane *plane,
> > +				 u32 format, u64 modifier);
> >  
> >  /* drm_bridge.c */
> >  void drm_bridge_detach(struct drm_bridge *bridge);
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 2c90519576a3..8ac19379fe99 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -545,16 +545,33 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
> >  	return 0;
> >  }
> >  
> > -int drm_plane_check_pixel_format(const struct drm_plane *plane, u32 format)
> > +int drm_plane_check_pixel_format(struct drm_plane *plane,
> > +				 u32 format, u64 modifier)
> >  {
> >  	unsigned int i;
> >  
> >  	for (i = 0; i < plane->format_count; i++) {
> >  		if (format == plane->format_types[i])
> > -			return 0;
> > +			break;
> > +	}
> > +	if (i == plane->format_count)
> > +		return -EINVAL;
> > +
> > +	if (!plane->modifier_count)
> > +		return 0;
> > +
> > +	for (i = 0; i < plane->modifier_count; i++) {
> > +		if (modifier == plane->modifiers[i])
> > +			break;
> >  	}
> > +	if (i == plane->modifier_count)
> > +		return -EINVAL;
> >  
> > -	return -EINVAL;
> > +	if (plane->funcs->format_mod_supported &&
> > +	    !plane->funcs->format_mod_supported(plane, format, modifier))
> > +		return -EINVAL;
> > +
> > +	return 0;
> >  }
> >  
> >  /*
> > @@ -598,12 +615,14 @@ static int __setplane_internal(struct drm_plane *plane,
> >  	}
> >  
> >  	/* Check whether this plane supports the fb pixel format. */
> > -	ret = drm_plane_check_pixel_format(plane, fb->format->format);
> > +	ret = drm_plane_check_pixel_format(plane, fb->format->format,
> > +					   fb->modifier);
> >  	if (ret) {
> >  		struct drm_format_name_buf format_name;
> > -		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> > -		              drm_get_format_name(fb->format->format,
> > -		                                  &format_name));
> > +		DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
> > +			      drm_get_format_name(fb->format->format,
> > +						  &format_name),
> > +			      fb->modifier);
> >  		goto out;
> >  	}
> >  
> > -- 
> > 2.13.6
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 8/8] drm/i915: Remove the pipe/plane ID checks from skl_check_ccs_aux_surface()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The core now checks that the plane supports the fb's format+modifier
combination, so we can drop the related checks from
skl_check_ccs_aux_surface(). These checks were specific to
SKL/KBL/BXT anyway.

Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index df29e33a2355..a037892ec92b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3033,9 +3033,6 @@ static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state)
 
 static int skl_check_ccs_aux_surface(struct intel_plane_state *plane_state)
 {
-	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
-	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
-	struct intel_crtc *crtc = to_intel_crtc(plane_state->base.crtc);
 	const struct drm_framebuffer *fb = plane_state->base.fb;
 	int src_x = plane_state->base.src.x1 >> 16;
 	int src_y = plane_state->base.src.y1 >> 16;
@@ -3045,11 +3042,6 @@ static int skl_check_ccs_aux_surface(struct intel_plane_state *plane_state)
 	int y = src_y / vsub;
 	u32 offset;
 
-	if (!skl_plane_has_ccs(dev_priv, crtc->pipe, plane->id)) {
-		DRM_DEBUG_KMS("No RC support on %s\n", plane->base.name);
-		return -EINVAL;
-	}
-
 	if (plane_state->base.rotation & ~(DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180)) {
 		DRM_DEBUG_KMS("RC support only with 0/180 degree rotation %x\n",
 			      plane_state->base.rotation);
-- 
2.13.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/8] drm/i915: Remove the pipe/plane ID checks from skl_check_ccs_aux_surface()

[Mika Kahola]

On Fri, 2017-12-22 at 21:22 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The core now checks that the plane supports the fb's format+modifier
> combination, so we can drop the related checks from
> skl_check_ccs_aux_surface(). These checks were specific to
> SKL/KBL/BXT anyway.
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Stone <daniels@collabora.com>

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index df29e33a2355..a037892ec92b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3033,9 +3033,6 @@ static int skl_check_nv12_aux_surface(struct
> intel_plane_state *plane_state)
>  
>  static int skl_check_ccs_aux_surface(struct intel_plane_state
> *plane_state)
>  {
> -	struct intel_plane *plane = to_intel_plane(plane_state-
> >base.plane);
> -	struct drm_i915_private *dev_priv = to_i915(plane-
> >base.dev);
> -	struct intel_crtc *crtc = to_intel_crtc(plane_state-
> >base.crtc);
>  	const struct drm_framebuffer *fb = plane_state->base.fb;
>  	int src_x = plane_state->base.src.x1 >> 16;
>  	int src_y = plane_state->base.src.y1 >> 16;
> @@ -3045,11 +3042,6 @@ static int skl_check_ccs_aux_surface(struct
> intel_plane_state *plane_state)
>  	int y = src_y / vsub;
>  	u32 offset;
>  
> -	if (!skl_plane_has_ccs(dev_priv, crtc->pipe, plane->id)) {
> -		DRM_DEBUG_KMS("No RC support on %s\n", plane-
> >base.name);
> -		return -EINVAL;
> -	}
> -
>  	if (plane_state->base.rotation & ~(DRM_MODE_ROTATE_0 |
> DRM_MODE_ROTATE_180)) {
>  		DRM_DEBUG_KMS("RC support only with 0/180 degree
> rotation %x\n",
>  			      plane_state->base.rotation);
-- 
Mika Kahola - Intel OTC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Fix up the CCS code (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Fix up the CCS code (rev2)
URL   : https://patchwork.freedesktop.org/series/29308/
State : success

== Summary ==

Series 29308v2 drm/i915: Fix up the CCS code
https://patchwork.freedesktop.org/api/1.0/series/29308/revisions/2/mbox/

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                dmesg-warn -> PASS       (fi-skl-6700hq) fdo#101144

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:434s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:439s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:386s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:499s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:277s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:488s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:496s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:476s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:470s
fi-elk-e7500     total:224  pass:163  dwarn:15  dfail:0   fail:0   skip:45 
fi-gdg-551       total:288  pass:180  dwarn:0   dfail:0   fail:0   skip:108 time:260s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:528s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:406s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:419s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:425s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:475s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:431s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:479s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:518s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:464s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:532s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:441s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:531s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:556s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:504s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:492s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:445s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:546s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:408s
Blacklisted hosts:
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:595s
fi-cnl-y         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:635s
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:482s

bd13c6b3b9bc8c5ec725ad7fe773a83d270145c0 drm-tip: 2017y-12m-22d-19h-41m-40s UTC integration manifest
a34bbb4ca18b drm/i915: Remove the pipe/plane ID checks from skl_check_ccs_aux_surface()
283a23e47f0b drm: Check that the plane supports the request format+modifier combo
684bcae393c7 drm/i915: Allow up to 32KB stride on SKL+ "sprites"
a58eefb83cc3 drm/i915: Add CCS capability for sprites
8670cd77dd9d drm/i915: Clean up the sprite modifier checks
75b9e7ea7a6e drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites
493e8d3adfd7 drm/i915: Nuke a pointless unreachable()
0e7bd3ce9e82 drm/i915: Add a comment exlaining CCS hsub/vsub

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7573/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for drm/i915: Fix up the CCS code (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Fix up the CCS code (rev2)
URL   : https://patchwork.freedesktop.org/series/29308/
State : warning

== Summary ==

Test perf:
        Subgroup polling:
                fail       -> PASS       (shard-hsw) fdo#102252
Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-c-planes:
                fail       -> INCOMPLETE (shard-hsw) fdo#103375 +1
Test pm_rc6_residency:
        Subgroup rc6-accuracy:
                pass       -> SKIP       (shard-snb)
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-blt:
                pass       -> FAIL       (shard-snb) fdo#101623 +1
        Subgroup fbc-2p-primscrn-pri-indfb-draw-blt:
                notrun     -> INCOMPLETE (shard-hsw)
Test gem_tiled_swapping:
        Subgroup non-threaded:
                incomplete -> PASS       (shard-hsw) fdo#104218

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#104218 https://bugs.freedesktop.org/show_bug.cgi?id=104218

shard-hsw        total:2576 pass:1459 dwarn:1   dfail:0   fail:10  skip:1104 time:8754s
shard-snb        total:2713 pass:1310 dwarn:1   dfail:0   fail:11  skip:1391 time:8042s
Blacklisted hosts:
shard-apl        total:2713 pass:1687 dwarn:1   dfail:0   fail:25  skip:999 time:13703s
shard-kbl        total:2560 pass:1705 dwarn:1   dfail:0   fail:27  skip:827 time:10521s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7573/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Fix up the CCS code (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Fix up the CCS code (rev3)
URL   : https://patchwork.freedesktop.org/series/29308/
State : failure

== Summary ==

Applying: drm/i915: Add a comment exlaining CCS hsub/vsub
Applying: drm/i915: Nuke a pointless unreachable()
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_display.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_sprite.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_sprite.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_sprite.c
Patch failed at 0003 drm/i915: Add the missing Y/Yf modifiers for SKL+ sprites
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx