Re: [Intel-gfx] [PATCH v3 01/11] drm/i915: Add a table with a descriptor for all i915 modifiers

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

On Wed, Oct 20, 2021 at 01:46:30PM +0300, Imre Deak wrote:
> On Wed, Oct 20, 2021 at 12:40:30PM +0300, Ville Syrjälä wrote:
> > On Fri, Oct 15, 2021 at 01:09:11AM +0300, Imre Deak wrote:
> > > +static const struct intel_modifier_desc intel_modifiers[] = {
> > > +	{
> > > +		.modifier = DRM_FORMAT_MOD_LINEAR,
> > > +		.display_ver = DISPLAY_VER_ALL,
> > > +
> > > +		.is_linear = true,
> > > +	},
> > > +	{
> > > +		.modifier = I915_FORMAT_MOD_X_TILED,
> > > +		.display_ver = DISPLAY_VER_ALL,
> > > +	},
> > > +	{
> > > +		.modifier = I915_FORMAT_MOD_Y_TILED,
> > > +		.display_ver = { 9, 13 },
> > > +	},
> > > +	{
> > > +		.modifier = I915_FORMAT_MOD_Yf_TILED,
> > > +		.display_ver = { 9, 11 },
> > > +	},
> > > +	{
> > > +		.modifier = I915_FORMAT_MOD_Y_TILED_CCS,
> > > +		.display_ver = { 9, 11 },
> > > +
> > > +		.ccs.type = INTEL_CCS_RC,
> > > +	},
> > > +	{
> > > +		.modifier = I915_FORMAT_MOD_Yf_TILED_CCS,
> > > +		.display_ver = { 9, 11 },
> > > +
> > > +		.ccs.type = INTEL_CCS_RC,
> > > +	},
> > > +	{
> > > +		.modifier = I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > > +		.display_ver = { 12, 13 },
> > > +
> > > +		.ccs.type = INTEL_CCS_RC,
> > > +	},
> > > +	{
> > > +		.modifier = I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC,
> > > +		.display_ver = { 12, 13 },
> > > +
> > > +		.ccs.type = INTEL_CCS_RC_CC,
> > > +	},
> > > +	{
> > > +		.modifier = I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS,
> > > +		.display_ver = { 12, 13 },
> > > +
> > > +		.ccs.type = INTEL_CCS_MC,
> > > +	},
> > > +};
> > > +
> > <snip>
> > > +u64 *intel_fb_plane_get_modifiers(struct drm_i915_private *i915,
> > > +				  enum intel_plane_caps plane_caps)
> > > +{
> > > +	u64 *list, *p;
> > > +	int count = 1;		/* +1 for invalid modifier terminator */
> > > +	int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(intel_modifiers); i++) {
> > > +		if (plane_has_modifier(i915, plane_caps, &intel_modifiers[i]))
> > > +			count++;
> > > +	}
> > > +
> > > +	list = kmalloc_array(count, sizeof(*list), GFP_KERNEL);
> > > +	if (drm_WARN_ON(&i915->drm, !list))
> > > +		return NULL;
> > > +
> > > +	p = list;
> > > +	for (i = 0; i < ARRAY_SIZE(intel_modifiers); i++) {
> > > +		if (plane_has_modifier(i915, plane_caps, &intel_modifiers[i]))
> > > +			*p++ = intel_modifiers[i].modifier;
> > > +	}
> > > +	*p++ = DRM_FORMAT_MOD_INVALID;
> > 
> > Oh, one thing I just realized is that this will now list the modifiers
> > in the opposite order to what we had before. Previously we had roughly
> > compressed->tiled->linear order. I'm not sure sure anything relies on
> > that, but seems best to try and preserve it. I guess one could think
> > of it as some kind of priority order for the modifiers, where the more
> > efficient ones (in some sense) come first.
> 
> Hm, right that was subtle, thanks for catching it. 
> 
> As I understood Mesa (for instance) has to know what kind of modifiers
> it sees and do a priority reorder for other clients anyway (which don't
> know more about the mods besides the ID?).
> 
> +Danvet.
> 
> But the order shouldn't definitely be changed if there is no reason for
> it. Ensuring some priority order scheme already at the kernel i/f makes
> also sense to me. So if it's ok, I'll fix it up to be in the
> 
> gen12_mc -> gen12_rc -> gen12_rc_cc -> gen9_yf_rc -> gen9_y_rc -> yf_tiled -> y_tiled -> x_tiled -> linear
> 
> order, which is the current one.
> 
> For that matter, shouldn't gen12_rc_cc be before gen12_rc?

Probably. No idea why it's not currently.

-- 
Ville Syrjälä
Intel