[Intel-gfx] [RFC 4/6] drm/i915: Use framebuffer tiling mode for display purposes

[Tvrtko Ursulin]

On 02/02/2015 09:49 AM, Daniel Vetter wrote:
> On Fri, Jan 30, 2015 at 05:36:56PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> To prepare for framebuffer modifiers, move tiling definition from the
>> object into the framebuffer. Move in a way that framebuffer tiling is
>> now used for display while object tiling remains for fencing.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 46 +++++++++++++++++++++---------------
>>   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, 46 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4425e86..e22afbe 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2211,7 +2211,7 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>>
>>   	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>
>> -	switch (obj->tiling_mode) {
>> +	switch (to_intel_framebuffer(fb)->tiling_mode) {
>>   	case I915_TILING_NONE:
>
> Imo we should just look at fb->modifier[0] and flip over all the enums. A
> bit more invasive, but we also don't need to change all the platform code
> at once - set_tiling already guarantees that no one can modify the bo
> tiling mode when there's an fb object using it. Which means we can change
> the code over at leasure.

What do you mean by "flip over"?

To make places which need to get fb tiling format use fb->modifier[0] 
directly rather than have them mapped to tiling enums at fb init?

>> @@ -9764,6 +9768,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>   	work->event = event;
>>   	work->crtc = crtc;
>>   	work->old_fb_obj = intel_fb_obj(old_fb);
>> +	work->old_tiling_mode = to_intel_framebuffer(old_fb)->tiling_mode;
>
> Hm, that's actually an interesting bugfix - currently userspace could be
> sneaky and destroy the old fb immediately after the flip completes and the
> change the tiling of the underlying object before the unpin work had a
> chance to run (needs some fudgin with rt prios to starve workers to make
> this work though).
>
> Imo the right fix is to hold a reference onto the fb and not the
> underlying gem object. With that tiling is guaranteed not to change.

Ok I'll pull it out in a separate patch.

>>   	INIT_WORK(&work->work, intel_unpin_work_fn);
>>
>>   	ret = drm_crtc_vblank_get(crtc);
>> @@ -9814,7 +9819,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>
>>   	if (IS_VALLEYVIEW(dev)) {
>>   		ring = &dev_priv->ring[BCS];
>> -		if (obj->tiling_mode != work->old_fb_obj->tiling_mode)
>> +		if (to_intel_framebuffer(fb)->tiling_mode !=
>> +		    work->old_tiling_mode)
>>   			/* vlv: DISPLAY_FLIP fails to change tiling */
>>   			ring = NULL;
>>   	} else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
>> @@ -12190,7 +12196,8 @@ intel_check_cursor_plane(struct drm_plane *plane,
>>
>>   	/* we only need to pin inside GTT if cursor is non-phy */
>>   	mutex_lock(&dev->struct_mutex);
>> -	if (!INTEL_INFO(dev)->cursor_needs_physical && obj->tiling_mode) {
>> +	if (!INTEL_INFO(dev)->cursor_needs_physical &&
>> +	    to_intel_framebuffer(fb)->tiling_mode) {
>>   		DRM_DEBUG_KMS("cursor cannot be tiled\n");
>>   		ret = -EINVAL;
>>   	}
>> @@ -12772,6 +12779,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;
>
> One side-effect of using fb->modifier[0] is that if the modifier flag is
> _not_ set, we need to reconstruct this field from obj->tiling_mode here.
> Otoh if it is set this code here should check that fb->modifier and
> obj->tiling_mode are consistent.
>
> Perhaps best to split this change out as a prep patch, like you've done
> with the code for the initial framebuffer.

If I understood correctly what you meant in the first quote then yes.

Regards,

Tvrtko