[Intel-gfx] [PATCH 6/6] drm/i915/tgl: Add Clear Color supoort for TGL Render Decompression
Matt Roper
matthew.d.roper at intel.com
Thu Sep 19 22:38:00 UTC 2019
On Thu, Sep 19, 2019 at 03:16:34PM -0700, Sripada, Radhakrishna wrote:
> Hi Matt,
> > -----Original Message-----
> > From: Roper, Matthew D
> > Sent: Tuesday, September 17, 2019 2:53 PM
> > To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>
> > Cc: intel-gfx at lists.freedesktop.org; Pandiyan, Dhinakaran
> > <dhinakaran.pandiyan at intel.com>; Syrjala, Ville <ville.syrjala at intel.com>;
> > Sharma, Shashank <shashank.sharma at intel.com>; Antognolli, Rafael
> > <rafael.antognolli at intel.com>; Chery, Nanley G <nanley.g.chery at intel.com>
> > Subject: Re: [PATCH 6/6] drm/i915/tgl: Add Clear Color supoort for TGL
> > Render Decompression
> >
> > On Tue, Sep 17, 2019 at 05:11:55AM -0700, 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.
> > >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > Cc: Ville Syrjala <ville.syrjala at intel.com>
> > > Cc: Shashank Sharma <shashank.sharma at intel.com>
> > > Cc: Rafael Antognolli <rafael.antognolli at intel.com>
> > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > Cc: Nanley G Chery <nanley.g.chery at intel.com>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 47 +++++++++++++++++--
> > > .../drm/i915/display/intel_display_types.h | 3 ++
> > > drivers/gpu/drm/i915/display/intel_sprite.c | 10 +++-
> > > drivers/gpu/drm/i915/i915_reg.h | 13 +++++
> > > 4 files changed, 69 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 2da721a6abab..725b9724da49 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -1913,6 +1913,7 @@ intel_tile_width_bytes(const struct
> > drm_framebuffer *fb, int color_plane)
> > > /* fall through */
> > > case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> > > + case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > if (color_plane == 1)
> > > return cpp;
> > > /* fall through */
> > > @@ -2051,6 +2052,7 @@ static unsigned int intel_surf_alignment(const
> > struct drm_framebuffer *fb,
> > > return 0;
> > > case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> > > + case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > return 4 * 4 * 1024;
> > > case I915_FORMAT_MOD_Y_TILED_CCS:
> > > case I915_FORMAT_MOD_Yf_TILED_CCS:
> > > @@ -2256,6 +2258,7 @@ static bool is_surface_linear(u64 modifier, int
> > color_plane)
> > > return true;
> > > case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> > > + case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > return color_plane == 1;
> > > default:
> > > return false;
> > > @@ -2448,6 +2451,7 @@ static unsigned int
> > intel_fb_modifier_to_tiling(u64 fb_modifier)
> > > case I915_FORMAT_MOD_Y_TILED_CCS:
> > > case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> > > + case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > > return I915_TILING_Y;
> > > default:
> > > return I915_TILING_NONE;
> > > @@ -2497,6 +2501,21 @@ static const struct drm_format_info
> > gen12_ccs_formats[] = {
> > > .cpp = { 4, 1, }, .hsub = 2, .vsub = 32, .has_alpha = true }, };
> > >
> > > +/*
> > > + * Gen-12 compression uses 4 bits of CCS data for each cache line
> > > +pair in the
> > > + * main surface. And each 64B CCS cache line represents an area of
> > > +4x1 Y-tiles
> > > + * in the main surface. With 4 byte pixels and each Y-tile having
> > > +dimensions of
> > > + * 32x32 pixels, the ratio turns out to 1B in the CCS for every 2 x
> > > +32 pixels in
> > > + * the main surface. Additional surface is used to pass the Clear
> > > +Color
> > > + * structure for the driver to program the DE.
> > > + */
> > > +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, },
> > > + { .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 3,
> > .cpp
> > > += { 4, 1, 0}, .hsub = 2, .vsub = 32, },
> >
> > I think these last two were supposed to have .has_alpha = true?
> Sure will include in the next rev.
>
> >
> > > +};
> > > +
> > > static const struct drm_format_info * lookup_format_info(const
> > > struct drm_format_info formats[],
> > > int num_formats, u32 format)
> > > @@ -2525,6 +2544,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_formats,
> > > +
> > ARRAY_SIZE(gen12_ccs_cc_formats),
> > > + cmd->pixel_format);
> > > default:
> > > return NULL;
> > > }
> > > @@ -2534,6 +2557,7 @@ bool is_ccs_modifier(u64 modifier) {
> > > return modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
> > > modifier == I915_FORMAT_MOD_Y_TILED_GEN12_MC_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; } @@ -4120,6
> > > +4144,9 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier)
> > > case I915_FORMAT_MOD_Y_TILED_CCS:
> > > /* fall through */
> > > case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > + return PLANE_CTL_TILED_Y |
> > PLANE_CTL_RENDER_DECOMPRESSION_ENABLE |
> > > + ICL_PLANE_CTL_CLEAR_COLOR_DISABLE;
> >
> > I think this needs to be squashed back into the previous patch that added
> > general gen12 CCS.
> Sure. I will work with DK to get this changes.
> >
> >
> > > + 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_MC_CCS:
> > > return PLANE_CTL_TILED_Y |
> > PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE;
> > > @@ -9897,9 +9924,13 @@ skylake_get_initial_plane_config(struct
> > intel_crtc *crtc,
> > > case PLANE_CTL_TILED_Y:
> > > plane_config->tiling = I915_TILING_Y;
> > > if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
> > > - fb->modifier = INTEL_GEN(dev_priv) >= 12 ?
> > > -
> > I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS :
> > > - I915_FORMAT_MOD_Y_TILED_CCS;
> > > + if (INTEL_GEN(dev_priv) >= 12)
> > > + fb->modifier = val &
> > > +
> > ICL_PLANE_CTL_CLEAR_COLOR_DISABLE ?
> > > +
> > I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS :
> > > +
> > I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC;
> > > + else
> > > + fb->modifier =
> > I915_FORMAT_MOD_Y_TILED_CCS;
> > > else if (val &
> > PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
> > > fb->modifier =
> > I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS;
> > > else
> > > @@ -14322,6 +14353,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));
> > > +
> >
> > Will this work on framebuffers in stolen memory? Not that it's likely a regular
> > BIOS would actually setup compressed framebuffers to begin with...
> It is unlikely that BIOS sets up a compressed fb. However FBC could impact as it works with
> Stolen memory. I am not sure about the interaction between fbc and render compression
> working at the same time. Apart from that I am unable to think of an issue. Will try to figure
> out during testing.
My main worry here was that if the BIOS or pre-OS bootloader setup a
framebuffer in stolen memory (which is probably unlikely, but
theoretically possible in some highly specialized embedded
environments), we wouldn't be able to kmap the buffer since stolen
memory isn't accessible by the CPU (i.e., the CPU can only access it
indirectly through the aperture). So if we tried to flip back to a
RC_CCS_CC fb located in stolen memory, the i915_gem_object_get_page()
above would GEM_BUG_ON() because i915_gem_object_stolen_ops.flags
doesn't have I915_GEM_OBJECT_HAS_STRUCT_PAGE.
We don't currently allow direct allocation of framebuffers in stolen
memory, so the only place those can come from is the initial config we
inherit from the BIOS. Probably the simplest way forward is to just
bail out of skylake_get_initial_plane_config() and give up on inheriting
the fb in the very unlikely case we notice that the BIOS is claiming
that render compression + clear color is enabled.
Matt
>
> >
> > > + plane_state->ccval = ((u64)*(ccaddr +
> > CC_VAL_HIGHER_OFFSET) << 32)
> > > + | *(ccaddr + CC_VAL_LOWER_OFFSET);
> >
> > Is there more data in the clear color page besides the color itself? If so,
> > maybe we should just use a struct to make the memory layout explicit?
> According to BSpec: 43854 the first 3 offsets are for the raw color which acts as an input to
> 3d engine which fills the info in the later 2 offsets in DE compatible format. The other data
> Therefore is redundant and cannot be consumed by the DE hence ignoring.
> >
> > > + kunmap_atomic(ccaddr);
> > > + }
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -15709,6 +15749,7 @@ static int intel_framebuffer_init(struct
> > intel_framebuffer *intel_fb,
> > > * tile widths.
> > > */
> > > if ((fb->modifier ==
> > I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
> > > + fb->modifier ==
> > I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC ||
> > > fb->modifier ==
> > I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS) &&
> > > i == 0)
> > > stride_alignment *= 4;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index d5cc4b810d9e..a82695a3e0b8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -579,6 +579,9 @@ struct intel_plane_state {
> > > u32 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 1655984955ca..3672e8b9c0bc 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > > @@ -549,6 +549,7 @@ skl_program_plane(struct intel_plane *plane,
> > > u32 plane_color_ctl = 0;
> > > unsigned long irqflags;
> > > u32 keymsk, keymax;
> > > + u64 ccval = plane_state->ccval;
> > >
> > > plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> > >
> > > @@ -609,6 +610,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);
> > @@
> > > -1738,7 +1743,8 @@ static int skl_plane_check_fb(const struct
> > intel_crtc_state *crtc_state,
> > > 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_MC_CCS))
> > {
> > > + fb->modifier == I915_FORMAT_MOD_Y_TILED_GEN12_MC_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;
> > > }
> > > @@ -2153,6 +2159,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_MC_CCS,
> > > + I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC,
> > > I915_FORMAT_MOD_Y_TILED,
> > > I915_FORMAT_MOD_X_TILED,
> > > DRM_FORMAT_MOD_LINEAR,
> > > @@ -2321,6 +2328,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 096be95ec7f9..00c59ed8b114
> > > 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6681,6 +6681,7 @@ enum {
> > > #define PLANE_CTL_YUV422_VYUY (3 << 16)
> > > #define PLANE_CTL_RENDER_DECOMPRESSION_ENABLE (1 << 15)
> > > #define PLANE_CTL_TRICKLE_FEED_DISABLE (1 << 14)
> > > +#define ICL_PLANE_CTL_CLEAR_COLOR_DISABLE (1 << 13)
> >
> > Looks like the whitespace is off here.
> Will fix it next rev.
>
> Thanks,
> RK
> >
> >
> > Matt
> >
> > > #define PLANE_CTL_PLANE_GAMMA_DISABLE (1 << 13) /*
> > Pre-GLK */
> > > #define PLANE_CTL_TILED_MASK (0x7 << 10)
> > > #define PLANE_CTL_TILED_LINEAR (0 << 10)
> > > @@ -6721,6 +6722,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
> > > @@ -6760,6 +6763,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
More information about the Intel-gfx
mailing list