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

[Tvrtko Ursulin]

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 <tvrtko.ursulin at intel.com>
>>>>
>>>> Use the fb modifier if it was specified over object tiling mode.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> ---
>>>>   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