[RFC,5/6] drm/i915: Allow fb modifier to set framebuffer tiling

Commit Message

Tvrtko Ursulin Jan. 30, 2015, 5:36 p.m. UTC

From: Tvrtko Ursulin <[e-mail]>

Use the fb modifier if it was specified over object tiling mode.

Signed-off-by: Tvrtko Ursulin <[e-mail]>
---
 drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 7 deletions(-)

Comments

Daniel Vetter Feb. 2, 2015, 9:54 a.m. UTC | #1

On Fri, Jan 30, 2015 at 05:36:57PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <[e-mail]>
> 
> Use the fb modifier if it was specified over object tiling mode.
> 
> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e22afbe..ca69da0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12671,6 +12671,20 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
>  	.create_handle = intel_user_framebuffer_create_handle,
>  };
>  
> +static unsigned int
> +intel_fb_modifier_to_tiling(u64 modifier)
> +{
> +	switch (modifier) {
> +	case I915_FORMAT_MOD_X_TILED:
> +		return I915_TILING_X;
> +	default:
> +	case I915_FORMAT_MOD_NONE:
> +		break;
> +	}
> +
> +	return I915_TILING_NONE;
> +}
> +
>  static int intel_framebuffer_init(struct drm_device *dev,
>  				  struct intel_framebuffer *intel_fb,
>  				  struct drm_mode_fb_cmd2 *mode_cmd,
> @@ -12678,11 +12692,23 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  {
>  	int aligned_height;
>  	int pitch_limit;
> +	unsigned int tiling_mode = obj->tiling_mode;
>  	int ret;
>  
>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>  
> -	if (obj->tiling_mode == I915_TILING_Y) {
> +	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> +		tiling_mode =
> +			intel_fb_modifier_to_tiling(mode_cmd->modifier[0]);
> +		if (tiling_mode != obj->tiling_mode &&
> +			obj->tiling_mode != I915_TILING_NONE) {
> +			DRM_ERROR("Tiling modifier mismatch %u vs obj %u!\n",
> +					tiling_mode, obj->tiling_mode);
> +			return -EINVAL;
> +		}
> +	}

Ah, here comes the magic. I think this might be simpler if we just use
->modifier (and fix it up if FB_MODIFIERS isn't set).

Btw another reason for this split is that this way we have a clear
separation between the tiling modes supported generally (as fb modifiers)
and the tiling modes supported by fences. It might therefore make sense to
rename obj->tiling_mode with a cocci patch to obj->fencing_mode or
->fence_tiling_mode). To make it really clear that it's just about the
global gtt fences and nothing more.
-Daniel

> +
> +	if (tiling_mode == I915_TILING_Y) {
>  		DRM_DEBUG("hardware does not support tiling Y\n");
>  		return -EINVAL;
>  	}
> @@ -12696,12 +12722,12 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  	if (INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev)) {
>  		pitch_limit = 32*1024;
>  	} else if (INTEL_INFO(dev)->gen >= 4) {
> -		if (obj->tiling_mode)
> +		if (tiling_mode)
>  			pitch_limit = 16*1024;
>  		else
>  			pitch_limit = 32*1024;
>  	} else if (INTEL_INFO(dev)->gen >= 3) {
> -		if (obj->tiling_mode)
> +		if (tiling_mode)
>  			pitch_limit = 8*1024;
>  		else
>  			pitch_limit = 16*1024;
> @@ -12711,12 +12737,12 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  
>  	if (mode_cmd->pitches[0] > pitch_limit) {
>  		DRM_DEBUG("%s pitch (%d) must be at less than %d\n",
> -			  obj->tiling_mode ? "tiled" : "linear",
> +			  tiling_mode ? "tiled" : "linear",
>  			  mode_cmd->pitches[0], pitch_limit);
>  		return -EINVAL;
>  	}
>  
> -	if (obj->tiling_mode != I915_TILING_NONE &&
> +	if (tiling_mode != I915_TILING_NONE && obj->stride &&
>  	    mode_cmd->pitches[0] != obj->stride) {
>  		DRM_DEBUG("pitch (%d) must match tiling stride (%d)\n",
>  			  mode_cmd->pitches[0], obj->stride);
> @@ -12771,7 +12797,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  		return -EINVAL;
>  
>  	aligned_height = intel_fb_align_height(dev, mode_cmd->height,
> -					       obj->tiling_mode);
> +					       tiling_mode);
>  	/* FIXME drm helper for size checks (especially planar formats)? */
>  	if (obj->base.size < aligned_height * mode_cmd->pitches[0])
>  		return -EINVAL;
> @@ -12779,7 +12805,7 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  	drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd);
>  	intel_fb->obj = obj;
>  	intel_fb->obj->framebuffer_references++;
> -	intel_fb->tiling_mode = obj->tiling_mode;
> +	intel_fb->tiling_mode = tiling_mode;
>  
>  	ret = drm_framebuffer_init(dev, &intel_fb->base, &intel_fb_funcs);
>  	if (ret) {
> -- 
> 2.2.2
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Tvrtko Ursulin Feb. 2, 2015, 10:36 a.m. UTC | #2

On 02/02/2015 09:54 AM, Daniel Vetter wrote:
> On Fri, Jan 30, 2015 at 05:36:57PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <[e-mail]>
>>
>> Use the fb modifier if it was specified over object tiling mode.
>>
>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++++++++-------
>>   1 file changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e22afbe..ca69da0 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12671,6 +12671,20 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
>>   	.create_handle = intel_user_framebuffer_create_handle,
>>   };
>>
>> +static unsigned int
>> +intel_fb_modifier_to_tiling(u64 modifier)
>> +{
>> +	switch (modifier) {
>> +	case I915_FORMAT_MOD_X_TILED:
>> +		return I915_TILING_X;
>> +	default:
>> +	case I915_FORMAT_MOD_NONE:
>> +		break;
>> +	}
>> +
>> +	return I915_TILING_NONE;
>> +}
>> +
>>   static int intel_framebuffer_init(struct drm_device *dev,
>>   				  struct intel_framebuffer *intel_fb,
>>   				  struct drm_mode_fb_cmd2 *mode_cmd,
>> @@ -12678,11 +12692,23 @@ static int intel_framebuffer_init(struct drm_device *dev,
>>   {
>>   	int aligned_height;
>>   	int pitch_limit;
>> +	unsigned int tiling_mode = obj->tiling_mode;
>>   	int ret;
>>
>>   	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>
>> -	if (obj->tiling_mode == I915_TILING_Y) {
>> +	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
>> +		tiling_mode =
>> +			intel_fb_modifier_to_tiling(mode_cmd->modifier[0]);
>> +		if (tiling_mode != obj->tiling_mode &&
>> +			obj->tiling_mode != I915_TILING_NONE) {
>> +			DRM_ERROR("Tiling modifier mismatch %u vs obj %u!\n",
>> +					tiling_mode, obj->tiling_mode);
>> +			return -EINVAL;
>> +		}
>> +	}
>
> Ah, here comes the magic. I think this might be simpler if we just use
> ->modifier (and fix it up if FB_MODIFIERS isn't set).
>
> Btw another reason for this split is that this way we have a clear
> separation between the tiling modes supported generally (as fb modifiers)
> and the tiling modes supported by fences. It might therefore make sense to
> rename obj->tiling_mode with a cocci patch to obj->fencing_mode or
> ->fence_tiling_mode). To make it really clear that it's just about the
> global gtt fences and nothing more.

I don't really like using ->modifier directly in tiling patch since it 
is an bag of unrelated stuff, not only a superset. Unrelated especially, 
but not only, from the point of view of call sites / users.

Therefore I see some design elegance in extracting the tiling, or any 
other logical group of modifiers before hand.

At the very least would call something like 
intel_fb_modifier_to_tiling(), but, it is very ugly to have a dynamic 
cost at every call site. Which is another reason why I preferred to 
extract the data before hand.

Regards,

Tvrtko

Daniel Vetter Feb. 2, 2015, 5:15 p.m. UTC | #3

On Mon, Feb 02, 2015 at 10:36:30AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/02/2015 09:54 AM, Daniel Vetter wrote:
> >On Fri, Jan 30, 2015 at 05:36:57PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <[e-mail]>
> >>
> >>Use the fb modifier if it was specified over object tiling mode.
> >>
> >>Signed-off-by: Tvrtko Ursulin <[e-mail]>
> >>---
> >>  drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++++++++-------
> >>  1 file changed, 33 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index e22afbe..ca69da0 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -12671,6 +12671,20 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
> >>  	.create_handle = intel_user_framebuffer_create_handle,
> >>  };
> >>
> >>+static unsigned int
> >>+intel_fb_modifier_to_tiling(u64 modifier)
> >>+{
> >>+	switch (modifier) {
> >>+	case I915_FORMAT_MOD_X_TILED:
> >>+		return I915_TILING_X;
> >>+	default:
> >>+	case I915_FORMAT_MOD_NONE:
> >>+		break;
> >>+	}
> >>+
> >>+	return I915_TILING_NONE;
> >>+}
> >>+
> >>  static int intel_framebuffer_init(struct drm_device *dev,
> >>  				  struct intel_framebuffer *intel_fb,
> >>  				  struct drm_mode_fb_cmd2 *mode_cmd,
> >>@@ -12678,11 +12692,23 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >>  {
> >>  	int aligned_height;
> >>  	int pitch_limit;
> >>+	unsigned int tiling_mode = obj->tiling_mode;
> >>  	int ret;
> >>
> >>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >>
> >>-	if (obj->tiling_mode == I915_TILING_Y) {
> >>+	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> >>+		tiling_mode =
> >>+			intel_fb_modifier_to_tiling(mode_cmd->modifier[0]);
> >>+		if (tiling_mode != obj->tiling_mode &&
> >>+			obj->tiling_mode != I915_TILING_NONE) {
> >>+			DRM_ERROR("Tiling modifier mismatch %u vs obj %u!\n",
> >>+					tiling_mode, obj->tiling_mode);
> >>+			return -EINVAL;
> >>+		}
> >>+	}
> >
> >Ah, here comes the magic. I think this might be simpler if we just use
> >->modifier (and fix it up if FB_MODIFIERS isn't set).
> >
> >Btw another reason for this split is that this way we have a clear
> >separation between the tiling modes supported generally (as fb modifiers)
> >and the tiling modes supported by fences. It might therefore make sense to
> >rename obj->tiling_mode with a cocci patch to obj->fencing_mode or
> >->fence_tiling_mode). To make it really clear that it's just about the
> >global gtt fences and nothing more.
> 
> I don't really like using ->modifier directly in tiling patch since it is an
> bag of unrelated stuff, not only a superset. Unrelated especially, but not
> only, from the point of view of call sites / users.
> 
> Therefore I see some design elegance in extracting the tiling, or any other
> logical group of modifiers before hand.
> 
> At the very least would call something like intel_fb_modifier_to_tiling(),
> but, it is very ugly to have a dynamic cost at every call site. Which is
> another reason why I preferred to extract the data before hand.

The reason is that the current tiling_mode enum is userspace ABI, and
it's just for how to fence global gtt mappings. That's the point of
splitting the fb modifiers out like in this rfc.

So if you add your fancy new tiling mode you can't do that, since you
can't extend the tiling_mode enum. Adding another enum also seems a bit
too much when we already have fb_modifiers.

And if fb_modifiers get too complicated we can add helper functions which
normalize stuff, e.g. extract just the base tiling mode and remove other
things (like compression mode or whatever it's going to be).
-Daniel

Tvrtko Ursulin Feb. 2, 2015, 5:30 p.m. UTC | #4

On 02/02/2015 05:15 PM, Daniel Vetter wrote:
> On Mon, Feb 02, 2015 at 10:36:30AM +0000, Tvrtko Ursulin wrote:
>>
>> On 02/02/2015 09:54 AM, Daniel Vetter wrote:
>>> On Fri, Jan 30, 2015 at 05:36:57PM +0000, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <[e-mail]>
>>>>
>>>> Use the fb modifier if it was specified over object tiling mode.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++++++++-------
>>>>   1 file changed, 33 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index e22afbe..ca69da0 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -12671,6 +12671,20 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
>>>>   	.create_handle = intel_user_framebuffer_create_handle,
>>>>   };
>>>>
>>>> +static unsigned int
>>>> +intel_fb_modifier_to_tiling(u64 modifier)
>>>> +{
>>>> +	switch (modifier) {
>>>> +	case I915_FORMAT_MOD_X_TILED:
>>>> +		return I915_TILING_X;
>>>> +	default:
>>>> +	case I915_FORMAT_MOD_NONE:
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return I915_TILING_NONE;
>>>> +}
>>>> +
>>>>   static int intel_framebuffer_init(struct drm_device *dev,
>>>>   				  struct intel_framebuffer *intel_fb,
>>>>   				  struct drm_mode_fb_cmd2 *mode_cmd,
>>>> @@ -12678,11 +12692,23 @@ static int intel_framebuffer_init(struct drm_device *dev,
>>>>   {
>>>>   	int aligned_height;
>>>>   	int pitch_limit;
>>>> +	unsigned int tiling_mode = obj->tiling_mode;
>>>>   	int ret;
>>>>
>>>>   	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>>>
>>>> -	if (obj->tiling_mode == I915_TILING_Y) {
>>>> +	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
>>>> +		tiling_mode =
>>>> +			intel_fb_modifier_to_tiling(mode_cmd->modifier[0]);
>>>> +		if (tiling_mode != obj->tiling_mode &&
>>>> +			obj->tiling_mode != I915_TILING_NONE) {
>>>> +			DRM_ERROR("Tiling modifier mismatch %u vs obj %u!\n",
>>>> +					tiling_mode, obj->tiling_mode);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +	}
>>>
>>> Ah, here comes the magic. I think this might be simpler if we just use
>>> ->modifier (and fix it up if FB_MODIFIERS isn't set).
>>>
>>> Btw another reason for this split is that this way we have a clear
>>> separation between the tiling modes supported generally (as fb modifiers)
>>> and the tiling modes supported by fences. It might therefore make sense to
>>> rename obj->tiling_mode with a cocci patch to obj->fencing_mode or
>>> ->fence_tiling_mode). To make it really clear that it's just about the
>>> global gtt fences and nothing more.
>>
>> I don't really like using ->modifier directly in tiling patch since it is an
>> bag of unrelated stuff, not only a superset. Unrelated especially, but not
>> only, from the point of view of call sites / users.
>>
>> Therefore I see some design elegance in extracting the tiling, or any other
>> logical group of modifiers before hand.
>>
>> At the very least would call something like intel_fb_modifier_to_tiling(),
>> but, it is very ugly to have a dynamic cost at every call site. Which is
>> another reason why I preferred to extract the data before hand.
>
> The reason is that the current tiling_mode enum is userspace ABI, and
> it's just for how to fence global gtt mappings. That's the point of
> splitting the fb modifiers out like in this rfc.
>
> So if you add your fancy new tiling mode you can't do that, since you
> can't extend the tiling_mode enum. Adding another enum also seems a bit

Why not? It is not changing the ABI since obj->tiling_mode stays exactly 
the same as it is today.

Do you worry about leaking new data out in i915_drm.h, under the 
I915_TILING_* #defines? I don't see that we have to change that at all.

> too much when we already have fb_modifiers.
>
> And if fb_modifiers get too complicated we can add helper functions which
> normalize stuff, e.g. extract just the base tiling mode and remove other
> things (like compression mode or whatever it's going to be).

So you are strongly for "looking into a bag of stuff" to see if anything 
interesting is there on every call site?

Helper functions in my view only marginally help there - they make the 
code neater but design is conceptually still untidy. And you add 
pointless processing on every call site.

I just don't see what is the problem with extracting the interesting 
data "from the bag" at fb init time. If you tried to make some 
synchronization argument in the other reply I don't get it.

fb->modifier[0] should be, in my opinion, viewed as immutable. And it 
lives at the base class level while in intel_frambuffer sub-class it 
should be just fine to "parse" that into directly usable data stored at 
the sub-class level.

Regards,

Tvrtko

Daniel Vetter Feb. 2, 2015, 8:17 p.m. UTC | #5

On Mon, Feb 02, 2015 at 05:30:36PM +0000, Tvrtko Ursulin wrote:
> 
> On 02/02/2015 05:15 PM, Daniel Vetter wrote:
> >On Mon, Feb 02, 2015 at 10:36:30AM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 02/02/2015 09:54 AM, Daniel Vetter wrote:
> >>>On Fri, Jan 30, 2015 at 05:36:57PM +0000, Tvrtko Ursulin wrote:
> >>>>From: Tvrtko Ursulin <[e-mail]>
> >>>>
> >>>>Use the fb modifier if it was specified over object tiling mode.
> >>>>
> >>>>Signed-off-by: Tvrtko Ursulin <[e-mail]>
> >>>>---
> >>>>  drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++++++++-------
> >>>>  1 file changed, 33 insertions(+), 7 deletions(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>index e22afbe..ca69da0 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_display.c
> >>>>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>@@ -12671,6 +12671,20 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
> >>>>  	.create_handle = intel_user_framebuffer_create_handle,
> >>>>  };
> >>>>
> >>>>+static unsigned int
> >>>>+intel_fb_modifier_to_tiling(u64 modifier)
> >>>>+{
> >>>>+	switch (modifier) {
> >>>>+	case I915_FORMAT_MOD_X_TILED:
> >>>>+		return I915_TILING_X;
> >>>>+	default:
> >>>>+	case I915_FORMAT_MOD_NONE:
> >>>>+		break;
> >>>>+	}
> >>>>+
> >>>>+	return I915_TILING_NONE;
> >>>>+}
> >>>>+
> >>>>  static int intel_framebuffer_init(struct drm_device *dev,
> >>>>  				  struct intel_framebuffer *intel_fb,
> >>>>  				  struct drm_mode_fb_cmd2 *mode_cmd,
> >>>>@@ -12678,11 +12692,23 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >>>>  {
> >>>>  	int aligned_height;
> >>>>  	int pitch_limit;
> >>>>+	unsigned int tiling_mode = obj->tiling_mode;
> >>>>  	int ret;
> >>>>
> >>>>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >>>>
> >>>>-	if (obj->tiling_mode == I915_TILING_Y) {
> >>>>+	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> >>>>+		tiling_mode =
> >>>>+			intel_fb_modifier_to_tiling(mode_cmd->modifier[0]);
> >>>>+		if (tiling_mode != obj->tiling_mode &&
> >>>>+			obj->tiling_mode != I915_TILING_NONE) {
> >>>>+			DRM_ERROR("Tiling modifier mismatch %u vs obj %u!\n",
> >>>>+					tiling_mode, obj->tiling_mode);
> >>>>+			return -EINVAL;
> >>>>+		}
> >>>>+	}
> >>>
> >>>Ah, here comes the magic. I think this might be simpler if we just use
> >>>->modifier (and fix it up if FB_MODIFIERS isn't set).
> >>>
> >>>Btw another reason for this split is that this way we have a clear
> >>>separation between the tiling modes supported generally (as fb modifiers)
> >>>and the tiling modes supported by fences. It might therefore make sense to
> >>>rename obj->tiling_mode with a cocci patch to obj->fencing_mode or
> >>>->fence_tiling_mode). To make it really clear that it's just about the
> >>>global gtt fences and nothing more.
> >>
> >>I don't really like using ->modifier directly in tiling patch since it is an
> >>bag of unrelated stuff, not only a superset. Unrelated especially, but not
> >>only, from the point of view of call sites / users.
> >>
> >>Therefore I see some design elegance in extracting the tiling, or any other
> >>logical group of modifiers before hand.
> >>
> >>At the very least would call something like intel_fb_modifier_to_tiling(),
> >>but, it is very ugly to have a dynamic cost at every call site. Which is
> >>another reason why I preferred to extract the data before hand.
> >
> >The reason is that the current tiling_mode enum is userspace ABI, and
> >it's just for how to fence global gtt mappings. That's the point of
> >splitting the fb modifiers out like in this rfc.
> >
> >So if you add your fancy new tiling mode you can't do that, since you
> >can't extend the tiling_mode enum. Adding another enum also seems a bit
> 
> Why not? It is not changing the ABI since obj->tiling_mode stays exactly the
> same as it is today.
> 
> Do you worry about leaking new data out in i915_drm.h, under the
> I915_TILING_* #defines? I don't see that we have to change that at all.

I prefer to keep enums for different types of values separate to avoid
confusion.

> >too much when we already have fb_modifiers.
> >
> >And if fb_modifiers get too complicated we can add helper functions which
> >normalize stuff, e.g. extract just the base tiling mode and remove other
> >things (like compression mode or whatever it's going to be).
> 
> So you are strongly for "looking into a bag of stuff" to see if anything
> interesting is there on every call site?
> 
> Helper functions in my view only marginally help there - they make the code
> neater but design is conceptually still untidy. And you add pointless
> processing on every call site.
> 
> I just don't see what is the problem with extracting the interesting data
> "from the bag" at fb init time. If you tried to make some synchronization
> argument in the other reply I don't get it.

So afaik at most we'll get a few more bits for compression, perhaps
swizzling (although that's dead on gen8+), whatelse. If we lay out the
defines in the intel vendor modifier space we can get at that by simple
masking. Also, kms operations are done at about 60fps rate, so computation
overhead is totally irrelevant (well as long as we just waste a few
cycles).

The synchronization argument is that any kind of duplicated data will get
out of sync sooner or later in my experience. We can't smash everything
into obj->tiling with the addfb2.5 abi (and because they're also for
different things), but we can avoid duplicating information between
fb->modifier and intel_fb->tiling_mode by not having the second.

Yes that means we need to fix up ->modifier (which your patches dont do).
But sooner or later someone will look at ->modifier and not ->tiling_mode
(because hey it worked on new userspace) and then *boom* we have a nice
confusing regression report from someone. Or someone looks at
obj->tiling_mode instead of intel_fb->tiling_mode (hey it works, because
they're using the same enum values) until the newfangled tiling thing
shows up.

> fb->modifier[0] should be, in my opinion, viewed as immutable. And it lives
> at the base class level while in intel_frambuffer sub-class it should be
> just fine to "parse" that into directly usable data stored at the sub-class
> level.

Fully agreed on immutable, but that doesn't exclude computing an
appropriate value at fb init time.
-Daniel

Tvrtko Ursulin Feb. 3, 2015, 10:41 a.m. UTC | #6

On 02/02/2015 08:17 PM, Daniel Vetter wrote:
> On Mon, Feb 02, 2015 at 05:30:36PM +0000, Tvrtko Ursulin wrote:
>>
>> On 02/02/2015 05:15 PM, Daniel Vetter wrote:
>>> On Mon, Feb 02, 2015 at 10:36:30AM +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 02/02/2015 09:54 AM, Daniel Vetter wrote:
>>>>> On Fri, Jan 30, 2015 at 05:36:57PM +0000, Tvrtko Ursulin wrote:
>>>>>> From: Tvrtko Ursulin <[e-mail]>
>>>>>>
>>>>>> Use the fb modifier if it was specified over object tiling mode.
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++++++++-------
>>>>>>   1 file changed, 33 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>> index e22afbe..ca69da0 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>> @@ -12671,6 +12671,20 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
>>>>>>   	.create_handle = intel_user_framebuffer_create_handle,
>>>>>>   };
>>>>>>
>>>>>> +static unsigned int
>>>>>> +intel_fb_modifier_to_tiling(u64 modifier)
>>>>>> +{
>>>>>> +	switch (modifier) {
>>>>>> +	case I915_FORMAT_MOD_X_TILED:
>>>>>> +		return I915_TILING_X;
>>>>>> +	default:
>>>>>> +	case I915_FORMAT_MOD_NONE:
>>>>>> +		break;
>>>>>> +	}
>>>>>> +
>>>>>> +	return I915_TILING_NONE;
>>>>>> +}
>>>>>> +
>>>>>>   static int intel_framebuffer_init(struct drm_device *dev,
>>>>>>   				  struct intel_framebuffer *intel_fb,
>>>>>>   				  struct drm_mode_fb_cmd2 *mode_cmd,
>>>>>> @@ -12678,11 +12692,23 @@ static int intel_framebuffer_init(struct drm_device *dev,
>>>>>>   {
>>>>>>   	int aligned_height;
>>>>>>   	int pitch_limit;
>>>>>> +	unsigned int tiling_mode = obj->tiling_mode;
>>>>>>   	int ret;
>>>>>>
>>>>>>   	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>>>>>
>>>>>> -	if (obj->tiling_mode == I915_TILING_Y) {
>>>>>> +	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
>>>>>> +		tiling_mode =
>>>>>> +			intel_fb_modifier_to_tiling(mode_cmd->modifier[0]);
>>>>>> +		if (tiling_mode != obj->tiling_mode &&
>>>>>> +			obj->tiling_mode != I915_TILING_NONE) {
>>>>>> +			DRM_ERROR("Tiling modifier mismatch %u vs obj %u!\n",
>>>>>> +					tiling_mode, obj->tiling_mode);
>>>>>> +			return -EINVAL;
>>>>>> +		}
>>>>>> +	}
>>>>>
>>>>> Ah, here comes the magic. I think this might be simpler if we just use
>>>>> ->modifier (and fix it up if FB_MODIFIERS isn't set).
>>>>>
>>>>> Btw another reason for this split is that this way we have a clear
>>>>> separation between the tiling modes supported generally (as fb modifiers)
>>>>> and the tiling modes supported by fences. It might therefore make sense to
>>>>> rename obj->tiling_mode with a cocci patch to obj->fencing_mode or
>>>>> ->fence_tiling_mode). To make it really clear that it's just about the
>>>>> global gtt fences and nothing more.
>>>>
>>>> I don't really like using ->modifier directly in tiling patch since it is an
>>>> bag of unrelated stuff, not only a superset. Unrelated especially, but not
>>>> only, from the point of view of call sites / users.
>>>>
>>>> Therefore I see some design elegance in extracting the tiling, or any other
>>>> logical group of modifiers before hand.
>>>>
>>>> At the very least would call something like intel_fb_modifier_to_tiling(),
>>>> but, it is very ugly to have a dynamic cost at every call site. Which is
>>>> another reason why I preferred to extract the data before hand.
>>>
>>> The reason is that the current tiling_mode enum is userspace ABI, and
>>> it's just for how to fence global gtt mappings. That's the point of
>>> splitting the fb modifiers out like in this rfc.
>>>
>>> So if you add your fancy new tiling mode you can't do that, since you
>>> can't extend the tiling_mode enum. Adding another enum also seems a bit
>>
>> Why not? It is not changing the ABI since obj->tiling_mode stays exactly the
>> same as it is today.
>>
>> Do you worry about leaking new data out in i915_drm.h, under the
>> I915_TILING_* #defines? I don't see that we have to change that at all.
>
> I prefer to keep enums for different types of values separate to avoid
> confusion.
>
>>> too much when we already have fb_modifiers.
>>>
>>> And if fb_modifiers get too complicated we can add helper functions which
>>> normalize stuff, e.g. extract just the base tiling mode and remove other
>>> things (like compression mode or whatever it's going to be).
>>
>> So you are strongly for "looking into a bag of stuff" to see if anything
>> interesting is there on every call site?
>>
>> Helper functions in my view only marginally help there - they make the code
>> neater but design is conceptually still untidy. And you add pointless
>> processing on every call site.
>>
>> I just don't see what is the problem with extracting the interesting data
>> "from the bag" at fb init time. If you tried to make some synchronization
>> argument in the other reply I don't get it.
>
> So afaik at most we'll get a few more bits for compression, perhaps
> swizzling (although that's dead on gen8+), whatelse. If we lay out the
> defines in the intel vendor modifier space we can get at that by simple
> masking. Also, kms operations are done at about 60fps rate, so computation
> overhead is totally irrelevant (well as long as we just waste a few
> cycles).
>
> The synchronization argument is that any kind of duplicated data will get
> out of sync sooner or later in my experience. We can't smash everything
> into obj->tiling with the addfb2.5 abi (and because they're also for
> different things), but we can avoid duplicating information between
> fb->modifier and intel_fb->tiling_mode by not having the second.
 >
> Yes that means we need to fix up ->modifier (which your patches dont do).
> But sooner or later someone will look at ->modifier and not ->tiling_mode
> (because hey it worked on new userspace) and then *boom* we have a nice
> confusing regression report from someone. Or someone looks at
> obj->tiling_mode instead of intel_fb->tiling_mode (hey it works, because
> they're using the same enum values) until the newfangled tiling thing
> shows up.
>
>> fb->modifier[0] should be, in my opinion, viewed as immutable. And it lives
>> at the base class level while in intel_frambuffer sub-class it should be
>> just fine to "parse" that into directly usable data stored at the sub-class
>> level.
>
> Fully agreed on immutable, but that doesn't exclude computing an
> appropriate value at fb init time.

So you want to carefully lay out our fb modifiers so we can get our data 
out easily rather than extract the data at fb creation and have no such 
constraints?

Or in other words you want to reserve some bits for tiling and define a 
mask straight away, did I get that right? Not like this:

1.

intel_fb creation
    -> parse fb modifier and extract private attributes for future use
       -> tiling_mode
       -> compression_mode (eg.)
       -> swizzling_mode (eg.)

tiling_mode use sites
    -> just use intel_fb->tiling_mode

But like this:

2.

intel_fb creation
    -> do nothing

call sites
    -> extract tiling_mode from fb modifier

* Note this still needs to map to same "enum" space as today at least in 
places which program the hardware.

You want option 2, correct?

Regards,

Tvrtko

Daniel Vetter Feb. 3, 2015, 11:41 a.m. UTC | #7

On Tue, Feb 03, 2015 at 10:41:49AM +0000, Tvrtko Ursulin wrote:
> 
> On 02/02/2015 08:17 PM, Daniel Vetter wrote:
> >On Mon, Feb 02, 2015 at 05:30:36PM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 02/02/2015 05:15 PM, Daniel Vetter wrote:
> >>>On Mon, Feb 02, 2015 at 10:36:30AM +0000, Tvrtko Ursulin wrote:
> >>>>
> >>>>On 02/02/2015 09:54 AM, Daniel Vetter wrote:
> >>>>>On Fri, Jan 30, 2015 at 05:36:57PM +0000, Tvrtko Ursulin wrote:
> >>>>>>From: Tvrtko Ursulin <[e-mail]>
> >>>>>>
> >>>>>>Use the fb modifier if it was specified over object tiling mode.
> >>>>>>
> >>>>>>Signed-off-by: Tvrtko Ursulin <[e-mail]>
> >>>>>>---
> >>>>>>  drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++++++++-------
> >>>>>>  1 file changed, 33 insertions(+), 7 deletions(-)
> >>>>>>
> >>>>>>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>>>index e22afbe..ca69da0 100644
> >>>>>>--- a/drivers/gpu/drm/i915/intel_display.c
> >>>>>>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>>>@@ -12671,6 +12671,20 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
> >>>>>>  	.create_handle = intel_user_framebuffer_create_handle,
> >>>>>>  };
> >>>>>>
> >>>>>>+static unsigned int
> >>>>>>+intel_fb_modifier_to_tiling(u64 modifier)
> >>>>>>+{
> >>>>>>+	switch (modifier) {
> >>>>>>+	case I915_FORMAT_MOD_X_TILED:
> >>>>>>+		return I915_TILING_X;
> >>>>>>+	default:
> >>>>>>+	case I915_FORMAT_MOD_NONE:
> >>>>>>+		break;
> >>>>>>+	}
> >>>>>>+
> >>>>>>+	return I915_TILING_NONE;
> >>>>>>+}
> >>>>>>+
> >>>>>>  static int intel_framebuffer_init(struct drm_device *dev,
> >>>>>>  				  struct intel_framebuffer *intel_fb,
> >>>>>>  				  struct drm_mode_fb_cmd2 *mode_cmd,
> >>>>>>@@ -12678,11 +12692,23 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >>>>>>  {
> >>>>>>  	int aligned_height;
> >>>>>>  	int pitch_limit;
> >>>>>>+	unsigned int tiling_mode = obj->tiling_mode;
> >>>>>>  	int ret;
> >>>>>>
> >>>>>>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >>>>>>
> >>>>>>-	if (obj->tiling_mode == I915_TILING_Y) {
> >>>>>>+	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> >>>>>>+		tiling_mode =
> >>>>>>+			intel_fb_modifier_to_tiling(mode_cmd->modifier[0]);
> >>>>>>+		if (tiling_mode != obj->tiling_mode &&
> >>>>>>+			obj->tiling_mode != I915_TILING_NONE) {
> >>>>>>+			DRM_ERROR("Tiling modifier mismatch %u vs obj %u!\n",
> >>>>>>+					tiling_mode, obj->tiling_mode);
> >>>>>>+			return -EINVAL;
> >>>>>>+		}
> >>>>>>+	}
> >>>>>
> >>>>>Ah, here comes the magic. I think this might be simpler if we just use
> >>>>>->modifier (and fix it up if FB_MODIFIERS isn't set).
> >>>>>
> >>>>>Btw another reason for this split is that this way we have a clear
> >>>>>separation between the tiling modes supported generally (as fb modifiers)
> >>>>>and the tiling modes supported by fences. It might therefore make sense to
> >>>>>rename obj->tiling_mode with a cocci patch to obj->fencing_mode or
> >>>>>->fence_tiling_mode). To make it really clear that it's just about the
> >>>>>global gtt fences and nothing more.
> >>>>
> >>>>I don't really like using ->modifier directly in tiling patch since it is an
> >>>>bag of unrelated stuff, not only a superset. Unrelated especially, but not
> >>>>only, from the point of view of call sites / users.
> >>>>
> >>>>Therefore I see some design elegance in extracting the tiling, or any other
> >>>>logical group of modifiers before hand.
> >>>>
> >>>>At the very least would call something like intel_fb_modifier_to_tiling(),
> >>>>but, it is very ugly to have a dynamic cost at every call site. Which is
> >>>>another reason why I preferred to extract the data before hand.
> >>>
> >>>The reason is that the current tiling_mode enum is userspace ABI, and
> >>>it's just for how to fence global gtt mappings. That's the point of
> >>>splitting the fb modifiers out like in this rfc.
> >>>
> >>>So if you add your fancy new tiling mode you can't do that, since you
> >>>can't extend the tiling_mode enum. Adding another enum also seems a bit
> >>
> >>Why not? It is not changing the ABI since obj->tiling_mode stays exactly the
> >>same as it is today.
> >>
> >>Do you worry about leaking new data out in i915_drm.h, under the
> >>I915_TILING_* #defines? I don't see that we have to change that at all.
> >
> >I prefer to keep enums for different types of values separate to avoid
> >confusion.
> >
> >>>too much when we already have fb_modifiers.
> >>>
> >>>And if fb_modifiers get too complicated we can add helper functions which
> >>>normalize stuff, e.g. extract just the base tiling mode and remove other
> >>>things (like compression mode or whatever it's going to be).
> >>
> >>So you are strongly for "looking into a bag of stuff" to see if anything
> >>interesting is there on every call site?
> >>
> >>Helper functions in my view only marginally help there - they make the code
> >>neater but design is conceptually still untidy. And you add pointless
> >>processing on every call site.
> >>
> >>I just don't see what is the problem with extracting the interesting data
> >>"from the bag" at fb init time. If you tried to make some synchronization
> >>argument in the other reply I don't get it.
> >
> >So afaik at most we'll get a few more bits for compression, perhaps
> >swizzling (although that's dead on gen8+), whatelse. If we lay out the
> >defines in the intel vendor modifier space we can get at that by simple
> >masking. Also, kms operations are done at about 60fps rate, so computation
> >overhead is totally irrelevant (well as long as we just waste a few
> >cycles).
> >
> >The synchronization argument is that any kind of duplicated data will get
> >out of sync sooner or later in my experience. We can't smash everything
> >into obj->tiling with the addfb2.5 abi (and because they're also for
> >different things), but we can avoid duplicating information between
> >fb->modifier and intel_fb->tiling_mode by not having the second.
> >
> >Yes that means we need to fix up ->modifier (which your patches dont do).
> >But sooner or later someone will look at ->modifier and not ->tiling_mode
> >(because hey it worked on new userspace) and then *boom* we have a nice
> >confusing regression report from someone. Or someone looks at
> >obj->tiling_mode instead of intel_fb->tiling_mode (hey it works, because
> >they're using the same enum values) until the newfangled tiling thing
> >shows up.
> >
> >>fb->modifier[0] should be, in my opinion, viewed as immutable. And it lives
> >>at the base class level while in intel_frambuffer sub-class it should be
> >>just fine to "parse" that into directly usable data stored at the sub-class
> >>level.
> >
> >Fully agreed on immutable, but that doesn't exclude computing an
> >appropriate value at fb init time.
> 
> So you want to carefully lay out our fb modifiers so we can get our data out
> easily rather than extract the data at fb creation and have no such
> constraints?

Not sure it'll be that bad really, often it should look like this I expect
(totally made up example):

	switch(fb->modifier) {
		case MOD_NONE:
			break;
		case MOD_X:
			plane_mode |= TILE_X
			break;
		case MOD_FANCY_TILING_WITH_COMPRESSION:
			plane_mode |= COMPRESSED;
		case MOD_FANCY_TILING:
			plane_mode |= TILE_FANCY;
			break;
		default:
			/* this plane doesn't support any other
			 * combination */
			MISSING_CASE(fb->modifier);
	}

So not even sure we'd need to split stuff up that badly and make it all
maskable. In any case 56 bits gives us a lot of places for submasks, I
don't think that's a severe restriction really.

> Or in other words you want to reserve some bits for tiling and define a mask
> straight away, did I get that right? Not like this:
> 
> 1.
> 
> intel_fb creation
>    -> parse fb modifier and extract private attributes for future use
>       -> tiling_mode
>       -> compression_mode (eg.)
>       -> swizzling_mode (eg.)
> 
> tiling_mode use sites
>    -> just use intel_fb->tiling_mode
> 
> But like this:
> 
> 2.
> 
> intel_fb creation
>    -> do nothing

s/do nothing/fixup fb modifier for legacy userspace not using it/

> call sites
>    -> extract tiling_mode from fb modifier
> 
> * Note this still needs to map to same "enum" space as today at least in
> places which program the hardware.
> 
> You want option 2, correct?

Yes, I think that overall has less opportunities for accidental bugs.
-Daniel

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e22afbe..ca69da0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12671,6 +12671,20 @@  static const struct drm_framebuffer_funcs intel_fb_funcs = {
 	.create_handle = intel_user_framebuffer_create_handle,
 };
 
+static unsigned int
+intel_fb_modifier_to_tiling(u64 modifier)
+{
+	switch (modifier) {
+	case I915_FORMAT_MOD_X_TILED:
+		return I915_TILING_X;
+	default:
+	case I915_FORMAT_MOD_NONE:
+		break;
+	}
+
+	return I915_TILING_NONE;
+}
+
 static int intel_framebuffer_init(struct drm_device *dev,
 				  struct intel_framebuffer *intel_fb,
 				  struct drm_mode_fb_cmd2 *mode_cmd,
@@ -12678,11 +12692,23 @@  static int intel_framebuffer_init(struct drm_device *dev,
 {
 	int aligned_height;
 	int pitch_limit;
+	unsigned int tiling_mode = obj->tiling_mode;
 	int ret;
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
-	if (obj->tiling_mode == I915_TILING_Y) {
+	if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
+		tiling_mode =
+			intel_fb_modifier_to_tiling(mode_cmd->modifier[0]);
+		if (tiling_mode != obj->tiling_mode &&
+			obj->tiling_mode != I915_TILING_NONE) {
+			DRM_ERROR("Tiling modifier mismatch %u vs obj %u!\n",
+					tiling_mode, obj->tiling_mode);
+			return -EINVAL;
+		}
+	}
+
+	if (tiling_mode == I915_TILING_Y) {
 		DRM_DEBUG("hardware does not support tiling Y\n");
 		return -EINVAL;
 	}
@@ -12696,12 +12722,12 @@  static int intel_framebuffer_init(struct drm_device *dev,
 	if (INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev)) {
 		pitch_limit = 32*1024;
 	} else if (INTEL_INFO(dev)->gen >= 4) {
-		if (obj->tiling_mode)
+		if (tiling_mode)
 			pitch_limit = 16*1024;
 		else
 			pitch_limit = 32*1024;
 	} else if (INTEL_INFO(dev)->gen >= 3) {
-		if (obj->tiling_mode)
+		if (tiling_mode)
 			pitch_limit = 8*1024;
 		else
 			pitch_limit = 16*1024;
@@ -12711,12 +12737,12 @@  static int intel_framebuffer_init(struct drm_device *dev,
 
 	if (mode_cmd->pitches[0] > pitch_limit) {
 		DRM_DEBUG("%s pitch (%d) must be at less than %d\n",
-			  obj->tiling_mode ? "tiled" : "linear",
+			  tiling_mode ? "tiled" : "linear",
 			  mode_cmd->pitches[0], pitch_limit);
 		return -EINVAL;
 	}
 
-	if (obj->tiling_mode != I915_TILING_NONE &&
+	if (tiling_mode != I915_TILING_NONE && obj->stride &&
 	    mode_cmd->pitches[0] != obj->stride) {
 		DRM_DEBUG("pitch (%d) must match tiling stride (%d)\n",
 			  mode_cmd->pitches[0], obj->stride);
@@ -12771,7 +12797,7 @@  static int intel_framebuffer_init(struct drm_device *dev,
 		return -EINVAL;
 
 	aligned_height = intel_fb_align_height(dev, mode_cmd->height,
-					       obj->tiling_mode);
+					       tiling_mode);
 	/* FIXME drm helper for size checks (especially planar formats)? */
 	if (obj->base.size < aligned_height * mode_cmd->pitches[0])
 		return -EINVAL;
@@ -12779,7 +12805,7 @@  static int intel_framebuffer_init(struct drm_device *dev,
 	drm_helper_mode_fill_fb_struct(&intel_fb->base, mode_cmd);
 	intel_fb->obj = obj;
 	intel_fb->obj->framebuffer_references++;
-	intel_fb->tiling_mode = obj->tiling_mode;
+	intel_fb->tiling_mode = tiling_mode;
 
 	ret = drm_framebuffer_init(dev, &intel_fb->base, &intel_fb_funcs);
 	if (ret) {