[Intel-gfx] [PATCH v7 7/7] drm/i915/tgl: Add Clear Color support for TGL Render Decompression
Sripada, Radhakrishna
radhakrishna.sripada at intel.com
Tue Nov 26 22:29:16 UTC 2019
> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, November 26, 2019 2:00 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 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 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 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 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: Nanley G Chery <nanley.g.chery at intel.com>
> > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > Signed-off-by: Radhakrishna Sripada
> > > > <radhakrishna.sripada at intel.com>
> > >
> > > A few minor comments below, but none of them are critical.
> > >
> > > Reviewed-by: Matt Roper <matthew.d.roper at 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
More information about the Intel-gfx
mailing list