[37/37] drm/i915: Implement .get_format_info() hook for CCS

Commit Message

Ville Syrjälä Nov. 18, 2016, 7:53 p.m. UTC

From: Ville Syrjälä <[e-mail]>

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 <[e-mail]>
Cc: [e-mail]
Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++++++++++++++++
 include/uapi/drm/drm_fourcc.h        |  3 +++
 2 files changed, 40 insertions(+)

Comments

Ben Widawsky Nov. 18, 2016, 11:31 p.m. UTC | #1

On 16-11-18 21:53:13, Ville Syrjälä wrote:
>From: Ville Syrjälä <[e-mail]>
>
>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.
>

I was hoping to see that patch as well :-). If you're adding the new fb
modifiers, I think it'd make sense to make it part of this series.
Alternatively, I can take 36, and 37 and make it part of my series, then
integrate that last bit. It's up to you.

>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 <[e-mail]>
>Cc: [e-mail]
>Signed-off-by: Ville Syrjälä <[e-mail]>

I have a comment below however, you can consider it:
Reviewed-by: Ben Widawsky <[e-mail]>

>---
> 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;
>+	}
>+}
>+

It sort of seems like somewhat of a waste to provide this if implementations are
allowed to return NULL. It's like saying, "DRM core will check stuff for you if
you provide the info, but you don't have to do it if you don't want to." If
that's the case you may as well provide a driver hook to just do the check, ie.
s/mod_funcs->get_format_info/mode_functs->check_format/

> 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)
>+
> /*
>  * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
>  *

Tvrtko Ursulin Nov. 21, 2016, 8:42 a.m. UTC | #2

Hi,

On 18/11/2016 19:53, [e-mail] wrote:
> From: Ville Syrjälä <[e-mail]>
>
> 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 <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Ville Syrjälä <[e-mail]>
> ---
>  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?

Regards,

Tvrtko

Ville Syrjälä Nov. 21, 2016, 1:27 p.m. UTC | #3

On Mon, Nov 21, 2016 at 08:42:13AM +0000, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 18/11/2016 19:53, [e-mail] wrote:
> > From: Ville Syrjälä <[e-mail]>
> >
> > 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 <[e-mail]>
> > Cc: [e-mail]
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> > ---
> >  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.

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.

Tvrtko Ursulin Nov. 21, 2016, 2:04 p.m. UTC | #4

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, [e-mail] wrote:
>>> From: Ville Syrjälä <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Cc: [e-mail]
>>> Signed-off-by: Ville Syrjälä <[e-mail]>
>>> ---
>>>  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

Ville Syrjälä Nov. 21, 2016, 2:37 p.m. UTC | #5

On Fri, Nov 18, 2016 at 03:31:48PM -0800, Ben Widawsky wrote:
> On 16-11-18 21:53:13, Ville Syrjälä wrote:
> >From: Ville Syrjälä <[e-mail]>
> >
> >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.
> >
> 
> I was hoping to see that patch as well :-). If you're adding the new fb
> modifiers, I think it'd make sense to make it part of this series.
> Alternatively, I can take 36, and 37 and make it part of my series, then
> integrate that last bit. It's up to you.
> 
> >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 <[e-mail]>
> >Cc: [e-mail]
> >Signed-off-by: Ville Syrjälä <[e-mail]>
> 
> I have a comment below however, you can consider it:
> Reviewed-by: Ben Widawsky <[e-mail]>
> 
> >---
> > 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;
> >+	}
> >+}
> >+
> 
> It sort of seems like somewhat of a waste to provide this if implementations are
> allowed to return NULL. It's like saying, "DRM core will check stuff for you if
> you provide the info, but you don't have to do it if you don't want to."

The core will check the stuff anyway. The NULL just means "I don't have
any special requirements, so the core format info will be sufficient".

> If
> that's the case you may as well provide a driver hook to just do the check, ie.
> s/mod_funcs->get_format_info/mode_functs->check_format/

Drivers already have to do a bunch of checks in .fb_create(). In
addition the core does some basic sanity checks before the driver
even sees the mode_cmd (except for the new .get_format_info() hook
that is). I don't want every driver to have to duplicate all of these
basic sanity checks.

One alternative to this scheme would be have a helper function that
every driver would call in .fb_create() that would do these basic sanity
checks. That way we wouldn't need the extra hook, with a slight risk
that driver would forget to call the helper.

> 
> > 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)
> >+
> > /*
> >  * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
> >  *
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center

Patch

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)
+
 /*
  * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
  *