[Intel-gfx] [PATCH 8/9] drm/i915: Implement .get_format_info() hook for CCS

[Ben Widawsky]

On 17-01-05 16:24:56, Tvrtko Ursulin wrote:
>
>On 04/01/2017 18:42, ville.syrjala at linux.intel.com wrote:
>>From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>
>>SKL+ display engine can scan out certain kinds of compressed surfaces
>>produced by the render engine. This involved telling the display engine
>>the location of the color control surfae (CCS) which describes which
>>parts of the main surface are compressed and which are not. The location
>>of CCS is provided by userspace as just another plane with its own offset.
>>
>>By providing our own format information for the CCS formats, we should
>>be able to make framebuffer_check() do the right thing for the CCS
>>surface as well.
>>
>>Note that we'll return the same format info for both Y and Yf tiled
>>format as that's what happens with the non-CCS Y vs. Yf as well. If
>>desired, we could potentially return a unique pointer for each
>>pixel_format+tiling+ccs combination, in which case we immediately be
>>able to tell if any of that stuff changed by just comparing the
>>pointers. But that does sound a bit wasteful space wise.
>>
>>v2: Drop the 'dev' argument from the hook
>>v3: Include the description of the CCS surface layout
>>
>>Cc: Vandana Kannan <vandana.kannan at intel.com>
>>Cc: Daniel Vetter <daniel at ffwll.ch>
>>Cc: Ben Widawsky <ben at bwidawsk.net>
>>Cc: Jason Ekstrand <jason at jlekstrand.net>
>>Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
>>Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>---
>> drivers/gpu/drm/i915/intel_display.c | 36 ++++++++++++++++++++++++++
>> include/uapi/drm/drm_fourcc.h        | 49 ++++++++++++++++++++++++++++++++++++
>> 2 files changed, 85 insertions(+)
>>
>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>index c4662b2e9613..38de9df0ec60 100644
>>--- a/drivers/gpu/drm/i915/intel_display.c
>>+++ b/drivers/gpu/drm/i915/intel_display.c
>>@@ -2478,6 +2478,41 @@ static unsigned int intel_fb_modifier_to_tiling(uint64_t fb_modifier)
>> 	}
>> }
>>
>>+static const struct drm_format_info ccs_formats[] = {
>>+	{ .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 8, },
>>+	{ .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 8, },
>>+	{ .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 8, },
>>+	{ .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 8, },
>>+};
>>+
>>+static const struct drm_format_info *
>>+lookup_format_info(const struct drm_format_info formats[],
>>+		   int num_formats, u32 format)
>>+{
>>+	int i;
>>+
>>+	for (i = 0; i < num_formats; i++) {
>>+		if (formats[i].format == format)
>>+			return &formats[i];
>>+	}
>>+
>>+	return NULL;
>>+}
>>+
>>+static const struct drm_format_info *
>>+intel_get_format_info(const struct drm_mode_fb_cmd2 *cmd)
>>+{
>>+	switch (cmd->modifier[0]) {
>>+	case I915_FORMAT_MOD_Y_TILED_CCS:
>>+	case I915_FORMAT_MOD_Yf_TILED_CCS:
>>+		return lookup_format_info(ccs_formats,
>>+					  ARRAY_SIZE(ccs_formats),
>>+					  cmd->pixel_format);
>>+	default:
>>+		return NULL;
>>+	}
>>+}
>>+
>> static int
>> intel_fill_fb_info(struct drm_i915_private *dev_priv,
>> 		   struct drm_framebuffer *fb)
>>@@ -16083,6 +16118,7 @@ static void intel_atomic_state_free(struct drm_atomic_state *state)
>>
>> static const struct drm_mode_config_funcs intel_mode_funcs = {
>> 	.fb_create = intel_user_framebuffer_create,
>>+	.get_format_info = intel_get_format_info,
>> 	.output_poll_changed = intel_fbdev_output_poll_changed,
>> 	.atomic_check = intel_atomic_check,
>> 	.atomic_commit = intel_atomic_commit,
>>diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
>>index 9e1bb7fabcde..4581e3d41e5c 100644
>>--- a/include/uapi/drm/drm_fourcc.h
>>+++ b/include/uapi/drm/drm_fourcc.h
>>@@ -230,6 +230,55 @@ extern "C" {
>> #define I915_FORMAT_MOD_Yf_TILED fourcc_mod_code(INTEL, 3)
>>
>> /*
>>+ * Intel color control surface (CCS) for render compression
>>+ *
>>+ * The framebuffer format must be one of the 8:8:8:8 RGB formats,
>>+ * the main surface will be plane index 0 and will be Y/Yf-tiled,
>>+ * the CCS will be plane index 1.
>>+ *
>>+ * Each byte of CCS contains 4 pairs of bits, with each pair of bits
>>+ * matching an area of 8x4 pixels of the main surface. Which would seem
>>+ * to match 2 cachelines containing 4x4 pixels each. The pairs bits within
>>+ * the byte form a 2x2 grid, which thus matches a 16x8 pixel area of the
>>+ * main surface. This is the 2x2 pattern the bits form (0=lsb, 7=msb):
>>+ * -----------
>>+ * | 01 | 23 |
>>+ *  ----------
>>+ * | 45 | 67 |
>>+ * -----------
>>+ *
>>+ * Actually only the lower bit of the pair seems to have any effect.
>>+ * No idea why. 0 in the lower bit would seem to mean not compressed,
>>+ * and 1 is compressed.  The interpreation of the main surface data
>>+ * when the block is marked compressed is unknown as of now.
>>+ *
>>+ * CCS tile is laid out in 8 byte horizontal strips each strip thus corresponds
>>+ * to a 128x8 pixel are of the main surface. So each 8x8 bytes of the CCS
>>+ * (1 cacheline) will match an area of 4x2 tiles on the main surface.
>>+ *
>>+ * Here is the layout of a full CCS tile, with the 8 byte strips numbered 0-511:
>>+ * ------------------------
>>+ * |  0 |  64 | ... | 448 |
>>+ * |  1 |  65 |     | 449 |
>>+ * |  2 |  66 |     | 450 |
>>+ * |  . |   . |     |   . |
>>+ * |  . |   . |     |   . |
>>+ * |  . |   . |     |   . |
>>+ * | 63 | 127 |     | 511 |
>>+ * ------------------------
>>+ *
>>+ * This will match an area of 1024x512 pixels on the main surface.
>>+ *
>>+ * The CCS plane pitch must be a multiple of the CCS tile width (64 bytes),
>>+ * and for the purposes of the CCS plane offset we assume cpp==1. As each
>>+ * byte matches a 16x8 area of the main surface, the dimensions of the CCS
>>+ * plane will thus appear to be width/16 x height/8. Both planes must be
>>+ * contained within the same gem object.
>>+ */
>>+#define I915_FORMAT_MOD_Y_TILED_CCS	fourcc_mod_code(INTEL, 4)
>>+#define I915_FORMAT_MOD_Yf_TILED_CCS	fourcc_mod_code(INTEL, 5)
>
>Are we sure this is better than reserving some bits for tiling mode 
>and having CCS as separate bit flag? IMHO code is already a bit 
>unsightly with this scheme and it would take just one more orthogonal 
>but simultaneously usable set of modifiers to drown us in 
>permutations. We got plenty of bits available.
>
>Regards,
>
>Tvrtko

Adding Kristian...

Obviously it's a matter of opinion but I certainly don't think it's unsightly
now. I agree that if we have 1 more orthogonal modifier makes it bad, and 2
pretty much make it terrible which was actually what I liked about having the
per plane modifiers. Anyway, it seems everyone has mostly agreed on this
already and I'd propose we move forward and say that if this scheme doesn't
work, we bail for addfb3.

I'm not entirely convinced we need a Yf even now....