diff mbox series

[v7,7/7] drm/i915/tgl: Add Clear Color support for TGL Render Decompression

Message ID 20191126002635.5779-8-radhakrishna.sripada@intel.com (mailing list archive)
State New, archived
Headers show
Series Clear Color Support for TGL Render Decompression | expand

Commit Message

Sripada, Radhakrishna Nov. 26, 2019, 12:26 a.m. UTC 
Render Decompression is supported with Y-Tiled main surface. The CCS is
linear and has 4 bits of data for each main surface cache line pair, a
ratio of 1:256. Additional Clear Color information is passed from the
user-space through an offset in the GEM BO. Add a new modifier to identify
and parse new Clear Color information and extend Gen12 render decompression
functionality to the newly added modifier.

v2: Fix has_alpha flag for modifiers, omit CC modifier during initial
    plane config(Matt). Fix Lookup error.
v3: Fix the panic while running kms_cube
v4: Add alignment check and reuse the comments for ge12_ccs_formats(Matt)
v5: Fix typos and wrap comments(Matt)
v6: Rebase
v7: Rebase, Add missing case in intel_tile_height(RK)

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Ville Syrjala <ville.syrjala@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Rafael Antognolli <rafael.antognolli@intel.com>
Cc: Nanley G Chery <nanley.g.chery@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++++++++++-
 .../drm/i915/display/intel_display_types.h    |  3 +
 drivers/gpu/drm/i915/display/intel_sprite.c   | 10 ++-
 drivers/gpu/drm/i915/i915_reg.h               | 12 ++++
 4 files changed, 86 insertions(+), 3 deletions(-)

Comments

Matt Roper Nov. 26, 2019, 8:48 p.m. UTC | #1 
On Mon, Nov 25, 2019 at 04:26:35PM -0800, Radhakrishna Sripada wrote:
> Render Decompression is supported with Y-Tiled main surface. The CCS is
> linear and has 4 bits of data for each main surface cache line pair, a
> ratio of 1:256. Additional Clear Color information is passed from the
> user-space through an offset in the GEM BO. Add a new modifier to identify
> and parse new Clear Color information and extend Gen12 render decompression
> functionality to the newly added modifier.
> 
> v2: Fix has_alpha flag for modifiers, omit CC modifier during initial
>     plane config(Matt). Fix Lookup error.
> v3: Fix the panic while running kms_cube
> v4: Add alignment check and reuse the comments for ge12_ccs_formats(Matt)
> v5: Fix typos and wrap comments(Matt)
> v6: Rebase
> v7: Rebase, Add missing case in intel_tile_height(RK)
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Ville Syrjala <ville.syrjala@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> Cc: Nanley G Chery <nanley.g.chery@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>

A few minor comments below, but none of them are critical.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

What's the current status of the underlying compression series this work
is based on?

> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++++++++++-
>  .../drm/i915/display/intel_display_types.h    |  3 +
>  drivers/gpu/drm/i915/display/intel_sprite.c   | 10 ++-
>  drivers/gpu/drm/i915/i915_reg.h               | 12 ++++
>  4 files changed, 86 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 6c4274c1564d..69e70be86f57 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1975,6 +1975,10 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane)
>  		if (color_plane == 1)
>  			return 64;
>  		/* fall through */

Since this one (GEN12_RC_CCS) now falls through to the new
GEN12_RC_CCS_CC, you can probably drop these lines since they'll be
handled the same way in the next case down.

> +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> +		if (color_plane == 1 || color_plane == 2)

I've reviewed a lot of versions of these compression series so I might
be getting confused, but didn't DK have a version at one time that added
some kind of is_ccs_plane(mod, plane) helper function?  I'm not sure
what happened to that, but it did seem like a nice way to consolidate
the "these planes contain CCS stuff" logic rather than having to
hardcode the plane ID's all over the place like this.


> +			return 64;
> +		/* fall through */
>  	case I915_FORMAT_MOD_Y_TILED:
>  		if (IS_GEN(dev_priv, 2) || HAS_128_BYTE_Y_TILING(dev_priv))
>  			return 128;
> @@ -2013,6 +2017,10 @@ intel_tile_height(const struct drm_framebuffer *fb, int color_plane)
>  		if (color_plane == 1)
>  			return 1;
>  		/* fall through */

As above, you can now consolidate this with the new block below if you
want.

> +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> +		if (color_plane == 1 || color_plane == 2)
> +			return 1;
> +		/* fall through */
>  	default:
>  		return intel_tile_size(to_i915(fb->dev)) /
>  			intel_tile_width_bytes(fb, color_plane);
> @@ -2116,6 +2124,7 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
>  			return 256 * 1024;
>  		return 0;
>  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
>  		return 16 * 1024;
>  	case I915_FORMAT_MOD_Y_TILED_CCS:
>  	case I915_FORMAT_MOD_Yf_TILED_CCS:
> @@ -2313,8 +2322,16 @@ static u32 intel_adjust_tile_offset(int *x, int *y,
>  
>  static bool is_surface_linear(u64 modifier, int color_plane)
>  {
> -	return modifier == DRM_FORMAT_MOD_LINEAR ||
> -	       (modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS && color_plane == 1);
> +	switch (modifier) {
> +	case DRM_FORMAT_MOD_LINEAR:
> +		return true;
> +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> +		return color_plane == 1;
> +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> +		return color_plane == 1 || color_plane == 2;
> +	default:
> +		return false;
> +	}
>  }
>  
>  static u32 intel_adjust_aligned_offset(int *x, int *y,
> @@ -2502,6 +2519,7 @@ static unsigned int intel_fb_modifier_to_tiling(u64 fb_modifier)
>  	case I915_FORMAT_MOD_Y_TILED:
>  	case I915_FORMAT_MOD_Y_TILED_CCS:
>  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
>  		return I915_TILING_Y;
>  	default:
>  		return I915_TILING_NONE;
> @@ -2551,6 +2569,21 @@ static const struct drm_format_info gen12_ccs_formats[] = {
>  	  .cpp = { 4, 1, }, .hsub = 2, .vsub = 32, .has_alpha = true },
>  };
>  
> +/*
> + * Same as gen12_ccs_formats[] above, but with additional surface used
> + * to pass Clear Color information in plane 2 with 64 bits of data.
> + */
> +static const struct drm_format_info gen12_ccs_cc_formats[] = {
> +	{ .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 3,
> +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
> +	{ .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 3,
> +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
> +	{ .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 3,
> +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true },
> +	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
> +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true },
> +};
> +
>  static const struct drm_format_info *
>  lookup_format_info(const struct drm_format_info formats[],
>  		   int num_formats, u32 format)
> @@ -2578,6 +2611,10 @@ intel_get_format_info(const struct drm_mode_fb_cmd2 *cmd)
>  		return lookup_format_info(gen12_ccs_formats,
>  					  ARRAY_SIZE(gen12_ccs_formats),
>  					  cmd->pixel_format);
> +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> +		return lookup_format_info(gen12_ccs_cc_formats,
> +					  ARRAY_SIZE(gen12_ccs_cc_formats),
> +					  cmd->pixel_format);
>  	default:
>  		return NULL;
>  	}
> @@ -2586,6 +2623,7 @@ intel_get_format_info(const struct drm_mode_fb_cmd2 *cmd)
>  bool is_ccs_modifier(u64 modifier)
>  {
>  	return modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
> +	       modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC ||
>  	       modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
>  	       modifier == I915_FORMAT_MOD_Yf_TILED_CCS;
>  }
> @@ -2798,6 +2836,18 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
>  		int x, y;
>  		int ret;
>  
> +		/*
> +		 * Plane 2 of Render Compression with Clear Color fb modifier
> +		 * is consumed by the driver and not passed to DE. Skip the
> +		 * arithmetic related to alignment and offset calculation.
> +		 */
> +		if (i == 2 && fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {

This line should be wrapped.

> +			if (IS_ALIGNED(fb->offsets[2], PAGE_SIZE))
> +				continue;
> +			else
> +				return -EINVAL;
> +		}
> +
>  		cpp = fb->format->cpp[i];
>  		width = drm_framebuffer_plane_width(fb->width, fb, i);
>  		height = drm_framebuffer_plane_height(fb->height, fb, i);
> @@ -4295,6 +4345,7 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier)
>  	case I915_FORMAT_MOD_Y_TILED:
>  		return PLANE_CTL_TILED_Y;
>  	case I915_FORMAT_MOD_Y_TILED_CCS:
> +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
>  		return PLANE_CTL_TILED_Y | PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
>  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
>  		return PLANE_CTL_TILED_Y |
> @@ -15245,6 +15296,15 @@ static int intel_plane_pin_fb(struct intel_plane_state *plane_state)
>  
>  	plane_state->vma = vma;
>  
> +	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
> +		u32 *ccaddr = kmap_atomic(i915_gem_object_get_page(intel_fb_obj(fb),
> +								  fb->offsets[2] >> PAGE_SHIFT));
> +
> +		plane_state->ccval = ((u64)*(ccaddr + CC_VAL_HIGHER_OFFSET) << 32)
> +				     | *(ccaddr + CC_VAL_LOWER_OFFSET);

Isn't this just the same thing as

        u64 *ccaddr = kmap_atomic(...);
        plane_state->ccval = ccaddr[2];

?

> +		kunmap_atomic(ccaddr);
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 83ea04149b77..e1bd18122ce6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -607,6 +607,9 @@ struct intel_plane_state {
>  	u32 planar_slave;
>  
>  	struct drm_intel_sprite_colorkey ckey;
> +
> +	/* Clear Color Value */
> +	u64 ccval;
>  };
>  
>  struct intel_initial_plane_config {
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index 67a90059900f..89f7fbc87ad9 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -597,6 +597,7 @@ skl_program_plane(struct intel_plane *plane,
>  	unsigned long irqflags;
>  	u32 keymsk, keymax;
>  	u32 plane_ctl = plane_state->ctl;
> +	u64 ccval = plane_state->ccval;
>  
>  	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
>  
> @@ -639,6 +640,10 @@ skl_program_plane(struct intel_plane *plane,
>  	if (fb->format->is_yuv && icl_is_hdr_plane(dev_priv, plane_id))
>  		icl_program_input_csc(plane, crtc_state, plane_state);
>  
> +	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC)
> +		intel_uncore_write64_fw(&dev_priv->uncore,
> +					PLANE_CC_VAL(pipe, plane_id), ccval);
> +
>  	skl_write_plane_wm(plane, crtc_state);
>  
>  	I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
> @@ -2106,7 +2111,8 @@ static int skl_plane_check_fb(const struct intel_crtc_state *crtc_state,
>  	     fb->modifier == I915_FORMAT_MOD_Yf_TILED ||
>  	     fb->modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
>  	     fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||
> -	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS)) {
> +	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
> +	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC)) {
>  		DRM_DEBUG_KMS("Y/Yf tiling not supported in IF-ID mode\n");
>  		return -EINVAL;
>  	}
> @@ -2579,6 +2585,7 @@ static const u64 skl_plane_format_modifiers_ccs[] = {
>  
>  static const u64 gen12_plane_format_modifiers_ccs[] = {
>  	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> +	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC,
>  	I915_FORMAT_MOD_Y_TILED,
>  	I915_FORMAT_MOD_X_TILED,
>  	DRM_FORMAT_MOD_LINEAR,
> @@ -2750,6 +2757,7 @@ static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
>  	case I915_FORMAT_MOD_X_TILED:
>  	case I915_FORMAT_MOD_Y_TILED:
>  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
>  		break;
>  	default:
>  		return false;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2e444a18ed0b..4a683db267c1 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6840,6 +6840,8 @@ enum {
>  #define _PLANE_KEYMAX_1_A			0x701a0
>  #define _PLANE_KEYMAX_2_A			0x702a0
>  #define  PLANE_KEYMAX_ALPHA(a)			((a) << 24)
> +#define _PLANE_CC_VAL_1_A			0x701b4
> +#define _PLANE_CC_VAL_2_A			0x702b4
>  #define _PLANE_AUX_DIST_1_A			0x701c0
>  #define _PLANE_AUX_DIST_2_A			0x702c0
>  #define _PLANE_AUX_OFFSET_1_A			0x701c4
> @@ -6879,6 +6881,16 @@ enum {
>  #define _PLANE_NV12_BUF_CFG_1_A		0x70278
>  #define _PLANE_NV12_BUF_CFG_2_A		0x70378
>  
> +#define _PLANE_CC_VAL_1_B			0x711b4
> +#define _PLANE_CC_VAL_2_B			0x712b4
> +#define _PLANE_CC_VAL_1(pipe)	_PIPE(pipe, _PLANE_CC_VAL_1_A, _PLANE_CC_VAL_1_B)
> +#define _PLANE_CC_VAL_2(pipe)	_PIPE(pipe, _PLANE_CC_VAL_2_A, _PLANE_CC_VAL_2_B)
> +#define PLANE_CC_VAL(pipe, plane)	\
> +	_MMIO_PLANE(plane, _PLANE_CC_VAL_1(pipe), _PLANE_CC_VAL_2(pipe))
> +
> +#define CC_VAL_LOWER_OFFSET		4
> +#define CC_VAL_HIGHER_OFFSET		5
> +
>  /* Input CSC Register Definitions */
>  #define _PLANE_INPUT_CSC_RY_GY_1_A	0x701E0
>  #define _PLANE_INPUT_CSC_RY_GY_2_A	0x702E0
> -- 
> 2.20.1
>
Sripada, Radhakrishna Nov. 26, 2019, 9:52 p.m. UTC | #2 
Hi Matt,

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, November 26, 2019 12:49 PM
> To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
> Sharma, Shashank <shashank.sharma@intel.com>; Antognolli, Rafael
> <rafael.antognolli@intel.com>; Chery, Nanley G <nanley.g.chery@intel.com>
> Subject: Re: [PATCH v7 7/7] drm/i915/tgl: Add Clear Color support for TGL
> Render Decompression
> 
> On Mon, Nov 25, 2019 at 04:26:35PM -0800, Radhakrishna Sripada wrote:
> > Render Decompression is supported with Y-Tiled main surface. The CCS
> > is linear and has 4 bits of data for each main surface cache line
> > pair, a ratio of 1:256. Additional Clear Color information is passed
> > from the user-space through an offset in the GEM BO. Add a new
> > modifier to identify and parse new Clear Color information and extend
> > Gen12 render decompression functionality to the newly added modifier.
> >
> > v2: Fix has_alpha flag for modifiers, omit CC modifier during initial
> >     plane config(Matt). Fix Lookup error.
> > v3: Fix the panic while running kms_cube
> > v4: Add alignment check and reuse the comments for
> > ge12_ccs_formats(Matt)
> > v5: Fix typos and wrap comments(Matt)
> > v6: Rebase
> > v7: Rebase, Add missing case in intel_tile_height(RK)
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@intel.com>
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > Cc: Nanley G Chery <nanley.g.chery@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> 
> A few minor comments below, but none of them are critical.
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> What's the current status of the underlying compression series this work is based
> on?
I did not see any patches from DK further on compression patches. From my understanding
The render compression patches got r-b's and the media compression patches need further  rework.
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++++++++++-
> >  .../drm/i915/display/intel_display_types.h    |  3 +
> >  drivers/gpu/drm/i915/display/intel_sprite.c   | 10 ++-
> >  drivers/gpu/drm/i915/i915_reg.h               | 12 ++++
> >  4 files changed, 86 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 6c4274c1564d..69e70be86f57 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1975,6 +1975,10 @@ intel_tile_width_bytes(const struct
> drm_framebuffer *fb, int color_plane)
> >  		if (color_plane == 1)
> >  			return 64;
> >  		/* fall through */
> 
> Since this one (GEN12_RC_CCS) now falls through to the new
> GEN12_RC_CCS_CC, you can probably drop these lines since they'll be handled
> the same way in the next case down.
Color_plane == 2 does not apply for GE12_RC_CCS. Do you think it is still ok to club
The two cases?

> 
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > +		if (color_plane == 1 || color_plane == 2)
> 
> I've reviewed a lot of versions of these compression series so I might be getting
> confused, but didn't DK have a version at one time that added some kind of
> is_ccs_plane(mod, plane) helper function?  I'm not sure what happened to that,
> but it did seem like a nice way to consolidate the "these planes contain CCS
> stuff" logic rather than having to hardcode the plane ID's all over the place like
> this.
The is_ccs_plane helper function was introduced in media  compression patches
Where further complexities were required while calculating plane dims.

> 
> 
> > +			return 64;
> > +		/* fall through */
> >  	case I915_FORMAT_MOD_Y_TILED:
> >  		if (IS_GEN(dev_priv, 2) || HAS_128_BYTE_Y_TILING(dev_priv))
> >  			return 128;
> > @@ -2013,6 +2017,10 @@ intel_tile_height(const struct drm_framebuffer
> *fb, int color_plane)
> >  		if (color_plane == 1)
> >  			return 1;
> >  		/* fall through */
> 
> As above, you can now consolidate this with the new block below if you want.
> 
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > +		if (color_plane == 1 || color_plane == 2)
> > +			return 1;
> > +		/* fall through */
> >  	default:
> >  		return intel_tile_size(to_i915(fb->dev)) /
> >  			intel_tile_width_bytes(fb, color_plane); @@ -2116,6
> +2124,7 @@
> > static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> >  			return 256 * 1024;
> >  		return 0;
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> >  		return 16 * 1024;
> >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> >  	case I915_FORMAT_MOD_Yf_TILED_CCS:
> > @@ -2313,8 +2322,16 @@ static u32 intel_adjust_tile_offset(int *x, int
> > *y,
> >
> >  static bool is_surface_linear(u64 modifier, int color_plane)  {
> > -	return modifier == DRM_FORMAT_MOD_LINEAR ||
> > -	       (modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS &&
> color_plane == 1);
> > +	switch (modifier) {
> > +	case DRM_FORMAT_MOD_LINEAR:
> > +		return true;
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > +		return color_plane == 1;
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > +		return color_plane == 1 || color_plane == 2;
> > +	default:
> > +		return false;
> > +	}
> >  }
> >
> >  static u32 intel_adjust_aligned_offset(int *x, int *y, @@ -2502,6
> > +2519,7 @@ static unsigned int intel_fb_modifier_to_tiling(u64 fb_modifier)
> >  	case I915_FORMAT_MOD_Y_TILED:
> >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> >  		return I915_TILING_Y;
> >  	default:
> >  		return I915_TILING_NONE;
> > @@ -2551,6 +2569,21 @@ static const struct drm_format_info
> gen12_ccs_formats[] = {
> >  	  .cpp = { 4, 1, }, .hsub = 2, .vsub = 32, .has_alpha = true },  };
> >
> > +/*
> > + * Same as gen12_ccs_formats[] above, but with additional surface
> > +used
> > + * to pass Clear Color information in plane 2 with 64 bits of data.
> > + */
> > +static const struct drm_format_info gen12_ccs_cc_formats[] = {
> > +	{ .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 3,
> > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
> > +	{ .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 3,
> > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
> > +	{ .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 3,
> > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true },
> > +	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
> > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true },
> > +};
> > +
> >  static const struct drm_format_info *  lookup_format_info(const
> > struct drm_format_info formats[],
> >  		   int num_formats, u32 format)
> > @@ -2578,6 +2611,10 @@ intel_get_format_info(const struct
> drm_mode_fb_cmd2 *cmd)
> >  		return lookup_format_info(gen12_ccs_formats,
> >  					  ARRAY_SIZE(gen12_ccs_formats),
> >  					  cmd->pixel_format);
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > +		return lookup_format_info(gen12_ccs_cc_formats,
> > +					  ARRAY_SIZE(gen12_ccs_cc_formats),
> > +					  cmd->pixel_format);
> >  	default:
> >  		return NULL;
> >  	}
> > @@ -2586,6 +2623,7 @@ intel_get_format_info(const struct
> > drm_mode_fb_cmd2 *cmd)  bool is_ccs_modifier(u64 modifier)  {
> >  	return modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
> > +	       modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC ||
> >  	       modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> >  	       modifier == I915_FORMAT_MOD_Yf_TILED_CCS;  } @@ -2798,6
> > +2836,18 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
> >  		int x, y;
> >  		int ret;
> >
> > +		/*
> > +		 * Plane 2 of Render Compression with Clear Color fb modifier
> > +		 * is consumed by the driver and not passed to DE. Skip the
> > +		 * arithmetic related to alignment and offset calculation.
> > +		 */
> > +		if (i == 2 && fb->modifier ==
> > +I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
> 
> This line should be wrapped.
Sure will include in final rev.
> 
> > +			if (IS_ALIGNED(fb->offsets[2], PAGE_SIZE))
> > +				continue;
> > +			else
> > +				return -EINVAL;
> > +		}
> > +
> >  		cpp = fb->format->cpp[i];
> >  		width = drm_framebuffer_plane_width(fb->width, fb, i);
> >  		height = drm_framebuffer_plane_height(fb->height, fb, i); @@
> > -4295,6 +4345,7 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier)
> >  	case I915_FORMAT_MOD_Y_TILED:
> >  		return PLANE_CTL_TILED_Y;
> >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> >  		return PLANE_CTL_TILED_Y |
> PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> >  		return PLANE_CTL_TILED_Y |
> > @@ -15245,6 +15296,15 @@ static int intel_plane_pin_fb(struct
> > intel_plane_state *plane_state)
> >
> >  	plane_state->vma = vma;
> >
> > +	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
> > +		u32 *ccaddr =
> kmap_atomic(i915_gem_object_get_page(intel_fb_obj(fb),
> > +								  fb-
> >offsets[2] >> PAGE_SHIFT));
> > +
> > +		plane_state->ccval = ((u64)*(ccaddr + CC_VAL_HIGHER_OFFSET)
> << 32)
> > +				     | *(ccaddr + CC_VAL_LOWER_OFFSET);
> 
> Isn't this just the same thing as
> 
>         u64 *ccaddr = kmap_atomic(...);
>         plane_state->ccval = ccaddr[2];
> 
> ?
I think it is ccval = ccaddr[5] << 32 | ccaddr[4]?
Is this preferred?

Thanks,
Radhakrishna(RK) Sripada
> 
> > +		kunmap_atomic(ccaddr);
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 83ea04149b77..e1bd18122ce6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -607,6 +607,9 @@ struct intel_plane_state {
> >  	u32 planar_slave;
> >
> >  	struct drm_intel_sprite_colorkey ckey;
> > +
> > +	/* Clear Color Value */
> > +	u64 ccval;
> >  };
> >
> >  struct intel_initial_plane_config {
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index 67a90059900f..89f7fbc87ad9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -597,6 +597,7 @@ skl_program_plane(struct intel_plane *plane,
> >  	unsigned long irqflags;
> >  	u32 keymsk, keymax;
> >  	u32 plane_ctl = plane_state->ctl;
> > +	u64 ccval = plane_state->ccval;
> >
> >  	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> >
> > @@ -639,6 +640,10 @@ skl_program_plane(struct intel_plane *plane,
> >  	if (fb->format->is_yuv && icl_is_hdr_plane(dev_priv, plane_id))
> >  		icl_program_input_csc(plane, crtc_state, plane_state);
> >
> > +	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC)
> > +		intel_uncore_write64_fw(&dev_priv->uncore,
> > +					PLANE_CC_VAL(pipe, plane_id), ccval);
> > +
> >  	skl_write_plane_wm(plane, crtc_state);
> >
> >  	I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value); @@
> > -2106,7 +2111,8 @@ static int skl_plane_check_fb(const struct
> intel_crtc_state *crtc_state,
> >  	     fb->modifier == I915_FORMAT_MOD_Yf_TILED ||
> >  	     fb->modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> >  	     fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||
> > -	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS)) {
> > +	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
> > +	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC))
> {
> >  		DRM_DEBUG_KMS("Y/Yf tiling not supported in IF-ID mode\n");
> >  		return -EINVAL;
> >  	}
> > @@ -2579,6 +2585,7 @@ static const u64
> > skl_plane_format_modifiers_ccs[] = {
> >
> >  static const u64 gen12_plane_format_modifiers_ccs[] = {
> >  	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > +	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC,
> >  	I915_FORMAT_MOD_Y_TILED,
> >  	I915_FORMAT_MOD_X_TILED,
> >  	DRM_FORMAT_MOD_LINEAR,
> > @@ -2750,6 +2757,7 @@ static bool
> gen12_plane_format_mod_supported(struct drm_plane *_plane,
> >  	case I915_FORMAT_MOD_X_TILED:
> >  	case I915_FORMAT_MOD_Y_TILED:
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> >  		break;
> >  	default:
> >  		return false;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 2e444a18ed0b..4a683db267c1
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6840,6 +6840,8 @@ enum {
> >  #define _PLANE_KEYMAX_1_A			0x701a0
> >  #define _PLANE_KEYMAX_2_A			0x702a0
> >  #define  PLANE_KEYMAX_ALPHA(a)			((a) << 24)
> > +#define _PLANE_CC_VAL_1_A			0x701b4
> > +#define _PLANE_CC_VAL_2_A			0x702b4
> >  #define _PLANE_AUX_DIST_1_A			0x701c0
> >  #define _PLANE_AUX_DIST_2_A			0x702c0
> >  #define _PLANE_AUX_OFFSET_1_A			0x701c4
> > @@ -6879,6 +6881,16 @@ enum {
> >  #define _PLANE_NV12_BUF_CFG_1_A		0x70278
> >  #define _PLANE_NV12_BUF_CFG_2_A		0x70378
> >
> > +#define _PLANE_CC_VAL_1_B			0x711b4
> > +#define _PLANE_CC_VAL_2_B			0x712b4
> > +#define _PLANE_CC_VAL_1(pipe)	_PIPE(pipe, _PLANE_CC_VAL_1_A,
> _PLANE_CC_VAL_1_B)
> > +#define _PLANE_CC_VAL_2(pipe)	_PIPE(pipe, _PLANE_CC_VAL_2_A,
> _PLANE_CC_VAL_2_B)
> > +#define PLANE_CC_VAL(pipe, plane)	\
> > +	_MMIO_PLANE(plane, _PLANE_CC_VAL_1(pipe),
> _PLANE_CC_VAL_2(pipe))
> > +
> > +#define CC_VAL_LOWER_OFFSET		4
> > +#define CC_VAL_HIGHER_OFFSET		5
> > +
> >  /* Input CSC Register Definitions */
> >  #define _PLANE_INPUT_CSC_RY_GY_1_A	0x701E0
> >  #define _PLANE_INPUT_CSC_RY_GY_2_A	0x702E0
> > --
> > 2.20.1
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
Matt Roper Nov. 26, 2019, 10 p.m. UTC | #3 
On Tue, Nov 26, 2019 at 01:52:39PM -0800, Sripada, Radhakrishna wrote:
> Hi Matt,
> 
> > -----Original Message-----
> > From: Roper, Matthew D
> > Sent: Tuesday, November 26, 2019 12:49 PM
> > To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Pandiyan, Dhinakaran
> > <dhinakaran.pandiyan@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
> > Sharma, Shashank <shashank.sharma@intel.com>; Antognolli, Rafael
> > <rafael.antognolli@intel.com>; Chery, Nanley G <nanley.g.chery@intel.com>
> > Subject: Re: [PATCH v7 7/7] drm/i915/tgl: Add Clear Color support for TGL
> > Render Decompression
> > 
> > On Mon, Nov 25, 2019 at 04:26:35PM -0800, Radhakrishna Sripada wrote:
> > > Render Decompression is supported with Y-Tiled main surface. The CCS
> > > is linear and has 4 bits of data for each main surface cache line
> > > pair, a ratio of 1:256. Additional Clear Color information is passed
> > > from the user-space through an offset in the GEM BO. Add a new
> > > modifier to identify and parse new Clear Color information and extend
> > > Gen12 render decompression functionality to the newly added modifier.
> > >
> > > v2: Fix has_alpha flag for modifiers, omit CC modifier during initial
> > >     plane config(Matt). Fix Lookup error.
> > > v3: Fix the panic while running kms_cube
> > > v4: Add alignment check and reuse the comments for
> > > ge12_ccs_formats(Matt)
> > > v5: Fix typos and wrap comments(Matt)
> > > v6: Rebase
> > > v7: Rebase, Add missing case in intel_tile_height(RK)
> > >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@intel.com>
> > > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > > Cc: Nanley G Chery <nanley.g.chery@intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > 
> > A few minor comments below, but none of them are critical.
> > 
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > What's the current status of the underlying compression series this work is based
> > on?
> I did not see any patches from DK further on compression patches. From my understanding
> The render compression patches got r-b's and the media compression patches need further  rework.
> > 
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++++++++++-
> > >  .../drm/i915/display/intel_display_types.h    |  3 +
> > >  drivers/gpu/drm/i915/display/intel_sprite.c   | 10 ++-
> > >  drivers/gpu/drm/i915/i915_reg.h               | 12 ++++
> > >  4 files changed, 86 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 6c4274c1564d..69e70be86f57 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -1975,6 +1975,10 @@ intel_tile_width_bytes(const struct
> > drm_framebuffer *fb, int color_plane)
> > >  		if (color_plane == 1)
> > >  			return 64;
> > >  		/* fall through */
> > 
> > Since this one (GEN12_RC_CCS) now falls through to the new
> > GEN12_RC_CCS_CC, you can probably drop these lines since they'll be handled
> > the same way in the next case down.
> Color_plane == 2 does not apply for GE12_RC_CCS. Do you think it is still ok to club
> The two cases?

Right.  But since it doesn't apply, you should never get called here
with color_plane == 2.  If we have a bug somewhere higher up and you did
get called with a '2' here, you'd still be following the exact same
logic (falling through to the CCS_CC handler), so keeping them separate
doesn't seem to provide any extra benefit.

That's also part of why I was asking about the is_ccs_plane helper ---
it could take care of testing the appropriate planes for a modifier and
also add a warning message if we encounter a plane that isn't supposed
to exist for the modifier.  

> > 
> > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > +		if (color_plane == 1 || color_plane == 2)
> > 
> > I've reviewed a lot of versions of these compression series so I might be getting
> > confused, but didn't DK have a version at one time that added some kind of
> > is_ccs_plane(mod, plane) helper function?  I'm not sure what happened to that,
> > but it did seem like a nice way to consolidate the "these planes contain CCS
> > stuff" logic rather than having to hardcode the plane ID's all over the place like
> > this.
> The is_ccs_plane helper function was introduced in media  compression patches
> Where further complexities were required while calculating plane dims.
> 
> > 
> > 
> > > +			return 64;
> > > +		/* fall through */
> > >  	case I915_FORMAT_MOD_Y_TILED:
> > >  		if (IS_GEN(dev_priv, 2) || HAS_128_BYTE_Y_TILING(dev_priv))
> > >  			return 128;
> > > @@ -2013,6 +2017,10 @@ intel_tile_height(const struct drm_framebuffer
> > *fb, int color_plane)
> > >  		if (color_plane == 1)
> > >  			return 1;
> > >  		/* fall through */
> > 
> > As above, you can now consolidate this with the new block below if you want.
> > 
> > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > +		if (color_plane == 1 || color_plane == 2)
> > > +			return 1;
> > > +		/* fall through */
> > >  	default:
> > >  		return intel_tile_size(to_i915(fb->dev)) /
> > >  			intel_tile_width_bytes(fb, color_plane); @@ -2116,6
> > +2124,7 @@
> > > static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> > >  			return 256 * 1024;
> > >  		return 0;
> > >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > >  		return 16 * 1024;
> > >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> > >  	case I915_FORMAT_MOD_Yf_TILED_CCS:
> > > @@ -2313,8 +2322,16 @@ static u32 intel_adjust_tile_offset(int *x, int
> > > *y,
> > >
> > >  static bool is_surface_linear(u64 modifier, int color_plane)  {
> > > -	return modifier == DRM_FORMAT_MOD_LINEAR ||
> > > -	       (modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS &&
> > color_plane == 1);
> > > +	switch (modifier) {
> > > +	case DRM_FORMAT_MOD_LINEAR:
> > > +		return true;
> > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > +		return color_plane == 1;
> > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > +		return color_plane == 1 || color_plane == 2;
> > > +	default:
> > > +		return false;
> > > +	}
> > >  }
> > >
> > >  static u32 intel_adjust_aligned_offset(int *x, int *y, @@ -2502,6
> > > +2519,7 @@ static unsigned int intel_fb_modifier_to_tiling(u64 fb_modifier)
> > >  	case I915_FORMAT_MOD_Y_TILED:
> > >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> > >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > >  		return I915_TILING_Y;
> > >  	default:
> > >  		return I915_TILING_NONE;
> > > @@ -2551,6 +2569,21 @@ static const struct drm_format_info
> > gen12_ccs_formats[] = {
> > >  	  .cpp = { 4, 1, }, .hsub = 2, .vsub = 32, .has_alpha = true },  };
> > >
> > > +/*
> > > + * Same as gen12_ccs_formats[] above, but with additional surface
> > > +used
> > > + * to pass Clear Color information in plane 2 with 64 bits of data.
> > > + */
> > > +static const struct drm_format_info gen12_ccs_cc_formats[] = {
> > > +	{ .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 3,
> > > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
> > > +	{ .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 3,
> > > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
> > > +	{ .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 3,
> > > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true },
> > > +	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
> > > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true },
> > > +};
> > > +
> > >  static const struct drm_format_info *  lookup_format_info(const
> > > struct drm_format_info formats[],
> > >  		   int num_formats, u32 format)
> > > @@ -2578,6 +2611,10 @@ intel_get_format_info(const struct
> > drm_mode_fb_cmd2 *cmd)
> > >  		return lookup_format_info(gen12_ccs_formats,
> > >  					  ARRAY_SIZE(gen12_ccs_formats),
> > >  					  cmd->pixel_format);
> > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > +		return lookup_format_info(gen12_ccs_cc_formats,
> > > +					  ARRAY_SIZE(gen12_ccs_cc_formats),
> > > +					  cmd->pixel_format);
> > >  	default:
> > >  		return NULL;
> > >  	}
> > > @@ -2586,6 +2623,7 @@ intel_get_format_info(const struct
> > > drm_mode_fb_cmd2 *cmd)  bool is_ccs_modifier(u64 modifier)  {
> > >  	return modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
> > > +	       modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC ||
> > >  	       modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> > >  	       modifier == I915_FORMAT_MOD_Yf_TILED_CCS;  } @@ -2798,6
> > > +2836,18 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
> > >  		int x, y;
> > >  		int ret;
> > >
> > > +		/*
> > > +		 * Plane 2 of Render Compression with Clear Color fb modifier
> > > +		 * is consumed by the driver and not passed to DE. Skip the
> > > +		 * arithmetic related to alignment and offset calculation.
> > > +		 */
> > > +		if (i == 2 && fb->modifier ==
> > > +I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
> > 
> > This line should be wrapped.
> Sure will include in final rev.
> > 
> > > +			if (IS_ALIGNED(fb->offsets[2], PAGE_SIZE))
> > > +				continue;
> > > +			else
> > > +				return -EINVAL;
> > > +		}
> > > +
> > >  		cpp = fb->format->cpp[i];
> > >  		width = drm_framebuffer_plane_width(fb->width, fb, i);
> > >  		height = drm_framebuffer_plane_height(fb->height, fb, i); @@
> > > -4295,6 +4345,7 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier)
> > >  	case I915_FORMAT_MOD_Y_TILED:
> > >  		return PLANE_CTL_TILED_Y;
> > >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > >  		return PLANE_CTL_TILED_Y |
> > PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
> > >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > >  		return PLANE_CTL_TILED_Y |
> > > @@ -15245,6 +15296,15 @@ static int intel_plane_pin_fb(struct
> > > intel_plane_state *plane_state)
> > >
> > >  	plane_state->vma = vma;
> > >
> > > +	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
> > > +		u32 *ccaddr =
> > kmap_atomic(i915_gem_object_get_page(intel_fb_obj(fb),
> > > +								  fb-
> > >offsets[2] >> PAGE_SHIFT));
> > > +
> > > +		plane_state->ccval = ((u64)*(ccaddr + CC_VAL_HIGHER_OFFSET)
> > << 32)
> > > +				     | *(ccaddr + CC_VAL_LOWER_OFFSET);
> > 
> > Isn't this just the same thing as
> > 
> >         u64 *ccaddr = kmap_atomic(...);
> >         plane_state->ccval = ccaddr[2];
> > 
> > ?
> I think it is ccval = ccaddr[5] << 32 | ccaddr[4]?
> Is this preferred?

Given that we're little-endian, both should be equivalent, right?  I.e.,
if you treat the pointer as u64*, then ccaddr[2] gives you the whole
value at once with no bit manipulation.  If you treat it as u32*, then
you need to grab both words and shift them into position and OR them as
you do here.

I don't think it really matters too much.  I was just suggesting
something that might be slightly shorter and easier to read.


Matt

> 
> Thanks,
> Radhakrishna(RK) Sripada
> > 
> > > +		kunmap_atomic(ccaddr);
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 83ea04149b77..e1bd18122ce6 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -607,6 +607,9 @@ struct intel_plane_state {
> > >  	u32 planar_slave;
> > >
> > >  	struct drm_intel_sprite_colorkey ckey;
> > > +
> > > +	/* Clear Color Value */
> > > +	u64 ccval;
> > >  };
> > >
> > >  struct intel_initial_plane_config {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > index 67a90059900f..89f7fbc87ad9 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > @@ -597,6 +597,7 @@ skl_program_plane(struct intel_plane *plane,
> > >  	unsigned long irqflags;
> > >  	u32 keymsk, keymax;
> > >  	u32 plane_ctl = plane_state->ctl;
> > > +	u64 ccval = plane_state->ccval;
> > >
> > >  	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> > >
> > > @@ -639,6 +640,10 @@ skl_program_plane(struct intel_plane *plane,
> > >  	if (fb->format->is_yuv && icl_is_hdr_plane(dev_priv, plane_id))
> > >  		icl_program_input_csc(plane, crtc_state, plane_state);
> > >
> > > +	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC)
> > > +		intel_uncore_write64_fw(&dev_priv->uncore,
> > > +					PLANE_CC_VAL(pipe, plane_id), ccval);
> > > +
> > >  	skl_write_plane_wm(plane, crtc_state);
> > >
> > >  	I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value); @@
> > > -2106,7 +2111,8 @@ static int skl_plane_check_fb(const struct
> > intel_crtc_state *crtc_state,
> > >  	     fb->modifier == I915_FORMAT_MOD_Yf_TILED ||
> > >  	     fb->modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> > >  	     fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||
> > > -	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS)) {
> > > +	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
> > > +	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC))
> > {
> > >  		DRM_DEBUG_KMS("Y/Yf tiling not supported in IF-ID mode\n");
> > >  		return -EINVAL;
> > >  	}
> > > @@ -2579,6 +2585,7 @@ static const u64
> > > skl_plane_format_modifiers_ccs[] = {
> > >
> > >  static const u64 gen12_plane_format_modifiers_ccs[] = {
> > >  	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > > +	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC,
> > >  	I915_FORMAT_MOD_Y_TILED,
> > >  	I915_FORMAT_MOD_X_TILED,
> > >  	DRM_FORMAT_MOD_LINEAR,
> > > @@ -2750,6 +2757,7 @@ static bool
> > gen12_plane_format_mod_supported(struct drm_plane *_plane,
> > >  	case I915_FORMAT_MOD_X_TILED:
> > >  	case I915_FORMAT_MOD_Y_TILED:
> > >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > >  		break;
> > >  	default:
> > >  		return false;
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h index 2e444a18ed0b..4a683db267c1
> > > 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6840,6 +6840,8 @@ enum {
> > >  #define _PLANE_KEYMAX_1_A			0x701a0
> > >  #define _PLANE_KEYMAX_2_A			0x702a0
> > >  #define  PLANE_KEYMAX_ALPHA(a)			((a) << 24)
> > > +#define _PLANE_CC_VAL_1_A			0x701b4
> > > +#define _PLANE_CC_VAL_2_A			0x702b4
> > >  #define _PLANE_AUX_DIST_1_A			0x701c0
> > >  #define _PLANE_AUX_DIST_2_A			0x702c0
> > >  #define _PLANE_AUX_OFFSET_1_A			0x701c4
> > > @@ -6879,6 +6881,16 @@ enum {
> > >  #define _PLANE_NV12_BUF_CFG_1_A		0x70278
> > >  #define _PLANE_NV12_BUF_CFG_2_A		0x70378
> > >
> > > +#define _PLANE_CC_VAL_1_B			0x711b4
> > > +#define _PLANE_CC_VAL_2_B			0x712b4
> > > +#define _PLANE_CC_VAL_1(pipe)	_PIPE(pipe, _PLANE_CC_VAL_1_A,
> > _PLANE_CC_VAL_1_B)
> > > +#define _PLANE_CC_VAL_2(pipe)	_PIPE(pipe, _PLANE_CC_VAL_2_A,
> > _PLANE_CC_VAL_2_B)
> > > +#define PLANE_CC_VAL(pipe, plane)	\
> > > +	_MMIO_PLANE(plane, _PLANE_CC_VAL_1(pipe),
> > _PLANE_CC_VAL_2(pipe))
> > > +
> > > +#define CC_VAL_LOWER_OFFSET		4
> > > +#define CC_VAL_HIGHER_OFFSET		5
> > > +
> > >  /* Input CSC Register Definitions */
> > >  #define _PLANE_INPUT_CSC_RY_GY_1_A	0x701E0
> > >  #define _PLANE_INPUT_CSC_RY_GY_2_A	0x702E0
> > > --
> > > 2.20.1
> > >
> > 
> > --
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
> > (916) 356-2795
Sripada, Radhakrishna Nov. 26, 2019, 10:29 p.m. UTC | #4 
> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, November 26, 2019 2:00 PM
> To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
> Sharma, Shashank <shashank.sharma@intel.com>; Antognolli, Rafael
> <rafael.antognolli@intel.com>; Chery, Nanley G <nanley.g.chery@intel.com>
> Subject: Re: [PATCH v7 7/7] drm/i915/tgl: Add Clear Color support for TGL
> Render Decompression
> 
> On Tue, Nov 26, 2019 at 01:52:39PM -0800, Sripada, Radhakrishna wrote:
> > Hi Matt,
> >
> > > -----Original Message-----
> > > From: Roper, Matthew D
> > > Sent: Tuesday, November 26, 2019 12:49 PM
> > > To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; Pandiyan, Dhinakaran
> > > <dhinakaran.pandiyan@intel.com>; Syrjala, Ville
> > > <ville.syrjala@intel.com>; Sharma, Shashank
> > > <shashank.sharma@intel.com>; Antognolli, Rafael
> > > <rafael.antognolli@intel.com>; Chery, Nanley G
> > > <nanley.g.chery@intel.com>
> > > Subject: Re: [PATCH v7 7/7] drm/i915/tgl: Add Clear Color support
> > > for TGL Render Decompression
> > >
> > > On Mon, Nov 25, 2019 at 04:26:35PM -0800, Radhakrishna Sripada wrote:
> > > > Render Decompression is supported with Y-Tiled main surface. The
> > > > CCS is linear and has 4 bits of data for each main surface cache
> > > > line pair, a ratio of 1:256. Additional Clear Color information is
> > > > passed from the user-space through an offset in the GEM BO. Add a
> > > > new modifier to identify and parse new Clear Color information and
> > > > extend
> > > > Gen12 render decompression functionality to the newly added modifier.
> > > >
> > > > v2: Fix has_alpha flag for modifiers, omit CC modifier during initial
> > > >     plane config(Matt). Fix Lookup error.
> > > > v3: Fix the panic while running kms_cube
> > > > v4: Add alignment check and reuse the comments for
> > > > ge12_ccs_formats(Matt)
> > > > v5: Fix typos and wrap comments(Matt)
> > > > v6: Rebase
> > > > v7: Rebase, Add missing case in intel_tile_height(RK)
> > > >
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Ville Syrjala <ville.syrjala@intel.com>
> > > > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > > > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > > > Cc: Nanley G Chery <nanley.g.chery@intel.com>
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Signed-off-by: Radhakrishna Sripada
> > > > <radhakrishna.sripada@intel.com>
> > >
> > > A few minor comments below, but none of them are critical.
> > >
> > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > >
> > > What's the current status of the underlying compression series this
> > > work is based on?
> > I did not see any patches from DK further on compression patches. From
> > my understanding The render compression patches got r-b's and the media
> compression patches need further  rework.
> > >
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++++++++++-
> > > >  .../drm/i915/display/intel_display_types.h    |  3 +
> > > >  drivers/gpu/drm/i915/display/intel_sprite.c   | 10 ++-
> > > >  drivers/gpu/drm/i915/i915_reg.h               | 12 ++++
> > > >  4 files changed, 86 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 6c4274c1564d..69e70be86f57 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -1975,6 +1975,10 @@ intel_tile_width_bytes(const struct
> > > drm_framebuffer *fb, int color_plane)
> > > >  		if (color_plane == 1)
> > > >  			return 64;
> > > >  		/* fall through */
> > >
> > > Since this one (GEN12_RC_CCS) now falls through to the new
> > > GEN12_RC_CCS_CC, you can probably drop these lines since they'll be
> > > handled the same way in the next case down.
> > Color_plane == 2 does not apply for GE12_RC_CCS. Do you think it is
> > still ok to club The two cases?
> 
> Right.  But since it doesn't apply, you should never get called here with
> color_plane == 2.  If we have a bug somewhere higher up and you did get called
> with a '2' here, you'd still be following the exact same logic (falling through to
> the CCS_CC handler), so keeping them separate doesn't seem to provide any
> extra benefit.
> 
> That's also part of why I was asking about the is_ccs_plane helper --- it could
> take care of testing the appropriate planes for a modifier and also add a warning
> message if we encounter a plane that isn't supposed to exist for the modifier.
> 
Sure will simplify in next rev.

> > >
> > > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > > +		if (color_plane == 1 || color_plane == 2)
> > >
> > > I've reviewed a lot of versions of these compression series so I
> > > might be getting confused, but didn't DK have a version at one time
> > > that added some kind of is_ccs_plane(mod, plane) helper function?
> > > I'm not sure what happened to that, but it did seem like a nice way
> > > to consolidate the "these planes contain CCS stuff" logic rather
> > > than having to hardcode the plane ID's all over the place like this.
> > The is_ccs_plane helper function was introduced in media  compression
> > patches Where further complexities were required while calculating plane
> dims.
> >
> > >
> > >
> > > > +			return 64;
> > > > +		/* fall through */
> > > >  	case I915_FORMAT_MOD_Y_TILED:
> > > >  		if (IS_GEN(dev_priv, 2) || HAS_128_BYTE_Y_TILING(dev_priv))
> > > >  			return 128;
> > > > @@ -2013,6 +2017,10 @@ intel_tile_height(const struct
> > > > drm_framebuffer
> > > *fb, int color_plane)
> > > >  		if (color_plane == 1)
> > > >  			return 1;
> > > >  		/* fall through */
> > >
> > > As above, you can now consolidate this with the new block below if you
> want.
> > >
> > > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > > +		if (color_plane == 1 || color_plane == 2)
> > > > +			return 1;
> > > > +		/* fall through */
> > > >  	default:
> > > >  		return intel_tile_size(to_i915(fb->dev)) /
> > > >  			intel_tile_width_bytes(fb, color_plane); @@ -2116,6
> > > +2124,7 @@
> > > > static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> > > >  			return 256 * 1024;
> > > >  		return 0;
> > > >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > >  		return 16 * 1024;
> > > >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> > > >  	case I915_FORMAT_MOD_Yf_TILED_CCS:
> > > > @@ -2313,8 +2322,16 @@ static u32 intel_adjust_tile_offset(int *x,
> > > > int *y,
> > > >
> > > >  static bool is_surface_linear(u64 modifier, int color_plane)  {
> > > > -	return modifier == DRM_FORMAT_MOD_LINEAR ||
> > > > -	       (modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS &&
> > > color_plane == 1);
> > > > +	switch (modifier) {
> > > > +	case DRM_FORMAT_MOD_LINEAR:
> > > > +		return true;
> > > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > > +		return color_plane == 1;
> > > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > > +		return color_plane == 1 || color_plane == 2;
> > > > +	default:
> > > > +		return false;
> > > > +	}
> > > >  }
> > > >
> > > >  static u32 intel_adjust_aligned_offset(int *x, int *y, @@ -2502,6
> > > > +2519,7 @@ static unsigned int intel_fb_modifier_to_tiling(u64
> > > > +fb_modifier)
> > > >  	case I915_FORMAT_MOD_Y_TILED:
> > > >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> > > >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > >  		return I915_TILING_Y;
> > > >  	default:
> > > >  		return I915_TILING_NONE;
> > > > @@ -2551,6 +2569,21 @@ static const struct drm_format_info
> > > gen12_ccs_formats[] = {
> > > >  	  .cpp = { 4, 1, }, .hsub = 2, .vsub = 32, .has_alpha = true },
> > > > };
> > > >
> > > > +/*
> > > > + * Same as gen12_ccs_formats[] above, but with additional surface
> > > > +used
> > > > + * to pass Clear Color information in plane 2 with 64 bits of data.
> > > > + */
> > > > +static const struct drm_format_info gen12_ccs_cc_formats[] = {
> > > > +	{ .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 3,
> > > > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
> > > > +	{ .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 3,
> > > > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
> > > > +	{ .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 3,
> > > > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true },
> > > > +	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
> > > > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true
> > > > +}, };
> > > > +
> > > >  static const struct drm_format_info *  lookup_format_info(const
> > > > struct drm_format_info formats[],
> > > >  		   int num_formats, u32 format) @@ -2578,6 +2611,10 @@
> > > > intel_get_format_info(const struct
> > > drm_mode_fb_cmd2 *cmd)
> > > >  		return lookup_format_info(gen12_ccs_formats,
> > > >  					  ARRAY_SIZE(gen12_ccs_formats),
> > > >  					  cmd->pixel_format);
> > > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > > +		return lookup_format_info(gen12_ccs_cc_formats,
> > > > +					  ARRAY_SIZE(gen12_ccs_cc_formats),
> > > > +					  cmd->pixel_format);
> > > >  	default:
> > > >  		return NULL;
> > > >  	}
> > > > @@ -2586,6 +2623,7 @@ intel_get_format_info(const struct
> > > > drm_mode_fb_cmd2 *cmd)  bool is_ccs_modifier(u64 modifier)  {
> > > >  	return modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
> > > > +	       modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC ||
> > > >  	       modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> > > >  	       modifier == I915_FORMAT_MOD_Yf_TILED_CCS;  } @@ -2798,6
> > > > +2836,18 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
> > > >  		int x, y;
> > > >  		int ret;
> > > >
> > > > +		/*
> > > > +		 * Plane 2 of Render Compression with Clear Color fb modifier
> > > > +		 * is consumed by the driver and not passed to DE. Skip the
> > > > +		 * arithmetic related to alignment and offset calculation.
> > > > +		 */
> > > > +		if (i == 2 && fb->modifier ==
> > > > +I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
> > >
> > > This line should be wrapped.
> > Sure will include in final rev.
> > >
> > > > +			if (IS_ALIGNED(fb->offsets[2], PAGE_SIZE))
> > > > +				continue;
> > > > +			else
> > > > +				return -EINVAL;
> > > > +		}
> > > > +
> > > >  		cpp = fb->format->cpp[i];
> > > >  		width = drm_framebuffer_plane_width(fb->width, fb, i);
> > > >  		height = drm_framebuffer_plane_height(fb->height, fb, i); @@
> > > > -4295,6 +4345,7 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier)
> > > >  	case I915_FORMAT_MOD_Y_TILED:
> > > >  		return PLANE_CTL_TILED_Y;
> > > >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> > > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > >  		return PLANE_CTL_TILED_Y |
> > > PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
> > > >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > >  		return PLANE_CTL_TILED_Y |
> > > > @@ -15245,6 +15296,15 @@ static int intel_plane_pin_fb(struct
> > > > intel_plane_state *plane_state)
> > > >
> > > >  	plane_state->vma = vma;
> > > >
> > > > +	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
> > > > +		u32 *ccaddr =
> > > kmap_atomic(i915_gem_object_get_page(intel_fb_obj(fb),
> > > > +								  fb-
> > > >offsets[2] >> PAGE_SHIFT));
> > > > +
> > > > +		plane_state->ccval = ((u64)*(ccaddr + CC_VAL_HIGHER_OFFSET)
> > > << 32)
> > > > +				     | *(ccaddr + CC_VAL_LOWER_OFFSET);
> > >
> > > Isn't this just the same thing as
> > >
> > >         u64 *ccaddr = kmap_atomic(...);
> > >         plane_state->ccval = ccaddr[2];
> > >
> > > ?
> > I think it is ccval = ccaddr[5] << 32 | ccaddr[4]?
> > Is this preferred?
> 
> Given that we're little-endian, both should be equivalent, right?  I.e., if you treat
> the pointer as u64*, then ccaddr[2] gives you the whole value at once with no
> bit manipulation.  If you treat it as u32*, then you need to grab both words and
> shift them into position and OR them as you do here.
> 
> I don't think it really matters too much.  I was just suggesting something that
> might be slightly shorter and easier to read.
Ah u64* yes. But I prefer keeping it explicit so as to clearly identify the higher and
lower offsets in case someone wants to use this further also explains the different
fields described in the documentation section of the previous patch.

RK
> 
> 
> Matt
> 
> >
> > Thanks,
> > Radhakrishna(RK) Sripada
> > >
> > > > +		kunmap_atomic(ccaddr);
> > > > +	}
> > > > +
> > > >  	return 0;
> > > >  }
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index 83ea04149b77..e1bd18122ce6 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -607,6 +607,9 @@ struct intel_plane_state {
> > > >  	u32 planar_slave;
> > > >
> > > >  	struct drm_intel_sprite_colorkey ckey;
> > > > +
> > > > +	/* Clear Color Value */
> > > > +	u64 ccval;
> > > >  };
> > > >
> > > >  struct intel_initial_plane_config { diff --git
> > > > a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > index 67a90059900f..89f7fbc87ad9 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > > @@ -597,6 +597,7 @@ skl_program_plane(struct intel_plane *plane,
> > > >  	unsigned long irqflags;
> > > >  	u32 keymsk, keymax;
> > > >  	u32 plane_ctl = plane_state->ctl;
> > > > +	u64 ccval = plane_state->ccval;
> > > >
> > > >  	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> > > >
> > > > @@ -639,6 +640,10 @@ skl_program_plane(struct intel_plane *plane,
> > > >  	if (fb->format->is_yuv && icl_is_hdr_plane(dev_priv, plane_id))
> > > >  		icl_program_input_csc(plane, crtc_state, plane_state);
> > > >
> > > > +	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC)
> > > > +		intel_uncore_write64_fw(&dev_priv->uncore,
> > > > +					PLANE_CC_VAL(pipe, plane_id), ccval);
> > > > +
> > > >  	skl_write_plane_wm(plane, crtc_state);
> > > >
> > > >  	I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value); @@
> > > > -2106,7 +2111,8 @@ static int skl_plane_check_fb(const struct
> > > intel_crtc_state *crtc_state,
> > > >  	     fb->modifier == I915_FORMAT_MOD_Yf_TILED ||
> > > >  	     fb->modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> > > >  	     fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||
> > > > -	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS)) {
> > > > +	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
> > > > +	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC))
> > > {
> > > >  		DRM_DEBUG_KMS("Y/Yf tiling not supported in IF-ID mode\n");
> > > >  		return -EINVAL;
> > > >  	}
> > > > @@ -2579,6 +2585,7 @@ static const u64
> > > > skl_plane_format_modifiers_ccs[] = {
> > > >
> > > >  static const u64 gen12_plane_format_modifiers_ccs[] = {
> > > >  	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > > > +	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC,
> > > >  	I915_FORMAT_MOD_Y_TILED,
> > > >  	I915_FORMAT_MOD_X_TILED,
> > > >  	DRM_FORMAT_MOD_LINEAR,
> > > > @@ -2750,6 +2757,7 @@ static bool
> > > gen12_plane_format_mod_supported(struct drm_plane *_plane,
> > > >  	case I915_FORMAT_MOD_X_TILED:
> > > >  	case I915_FORMAT_MOD_Y_TILED:
> > > >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > >  		break;
> > > >  	default:
> > > >  		return false;
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h index 2e444a18ed0b..4a683db267c1
> > > > 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -6840,6 +6840,8 @@ enum {
> > > >  #define _PLANE_KEYMAX_1_A			0x701a0
> > > >  #define _PLANE_KEYMAX_2_A			0x702a0
> > > >  #define  PLANE_KEYMAX_ALPHA(a)			((a) << 24)
> > > > +#define _PLANE_CC_VAL_1_A			0x701b4
> > > > +#define _PLANE_CC_VAL_2_A			0x702b4
> > > >  #define _PLANE_AUX_DIST_1_A			0x701c0
> > > >  #define _PLANE_AUX_DIST_2_A			0x702c0
> > > >  #define _PLANE_AUX_OFFSET_1_A			0x701c4
> > > > @@ -6879,6 +6881,16 @@ enum {
> > > >  #define _PLANE_NV12_BUF_CFG_1_A		0x70278
> > > >  #define _PLANE_NV12_BUF_CFG_2_A		0x70378
> > > >
> > > > +#define _PLANE_CC_VAL_1_B			0x711b4
> > > > +#define _PLANE_CC_VAL_2_B			0x712b4
> > > > +#define _PLANE_CC_VAL_1(pipe)	_PIPE(pipe, _PLANE_CC_VAL_1_A,
> > > _PLANE_CC_VAL_1_B)
> > > > +#define _PLANE_CC_VAL_2(pipe)	_PIPE(pipe, _PLANE_CC_VAL_2_A,
> > > _PLANE_CC_VAL_2_B)
> > > > +#define PLANE_CC_VAL(pipe, plane)	\
> > > > +	_MMIO_PLANE(plane, _PLANE_CC_VAL_1(pipe),
> > > _PLANE_CC_VAL_2(pipe))
> > > > +
> > > > +#define CC_VAL_LOWER_OFFSET		4
> > > > +#define CC_VAL_HIGHER_OFFSET		5
> > > > +
> > > >  /* Input CSC Register Definitions */
> > > >  #define _PLANE_INPUT_CSC_RY_GY_1_A	0x701E0
> > > >  #define _PLANE_INPUT_CSC_RY_GY_2_A	0x702E0
> > > > --
> > > > 2.20.1
> > > >
> > >
> > > --
> > > Matt Roper
> > > Graphics Software Engineer
> > > VTT-OSGC Platform Enablement
> > > Intel Corporation
> > > (916) 356-2795
> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
Saarinen, Jani Nov. 27, 2019, 6:49 a.m. UTC | #5 
+ Imre

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Matt
> Roper
> Sent: tiistai 26. marraskuuta 2019 22.49
> To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>
> Cc: Chery, Nanley G <nanley.g.chery@intel.com>; intel-
> gfx@lists.freedesktop.org; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v7 7/7] drm/i915/tgl: Add Clear Color support for
> TGL Render Decompression
> 
> On Mon, Nov 25, 2019 at 04:26:35PM -0800, Radhakrishna Sripada wrote:
> > Render Decompression is supported with Y-Tiled main surface. The CCS
> > is linear and has 4 bits of data for each main surface cache line
> > pair, a ratio of 1:256. Additional Clear Color information is passed
> > from the user-space through an offset in the GEM BO. Add a new
> > modifier to identify and parse new Clear Color information and extend
> > Gen12 render decompression functionality to the newly added modifier.
> >
> > v2: Fix has_alpha flag for modifiers, omit CC modifier during initial
> >     plane config(Matt). Fix Lookup error.
> > v3: Fix the panic while running kms_cube
> > v4: Add alignment check and reuse the comments for
> > ge12_ccs_formats(Matt)
> > v5: Fix typos and wrap comments(Matt)
> > v6: Rebase
> > v7: Rebase, Add missing case in intel_tile_height(RK)
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@intel.com>
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > Cc: Nanley G Chery <nanley.g.chery@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> 
> A few minor comments below, but none of them are critical.
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> What's the current status of the underlying compression series this work is based
> on?
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c  | 64 ++++++++++++++++++-
> >  .../drm/i915/display/intel_display_types.h    |  3 +
> >  drivers/gpu/drm/i915/display/intel_sprite.c   | 10 ++-
> >  drivers/gpu/drm/i915/i915_reg.h               | 12 ++++
> >  4 files changed, 86 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 6c4274c1564d..69e70be86f57 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1975,6 +1975,10 @@ intel_tile_width_bytes(const struct
> drm_framebuffer *fb, int color_plane)
> >  		if (color_plane == 1)
> >  			return 64;
> >  		/* fall through */
> 
> Since this one (GEN12_RC_CCS) now falls through to the new
> GEN12_RC_CCS_CC, you can probably drop these lines since they'll be handled
> the same way in the next case down.
> 
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > +		if (color_plane == 1 || color_plane == 2)
> 
> I've reviewed a lot of versions of these compression series so I might be getting
> confused, but didn't DK have a version at one time that added some kind of
> is_ccs_plane(mod, plane) helper function?  I'm not sure what happened to that,
> but it did seem like a nice way to consolidate the "these planes contain CCS stuff"
> logic rather than having to hardcode the plane ID's all over the place like this.
> 
> 
> > +			return 64;
> > +		/* fall through */
> >  	case I915_FORMAT_MOD_Y_TILED:
> >  		if (IS_GEN(dev_priv, 2) ||
> HAS_128_BYTE_Y_TILING(dev_priv))
> >  			return 128;
> > @@ -2013,6 +2017,10 @@ intel_tile_height(const struct drm_framebuffer *fb,
> int color_plane)
> >  		if (color_plane == 1)
> >  			return 1;
> >  		/* fall through */
> 
> As above, you can now consolidate this with the new block below if you want.
> 
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > +		if (color_plane == 1 || color_plane == 2)
> > +			return 1;
> > +		/* fall through */
> >  	default:
> >  		return intel_tile_size(to_i915(fb->dev)) /
> >  			intel_tile_width_bytes(fb,
> color_plane); @@ -2116,6 +2124,7 @@
> > static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> >  			return 256 * 1024;
> >  		return 0;
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> >  		return 16 * 1024;
> >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> >  	case I915_FORMAT_MOD_Yf_TILED_CCS:
> > @@ -2313,8 +2322,16 @@ static u32 intel_adjust_tile_offset(int *x, int
> > *y,
> >
> >  static bool is_surface_linear(u64 modifier, int color_plane)  {
> > -	return modifier == DRM_FORMAT_MOD_LINEAR ||
> > -	       (modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS
> && color_plane == 1);
> > +	switch (modifier) {
> > +	case DRM_FORMAT_MOD_LINEAR:
> > +		return true;
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > +		return color_plane == 1;
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > +		return color_plane == 1 || color_plane == 2;
> > +	default:
> > +		return false;
> > +	}
> >  }
> >
> >  static u32 intel_adjust_aligned_offset(int *x, int *y, @@ -2502,6
> > +2519,7 @@ static unsigned int intel_fb_modifier_to_tiling(u64 fb_modifier)
> >  	case I915_FORMAT_MOD_Y_TILED:
> >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> >  		return I915_TILING_Y;
> >  	default:
> >  		return I915_TILING_NONE;
> > @@ -2551,6 +2569,21 @@ static const struct drm_format_info
> gen12_ccs_formats[] = {
> >  	  .cpp = { 4, 1, }, .hsub = 2, .vsub = 32, .has_alpha = true },  };
> >
> > +/*
> > + * Same as gen12_ccs_formats[] above, but with additional surface
> > +used
> > + * to pass Clear Color information in plane 2 with 64 bits of data.
> > + */
> > +static const struct drm_format_info gen12_ccs_cc_formats[] = {
> > +	{ .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes =
> 3,
> > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
> > +	{ .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes =
> 3,
> > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
> > +	{ .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes =
> 3,
> > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true },
> > +	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes =
> 3,
> > +	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true },
> > +};
> > +
> >  static const struct drm_format_info *  lookup_format_info(const
> > struct drm_format_info formats[],
> >  		   int num_formats, u32 format)
> > @@ -2578,6 +2611,10 @@ intel_get_format_info(const struct
> drm_mode_fb_cmd2 *cmd)
> >  		return lookup_format_info(gen12_ccs_formats,
> >
> ARRAY_SIZE(gen12_ccs_formats),
> >  					  cmd-
> >pixel_format);
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > +		return lookup_format_info(gen12_ccs_cc_formats,
> > +
> ARRAY_SIZE(gen12_ccs_cc_formats),
> > +					  cmd-
> >pixel_format);
> >  	default:
> >  		return NULL;
> >  	}
> > @@ -2586,6 +2623,7 @@ intel_get_format_info(const struct
> > drm_mode_fb_cmd2 *cmd)  bool is_ccs_modifier(u64 modifier)  {
> >  	return modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS
> ||
> > +	       modifier ==
> I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC ||
> >  	       modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> >  	       modifier == I915_FORMAT_MOD_Yf_TILED_CCS;  } @@ -
> 2798,6
> > +2836,18 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv,
> >  		int x, y;
> >  		int ret;
> >
> > +		/*
> > +		 * Plane 2 of Render Compression with Clear Color fb
> modifier
> > +		 * is consumed by the driver and not passed to DE.
> Skip the
> > +		 * arithmetic related to alignment and offset
> calculation.
> > +		 */
> > +		if (i == 2 && fb->modifier ==
> > +I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
> 
> This line should be wrapped.
> 
> > +			if (IS_ALIGNED(fb->offsets[2],
> PAGE_SIZE))
> > +				continue;
> > +			else
> > +				return -EINVAL;
> > +		}
> > +
> >  		cpp = fb->format->cpp[i];
> >  		width = drm_framebuffer_plane_width(fb->width,
> fb, i);
> >  		height = drm_framebuffer_plane_height(fb->height,
> fb, i); @@
> > -4295,6 +4345,7 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier)
> >  	case I915_FORMAT_MOD_Y_TILED:
> >  		return PLANE_CTL_TILED_Y;
> >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> >  		return PLANE_CTL_TILED_Y |
> PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> >  		return PLANE_CTL_TILED_Y |
> > @@ -15245,6 +15296,15 @@ static int intel_plane_pin_fb(struct
> > intel_plane_state *plane_state)
> >
> >  	plane_state->vma = vma;
> >
> > +	if (fb->modifier ==
> I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
> > +		u32 *ccaddr =
> kmap_atomic(i915_gem_object_get_page(intel_fb_obj(fb),
> > +
> 			  fb->offsets[2] >> PAGE_SHIFT));
> > +
> > +		plane_state->ccval = ((u64)*(ccaddr +
> CC_VAL_HIGHER_OFFSET) << 32)
> > +				     | *(ccaddr +
> CC_VAL_LOWER_OFFSET);
> 
> Isn't this just the same thing as
> 
>         u64 *ccaddr = kmap_atomic(...);
>         plane_state->ccval = ccaddr[2];
> 
> ?
> 
> > +		kunmap_atomic(ccaddr);
> > +	}
> > +
> >  	return 0;
> >  }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 83ea04149b77..e1bd18122ce6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -607,6 +607,9 @@ struct intel_plane_state {
> >  	u32 planar_slave;
> >
> >  	struct drm_intel_sprite_colorkey ckey;
> > +
> > +	/* Clear Color Value */
> > +	u64 ccval;
> >  };
> >
> >  struct intel_initial_plane_config {
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index 67a90059900f..89f7fbc87ad9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -597,6 +597,7 @@ skl_program_plane(struct intel_plane *plane,
> >  	unsigned long irqflags;
> >  	u32 keymsk, keymax;
> >  	u32 plane_ctl = plane_state->ctl;
> > +	u64 ccval = plane_state->ccval;
> >
> >  	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> >
> > @@ -639,6 +640,10 @@ skl_program_plane(struct intel_plane *plane,
> >  	if (fb->format->is_yuv && icl_is_hdr_plane(dev_priv, plane_id))
> >  		icl_program_input_csc(plane, crtc_state,
> plane_state);
> >
> > +	if (fb->modifier ==
> I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC)
> > +		intel_uncore_write64_fw(&dev_priv->uncore,
> > +
> 	PLANE_CC_VAL(pipe, plane_id), ccval);
> > +
> >  	skl_write_plane_wm(plane, crtc_state);
> >
> >  	I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
> @@
> > -2106,7 +2111,8 @@ static int skl_plane_check_fb(const struct intel_crtc_state
> *crtc_state,
> >  	     fb->modifier == I915_FORMAT_MOD_Yf_TILED ||
> >  	     fb->modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> >  	     fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||
> > -	     fb->modifier ==
> I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS)) {
> > +	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS
> ||
> > +	     fb->modifier ==
> I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC)) {
> >  		DRM_DEBUG_KMS("Y/Yf tiling not supported in IF-ID
> mode\n");
> >  		return -EINVAL;
> >  	}
> > @@ -2579,6 +2585,7 @@ static const u64
> > skl_plane_format_modifiers_ccs[] = {
> >
> >  static const u64 gen12_plane_format_modifiers_ccs[] = {
> >  	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > +	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC,
> >  	I915_FORMAT_MOD_Y_TILED,
> >  	I915_FORMAT_MOD_X_TILED,
> >  	DRM_FORMAT_MOD_LINEAR,
> > @@ -2750,6 +2757,7 @@ static bool
> gen12_plane_format_mod_supported(struct drm_plane *_plane,
> >  	case I915_FORMAT_MOD_X_TILED:
> >  	case I915_FORMAT_MOD_Y_TILED:
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> >  		break;
> >  	default:
> >  		return false;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 2e444a18ed0b..4a683db267c1
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6840,6 +6840,8 @@ enum {
> >  #define _PLANE_KEYMAX_1_A			0x701a0
> >  #define _PLANE_KEYMAX_2_A			0x702a0
> >  #define  PLANE_KEYMAX_ALPHA(a)			((a) << 24)
> > +#define _PLANE_CC_VAL_1_A			0x701b4
> > +#define _PLANE_CC_VAL_2_A			0x702b4
> >  #define _PLANE_AUX_DIST_1_A			0x701c0
> >  #define _PLANE_AUX_DIST_2_A			0x702c0
> >  #define _PLANE_AUX_OFFSET_1_A			0x701c4
> > @@ -6879,6 +6881,16 @@ enum {
> >  #define _PLANE_NV12_BUF_CFG_1_A		0x70278
> >  #define _PLANE_NV12_BUF_CFG_2_A		0x70378
> >
> > +#define _PLANE_CC_VAL_1_B			0x711b4
> > +#define _PLANE_CC_VAL_2_B			0x712b4
> > +#define _PLANE_CC_VAL_1(pipe)	_PIPE(pipe, _PLANE_CC_VAL_1_A,
> _PLANE_CC_VAL_1_B)
> > +#define _PLANE_CC_VAL_2(pipe)	_PIPE(pipe, _PLANE_CC_VAL_2_A,
> _PLANE_CC_VAL_2_B)
> > +#define PLANE_CC_VAL(pipe, plane)	\
> > +	_MMIO_PLANE(plane, _PLANE_CC_VAL_1(pipe),
> _PLANE_CC_VAL_2(pipe))
> > +
> > +#define CC_VAL_LOWER_OFFSET		4
> > +#define CC_VAL_HIGHER_OFFSET		5
> > +
> >  /* Input CSC Register Definitions */
> >  #define _PLANE_INPUT_CSC_RY_GY_1_A	0x701E0
> >  #define _PLANE_INPUT_CSC_RY_GY_2_A	0x702E0
> > --
> > 2.20.1
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 6c4274c1564d..69e70be86f57 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1975,6 +1975,10 @@  intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane)
 		if (color_plane == 1)
 			return 64;
 		/* fall through */
+	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
+		if (color_plane == 1 || color_plane == 2)
+			return 64;
+		/* fall through */
 	case I915_FORMAT_MOD_Y_TILED:
 		if (IS_GEN(dev_priv, 2) || HAS_128_BYTE_Y_TILING(dev_priv))
 			return 128;
@@ -2013,6 +2017,10 @@  intel_tile_height(const struct drm_framebuffer *fb, int color_plane)
 		if (color_plane == 1)
 			return 1;
 		/* fall through */
+	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
+		if (color_plane == 1 || color_plane == 2)
+			return 1;
+		/* fall through */
 	default:
 		return intel_tile_size(to_i915(fb->dev)) /
 			intel_tile_width_bytes(fb, color_plane);
@@ -2116,6 +2124,7 @@  static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
 			return 256 * 1024;
 		return 0;
 	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
+	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
 		return 16 * 1024;
 	case I915_FORMAT_MOD_Y_TILED_CCS:
 	case I915_FORMAT_MOD_Yf_TILED_CCS:
@@ -2313,8 +2322,16 @@  static u32 intel_adjust_tile_offset(int *x, int *y,
 
 static bool is_surface_linear(u64 modifier, int color_plane)
 {
-	return modifier == DRM_FORMAT_MOD_LINEAR ||
-	       (modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS && color_plane == 1);
+	switch (modifier) {
+	case DRM_FORMAT_MOD_LINEAR:
+		return true;
+	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
+		return color_plane == 1;
+	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
+		return color_plane == 1 || color_plane == 2;
+	default:
+		return false;
+	}
 }
 
 static u32 intel_adjust_aligned_offset(int *x, int *y,
@@ -2502,6 +2519,7 @@  static unsigned int intel_fb_modifier_to_tiling(u64 fb_modifier)
 	case I915_FORMAT_MOD_Y_TILED:
 	case I915_FORMAT_MOD_Y_TILED_CCS:
 	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
+	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
 		return I915_TILING_Y;
 	default:
 		return I915_TILING_NONE;
@@ -2551,6 +2569,21 @@  static const struct drm_format_info gen12_ccs_formats[] = {
 	  .cpp = { 4, 1, }, .hsub = 2, .vsub = 32, .has_alpha = true },
 };
 
+/*
+ * Same as gen12_ccs_formats[] above, but with additional surface used
+ * to pass Clear Color information in plane 2 with 64 bits of data.
+ */
+static const struct drm_format_info gen12_ccs_cc_formats[] = {
+	{ .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 3,
+	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
+	{ .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 3,
+	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, },
+	{ .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 3,
+	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true },
+	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
+	  .cpp = { 4, 1, 0, }, .hsub = 2, .vsub = 32, .has_alpha = true },
+};
+
 static const struct drm_format_info *
 lookup_format_info(const struct drm_format_info formats[],
 		   int num_formats, u32 format)
@@ -2578,6 +2611,10 @@  intel_get_format_info(const struct drm_mode_fb_cmd2 *cmd)
 		return lookup_format_info(gen12_ccs_formats,
 					  ARRAY_SIZE(gen12_ccs_formats),
 					  cmd->pixel_format);
+	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
+		return lookup_format_info(gen12_ccs_cc_formats,
+					  ARRAY_SIZE(gen12_ccs_cc_formats),
+					  cmd->pixel_format);
 	default:
 		return NULL;
 	}
@@ -2586,6 +2623,7 @@  intel_get_format_info(const struct drm_mode_fb_cmd2 *cmd)
 bool is_ccs_modifier(u64 modifier)
 {
 	return modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
+	       modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC ||
 	       modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
 	       modifier == I915_FORMAT_MOD_Yf_TILED_CCS;
 }
@@ -2798,6 +2836,18 @@  intel_fill_fb_info(struct drm_i915_private *dev_priv,
 		int x, y;
 		int ret;
 
+		/*
+		 * Plane 2 of Render Compression with Clear Color fb modifier
+		 * is consumed by the driver and not passed to DE. Skip the
+		 * arithmetic related to alignment and offset calculation.
+		 */
+		if (i == 2 && fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
+			if (IS_ALIGNED(fb->offsets[2], PAGE_SIZE))
+				continue;
+			else
+				return -EINVAL;
+		}
+
 		cpp = fb->format->cpp[i];
 		width = drm_framebuffer_plane_width(fb->width, fb, i);
 		height = drm_framebuffer_plane_height(fb->height, fb, i);
@@ -4295,6 +4345,7 @@  static u32 skl_plane_ctl_tiling(u64 fb_modifier)
 	case I915_FORMAT_MOD_Y_TILED:
 		return PLANE_CTL_TILED_Y;
 	case I915_FORMAT_MOD_Y_TILED_CCS:
+	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
 		return PLANE_CTL_TILED_Y | PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
 	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
 		return PLANE_CTL_TILED_Y |
@@ -15245,6 +15296,15 @@  static int intel_plane_pin_fb(struct intel_plane_state *plane_state)
 
 	plane_state->vma = vma;
 
+	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC) {
+		u32 *ccaddr = kmap_atomic(i915_gem_object_get_page(intel_fb_obj(fb),
+								  fb->offsets[2] >> PAGE_SHIFT));
+
+		plane_state->ccval = ((u64)*(ccaddr + CC_VAL_HIGHER_OFFSET) << 32)
+				     | *(ccaddr + CC_VAL_LOWER_OFFSET);
+		kunmap_atomic(ccaddr);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 83ea04149b77..e1bd18122ce6 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -607,6 +607,9 @@  struct intel_plane_state {
 	u32 planar_slave;
 
 	struct drm_intel_sprite_colorkey ckey;
+
+	/* Clear Color Value */
+	u64 ccval;
 };
 
 struct intel_initial_plane_config {
diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
index 67a90059900f..89f7fbc87ad9 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite.c
@@ -597,6 +597,7 @@  skl_program_plane(struct intel_plane *plane,
 	unsigned long irqflags;
 	u32 keymsk, keymax;
 	u32 plane_ctl = plane_state->ctl;
+	u64 ccval = plane_state->ccval;
 
 	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
 
@@ -639,6 +640,10 @@  skl_program_plane(struct intel_plane *plane,
 	if (fb->format->is_yuv && icl_is_hdr_plane(dev_priv, plane_id))
 		icl_program_input_csc(plane, crtc_state, plane_state);
 
+	if (fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC)
+		intel_uncore_write64_fw(&dev_priv->uncore,
+					PLANE_CC_VAL(pipe, plane_id), ccval);
+
 	skl_write_plane_wm(plane, crtc_state);
 
 	I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
@@ -2106,7 +2111,8 @@  static int skl_plane_check_fb(const struct intel_crtc_state *crtc_state,
 	     fb->modifier == I915_FORMAT_MOD_Yf_TILED ||
 	     fb->modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
 	     fb->modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||
-	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS)) {
+	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
+	     fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC)) {
 		DRM_DEBUG_KMS("Y/Yf tiling not supported in IF-ID mode\n");
 		return -EINVAL;
 	}
@@ -2579,6 +2585,7 @@  static const u64 skl_plane_format_modifiers_ccs[] = {
 
 static const u64 gen12_plane_format_modifiers_ccs[] = {
 	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
+	I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC,
 	I915_FORMAT_MOD_Y_TILED,
 	I915_FORMAT_MOD_X_TILED,
 	DRM_FORMAT_MOD_LINEAR,
@@ -2750,6 +2757,7 @@  static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
 	case I915_FORMAT_MOD_X_TILED:
 	case I915_FORMAT_MOD_Y_TILED:
 	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
+	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
 		break;
 	default:
 		return false;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2e444a18ed0b..4a683db267c1 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6840,6 +6840,8 @@  enum {
 #define _PLANE_KEYMAX_1_A			0x701a0
 #define _PLANE_KEYMAX_2_A			0x702a0
 #define  PLANE_KEYMAX_ALPHA(a)			((a) << 24)
+#define _PLANE_CC_VAL_1_A			0x701b4
+#define _PLANE_CC_VAL_2_A			0x702b4
 #define _PLANE_AUX_DIST_1_A			0x701c0
 #define _PLANE_AUX_DIST_2_A			0x702c0
 #define _PLANE_AUX_OFFSET_1_A			0x701c4
@@ -6879,6 +6881,16 @@  enum {
 #define _PLANE_NV12_BUF_CFG_1_A		0x70278
 #define _PLANE_NV12_BUF_CFG_2_A		0x70378
 
+#define _PLANE_CC_VAL_1_B			0x711b4
+#define _PLANE_CC_VAL_2_B			0x712b4
+#define _PLANE_CC_VAL_1(pipe)	_PIPE(pipe, _PLANE_CC_VAL_1_A, _PLANE_CC_VAL_1_B)
+#define _PLANE_CC_VAL_2(pipe)	_PIPE(pipe, _PLANE_CC_VAL_2_A, _PLANE_CC_VAL_2_B)
+#define PLANE_CC_VAL(pipe, plane)	\
+	_MMIO_PLANE(plane, _PLANE_CC_VAL_1(pipe), _PLANE_CC_VAL_2(pipe))
+
+#define CC_VAL_LOWER_OFFSET		4
+#define CC_VAL_HIGHER_OFFSET		5
+
 /* Input CSC Register Definitions */
 #define _PLANE_INPUT_CSC_RY_GY_1_A	0x701E0
 #define _PLANE_INPUT_CSC_RY_GY_2_A	0x702E0