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

[Tvrtko Ursulin]

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 <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.
>
> 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