Re: [PATCH 3/4] drm/i915: Use frame buffer modifiers for tiled display

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 02/04/2015 02:25 PM, Daniel Vetter wrote:
> On Wed, Feb 04, 2015 at 10:01:45AM +0000, Tvrtko Ursulin wrote:
>>
>> On 02/03/2015 07:47 PM, Daniel Vetter wrote:
>>> On Tue, Feb 03, 2015 at 05:22:31PM +0000, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Start using frame buffer modifiers instead of object tiling mode
>>>> for display purposes.
>>>>
>>>> To ensure compatibility with old userspace which is using set_tiling
>>>> and does not know about frame buffer modifiers, the latter are faked
>>>> internally when tile object is set for display. This way all interested
>>>> call sites can use fb modifiers exclusively.
>>>>
>>>> Also ensure tiling specified via fb modifiers must match object tiling
>>>> used for fencing if both are specified.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_display.c | 95 +++++++++++++++++++++++++-----------
>>>>   drivers/gpu/drm/i915/intel_drv.h     |  2 +
>>>>   drivers/gpu/drm/i915/intel_pm.c      |  7 +--
>>>>   drivers/gpu/drm/i915/intel_sprite.c  | 26 +++++-----
>>>>   4 files changed, 85 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 7a3ed61..6825016 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -2198,6 +2198,19 @@ intel_fb_align_height(struct drm_device *dev, int height, unsigned int tiling)
>>>>   	return ALIGN(height, tile_height);
>>>>   }
>>>>
>>>> +static unsigned int intel_fb_modifier_to_tiling(u64 mod)
>>>> +{
>>>> +	BUILD_BUG_ON((I915_FORMAT_MOD_X_TILED & 0x00ffffffffffffffL) !=
>>>> +		     I915_TILING_X);
>>>> +
>>>> +	return mod & 1;
>>>> +}
>>>> +
>>>> +unsigned int intel_fb_tiling_mode(struct drm_framebuffer *fb)
>>>> +{
>>>> +	return intel_fb_modifier_to_tiling(fb->modifier[0]);
>>>> +}
>>>
>>> I expect that these here will create a bit of churn with the skl patches
>>> you have based, since I really don't want a new I915_TILING_FANCY define
>>> in the enum space used by obj->tiling mode. But makes sense for backwards
>>> compat with older platforms and less churn in code.
>>
>> I thought we talked about effectively creating a new enum space for fb
>> tiling? By masking out bits from the fb modifier, no? Only thing for
>> backward compatibility is that object X tiling and fb X tiling == 1.
>
> intel_fb_tiling_mode maps modifier (the new enum space) to
> obj->tiling_mode (the old enum space). Means a notch less churn in legacy
> code (but if that's the metric I'd just have kept using obj->tiling_mode
> there). But means that you get to chance skl code twice, because I very
> much don't want a new I915_TILING_DEFINE but instead the skl code should
> check the new modifiers directly. Otherwise we can mash up tiling modes
> valid just for ggtt fencing and fb modifiers in general.
>
> Maybe I wasn't really clear with what I've meant ...

It does seem it is taking very long to get on the same page here. :/

I did not plan to add new I915_TILING_xxx. I was exploiting the fact 
both map to the same value, with masking. So legacy continues to work 
since this will be true forever. (ABI)

Then the plan was to add a new namespace for display tiling enums.

This was since fb modifier could contain more than tiling and this way 
it is possible to mask out and case-switch just as the current code does.

There are three namespaces here:

1. I915_TILING_xxx
2. I915_FORMAT_MOD_ (fb modifiers)
3. Tiling as programmed to display hardware

And then add a fourth one:

4. I915_DISPLAY_TILING_xxx

At this step also add something like I915_FORMAT_MOD_TILING_MASK and 
redefine I915_FORMAT_MOD_X_TILE to be fourcc_mod(INTEL, 
I915_DISPLAY_TILING_X). (Instead of hardcoded 1)

At call sites (opencoded):

switch (fb->modifier[0] & I915_FORMAT_MOD_TILING) {
case I915_DISPLAY_TILING_X:
...

I mean we could do:

switch (fb->modifier[0]) {
case I915_FORMAT_MOD_X_TILE:
...

If fb modifiers won't have any overlap, like for example:

#define I915_FORMAT_MOD_X_TILE fourcc_mod(INTEL, 1)
#define I915_FORMAT_MOD_X_TILE_AND_UNRELATED fourcc_mod(INTEL, 1<<8 && 1)

Then the direct usage stops working..

Up to you, I have to unblock other stuff so we can't strangle this for 
too long.

>>> With igt for the new cases in addfb and review this is imo good to get in.
>>
>> I can do the IGT, but who is doing the libdrm part? :)
>
> Generally when we do igts for new interfaces we just copypaste the new
> struct definitions with local_ prefixed to avoid blocking the test on a
> new libdrm release. So no one needs to do a libdrm patch ;-)

Okay I can do that. Even better that's what I already did. :)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx