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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 21, 2016 at 08:42:13AM +0000, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 18/11/2016 19:53, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@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@bwidawsk.net>
> > Cc: intel-gfx@lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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.

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx