drm/i915/fbdev: Call intel_unpin_fb_obj() on release
diff mbox

Message ID 1461311222-32406-1-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson April 22, 2016, 7:47 a.m. UTC
When releasing the intel_fbdev, we should unpin the framebuffer that we
pinned during construction.

Signed-off-by: Chris Wilson <chris@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(-)

Comments

Tvrtko Ursulin April 22, 2016, 8:47 a.m. UTC | #1
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@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?

Regards,

Tvrtko
Chris Wilson April 22, 2016, 9 a.m. UTC | #2
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@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...
-Chris
Tvrtko Ursulin April 22, 2016, 11:59 a.m. UTC | #3
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@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

Patch
diff mbox

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);
 	}
 }