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

[Tvrtko Ursulin]

On 21/11/2016 13:27, Ville Syrjälä wrote:
> On Mon, Nov 21, 2016 at 08:42:13AM +0000, Tvrtko Ursulin wrote:
>>
>> Hi,
>>
>> On 18/11/2016 19:53, ville.syrjala at linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>
>>> 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.
>>>
>>> Cc: Ben Widawsky <ben at bwidawsk.net>
>>> Cc: intel-gfx at lists.freedesktop.org
>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++++++++++++++++
>>>  include/uapi/drm/drm_fourcc.h        |  3 +++
>>>  2 files changed, 40 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 7b7135be3b9e..de6909770c68 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -2488,6 +2488,42 @@ 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(struct drm_device *dev,
>>> +		      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)
>>> @@ -15922,6 +15958,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
>>>
>>>  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 a5890bf44c0a..2926d916f199 100644
>>> --- a/include/uapi/drm/drm_fourcc.h
>>> +++ b/include/uapi/drm/drm_fourcc.h
>>> @@ -218,6 +218,9 @@ extern "C" {
>>>   */
>>>  #define I915_FORMAT_MOD_Yf_TILED fourcc_mod_code(INTEL, 3)
>>>
>>> +#define I915_FORMAT_MOD_Y_TILED_CCS	fourcc_mod_code(INTEL, 4)
>>> +#define I915_FORMAT_MOD_Yf_TILED_CCS	fourcc_mod_code(INTEL, 5)
>>> +
>>
>> I think when fb modifiers were started the idea was that we would later
>> partition our vendor bit space for different classes of things and have
>> helper functions to extract the tiling, etc, from them.
>>
>> For example have first 3-4 bits represent the tiling, then in this case
>> one bit for CCS, etc.
>>
>> Have you considered that when adding these ones, and concluded this
>> different scheme is better for some reason?
>
> I haven't considered anything. And obviously this patch isn't meant
> for inclusion as is. I just needed sometime to make it compile.

No idea on the status of this series. Just noticed new modifiers by 
accident and remembered the early discussions.

> Generally I don't think adding magic meaning for individual bits for
> things like this is a particularly good idea. Every time I've seen a
> scheme like that it has eventually turned ugly on account of running
> out of bits in one place or another.

I think in this case it might be much better. You just need one more 
feature which intersects with tiling and ccs to make the list not very 
manageable.

Regards,

Tvrtko