From: "Sripada, Radhakrishna" <radhakrishna.sripada@intel.com>
To: "Roper, Matthew D" <matthew.d.roper@intel.com>
Cc: "Chery, Nanley G" <nanley.g.chery@intel.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>,
"Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>,
"Syrjala, Ville" <ville.syrjala@intel.com>
Subject: Re: [PATCH 6/6] drm/i915/tgl: Add Clear Color supoort for TGL Render Decompression
Date: Thu, 19 Sep 2019 22:16:34 +0000 [thread overview]
Message-ID: <8C2593290C2B3E488D763E819AF1F02E15F4E85D@ORSMSX101.amr.corp.intel.com> (raw)
In-Reply-To: <20190917215245.GB28384@mdroper-desk1.amr.corp.intel.com>
Hi Matt,
> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, September 17, 2019 2:53 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 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@intel.com>
> > Cc: Ville Syrjala <ville.syrjala@intel.com>
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Nanley G Chery <nanley.g.chery@intel.com>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@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.
>
> > + 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-09-19 22:16 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-17 12:11 [PATCH 0/6] Clear Color Support for TGL Render Decompression Radhakrishna Sripada
2019-09-17 12:11 ` [PATCH 1/6] drm/framebuffer/tgl: Format modifier for Intel Gen-12 render compression Radhakrishna Sripada
2019-09-17 12:11 ` [PATCH 2/6] drm/i915/tgl: Gen-12 render decompression Radhakrishna Sripada
2019-09-17 12:11 ` [PATCH 3/6] drm/framebuffer/tgl: Format modifier for Intel Gen-12 media compression Radhakrishna Sripada
2019-09-17 12:11 ` [PATCH 4/6] drm/i915/tgl: " Radhakrishna Sripada
2019-09-17 12:11 ` [PATCH 5/6] drm/framebuffer/tgl: Format modifier for Intel Gen 12 render compression with Clear Color Radhakrishna Sripada
2019-09-17 12:11 ` [PATCH 6/6] drm/i915/tgl: Add Clear Color supoort for TGL Render Decompression Radhakrishna Sripada
2019-09-17 21:52 ` Matt Roper
2019-09-19 22:16 ` Sripada, Radhakrishna [this message]
2019-09-19 22:38 ` Matt Roper
2019-09-19 0:53 ` Jordan Justen
2019-09-17 15:05 ` ✗ Fi.CI.CHECKPATCH: warning for Clear Color Support " Patchwork
2019-09-17 15:27 ` ✓ Fi.CI.BAT: success " Patchwork
2019-09-18 2:09 ` ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8C2593290C2B3E488D763E819AF1F02E15F4E85D@ORSMSX101.amr.corp.intel.com \
--to=radhakrishna.sripada@intel.com \
--cc=dhinakaran.pandiyan@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=matthew.d.roper@intel.com \
--cc=nanley.g.chery@intel.com \
--cc=ville.syrjala@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.