Hi Matt,
> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, October 22, 2019 11:15 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@xxxxxxxxx>; Syrjala, Ville <ville.syrjala@xxxxxxxxx>;
> Sharma, Shashank <shashank.sharma@xxxxxxxxx>; Antognolli, Rafael
> <rafael.antognolli@xxxxxxxxx>; Chery, Nanley G <nanley.g.chery@xxxxxxxxx>
> Subject: Re: [PATCH v4 10/10] drm/i915/tgl: Add Clear Color supoort for TGL
> Render Decompression
>
> On Mon, Oct 14, 2019 at 05:05:33PM -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.
> >
> > 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)
> >
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxx>
> > Cc: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > Cc: Rafael Antognolli <rafael.antognolli@xxxxxxxxx>
> > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > Cc: Nanley G Chery <nanley.g.chery@xxxxxxxxx>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
>
> One minor typo and one documentation suggestion, but aside from fixing
> those this patch looks good to me:
>
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
Thanks for the review.
> The previous patch that adds the modifier looks fine to me too, but I figure
> we should leave it to whoever is working on the corresponding userspace
> code to ack/rb that one.
>
>
> Matt
>
>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 52 +++++++++++++++++++
> > .../drm/i915/display/intel_display_types.h | 3 ++
> > drivers/gpu/drm/i915/display/intel_sprite.c | 11 +++-
> > drivers/gpu/drm/i915/i915_reg.h | 12 +++++
> > 4 files changed, 77 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 448168a4d780..8835920cb005 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1917,6 +1917,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;
> > @@ -2058,6 +2062,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:
> > @@ -2260,6 +2265,8 @@ static bool is_surface_linear(u64 modifier, int
> color_plane)
> > 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;
> > case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> > return color_plane == 1 || color_plane == 3;
> > default:
> > @@ -2453,6 +2460,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;
> > @@ -2506,6 +2514,21 @@ static const struct drm_format_info
> gen12_ccs_formats[] = {
> > .cpp = { 1, 1, 2, 1}, .hsub = 2, .vsub = 2, .is_yuv = true }, };
> >
> > +/*
> > + * Same as gen12_ccs_formats[] above, but with additional surface
> > +used
> > + * top pass Clear Color information in plane 2 with 64 bits of data.
>
> s/top/to
Oops will fix it.
>
> > + */
> > +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)
> > @@ -2533,6 +2556,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;
> > }
> > @@ -2542,6 +2569,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; } @@ -2729,6
> > +2757,8 @@ static bool is_ccs_plane(u64 modifier, int color_plane)
> > return false;
> > else if (modifier == I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS)
> > return color_plane == 3 || color_plane == 1;
> > + else if (modifier ==
> I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC)
> > + return color_plane == 1 || color_plane == 2;
> > else
> > return color_plane == 1;
> > }
> > @@ -2797,6 +2827,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
>
> These comment lines are >80 characters and should be re-wrapped.
>
> > + * 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];
> > intel_fb_plane_dims(&width, &height, fb, i);
> >
> > @@ -4263,6 +4305,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 |
> > @@ -14586,6 +14629,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);
>
> I think it would be good to document what the expected format is, either
> through comments and/or through a structure definition that describes the
> exact layout. As you mentioned before, some of the bytes provided by the
> render engine aren't used by the display engine, so a more formal
> description of the layout would make this easier to understand.
Documenting this in the previous patch would be the right place. So that
The description of the Color format is available at the modifier declaration.
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 91241f2bc575..213899e202a3 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 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 fdb9dcc4060c..baf1351910ca 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);
> @@
> > -1739,7 +1744,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;
> > }
> > @@ -2154,6 +2160,7 @@ static const u64
> > skl_plane_format_modifiers_ccs[] = { static const u64
> gen12_plane_format_modifiers_mc_ccs[] = {
> > I915_FORMAT_MOD_Y_TILED_GEN12_MC_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,
> > @@ -2162,6 +2169,7 @@ static const u64
> > gen12_plane_format_modifiers_mc_ccs[] = {
> >
> > static const u64 gen12_plane_format_modifiers_rc_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,
> > @@ -2335,6 +2343,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 5fad9cb35979..d8883ad99a67
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6743,6 +6743,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
> > @@ -6782,6 +6784,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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
