[Intel-gfx] [PATCH] drm/i915/fbdev: Call intel_unpin_fb_obj() on release

[Tvrtko Ursulin]

On 22/04/16 10:00, Chris Wilson wrote:
> On Fri, Apr 22, 2016 at 09:47:47AM +0100, Tvrtko Ursulin wrote:
>>
>> On 22/04/16 08:47, Chris Wilson wrote:
>>> When releasing the intel_fbdev, we should unpin the framebuffer that we
>>> pinned during construction.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> ---
>>> I'm hoping this explains some of the flip-flip in DMESG-WARN for
>>> drv_module_reload_basic...
>>> -Chris
>>> ---
>>>   drivers/gpu/drm/i915/intel_display.c | 2 +-
>>>   drivers/gpu/drm/i915/intel_drv.h     | 1 +
>>>   drivers/gpu/drm/i915/intel_fbdev.c   | 7 ++++++-
>>>   3 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index ff60241b1f76..a59fe7b032b0 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -2309,7 +2309,7 @@ err_pm:
>>>   	return ret;
>>>   }
>>>
>>> -static void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
>>> +void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
>>>   {
>>>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>>   	struct i915_ggtt_view view;
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index beed9e81252b..e336b19adb37 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1161,6 +1161,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>>>   				    struct drm_modeset_acquire_ctx *ctx);
>>>   int intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb,
>>>   			       unsigned int rotation);
>>> +void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation);
>>>   struct drm_framebuffer *
>>>   __intel_framebuffer_create(struct drm_device *dev,
>>>   			   struct drm_mode_fb_cmd2 *mode_cmd,
>>> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
>>> index 79ac202f3870..c3c265d6f521 100644
>>> --- a/drivers/gpu/drm/i915/intel_fbdev.c
>>> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
>>> @@ -287,7 +287,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>>>   out_destroy_fbi:
>>>   	drm_fb_helper_release_fbi(helper);
>>>   out_unpin:
>>> -	i915_gem_object_ggtt_unpin(obj);
>>> +	intel_unpin_fb_obj(&ifbdev->fb->base, BIT(DRM_ROTATE_0));
>>>   out_unlock:
>>>   	mutex_unlock(&dev->struct_mutex);
>>>   	return ret;
>>> @@ -551,6 +551,11 @@ static void intel_fbdev_destroy(struct drm_device *dev,
>>>
>>>   	if (ifbdev->fb) {
>>>   		drm_framebuffer_unregister_private(&ifbdev->fb->base);
>>> +
>>> +		mutex_lock(&dev->struct_mutex);
>>> +		intel_unpin_fb_obj(&ifbdev->fb->base, BIT(DRM_ROTATE_0));
>>> +		mutex_unlock(&dev->struct_mutex);
>>> +
>>>   		drm_framebuffer_remove(&ifbdev->fb->base);
>>>   	}
>>>   }
>>>
>>
>> What is new to need this? I looked at the related code, and maybe
>> got the wrong conclusions, but it looks like that warning in
>> drm_mm_takedown would fire all the time if this
>> drm_framebuffer_remove was not the last reference and wouldn't do
>> the unpinning?
>
> I've got an extra warning in my tree for the untracked vma (which fires
> ofc). However, it doesn't fix the issue of !fbdev leaking memory. So,
> yes, it is just a warning fix from later that I'm optimistically
> applying now.
>
> Something about CI results being hidden and not seeing the same failures
> locally...

Didn't really understand any of that. Is think blocking the premature 
unpin series? Is the warning only appearing with that series or is it 
something already present?

Regards,

Tvrtko