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

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

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

Regards,

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