drm/i915: Drop fb reference on load_detect_pipe failure path
diff mbox

Message ID 20171207220025.22698-1-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson Dec. 7, 2017, 10 p.m. UTC
When intel_modeset_setup_plane_state() fails drop the local framebuffer
reference before jumping to the error, otherwise we leak the framebuffer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Chris Wilson Dec. 7, 2017, 10:06 p.m. UTC | #1
Quoting Chris Wilson (2017-12-07 22:00:25)
> When intel_modeset_setup_plane_state() fails drop the local framebuffer
> reference before jumping to the error, otherwise we leak the framebuffer.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

I believe,
Fixes: edde361711ef ("drm/i915: Use atomic state to obtain load detection crtc, v3.")
-Chris
Chris Wilson Dec. 7, 2017, 10:26 p.m. UTC | #2
Quoting Patchwork (2017-12-07 22:20:31)
> == Series Details ==
> 
> Series: drm/i915: Drop fb reference on load_detect_pipe failure path
> URL   : https://patchwork.freedesktop.org/series/35057/
> State : success
> 
> == Summary ==
> 
> Series 35057v1 drm/i915: Drop fb reference on load_detect_pipe failure path
> https://patchwork.freedesktop.org/api/1.0/series/35057/revisions/1/mbox/
> 
> Test debugfs_test:
>         Subgroup read_all_entries:
>                 dmesg-fail -> DMESG-WARN (fi-elk-e7500) fdo#103989
> Test kms_frontbuffer_tracking:
>         Subgroup basic:
>                 pass       -> FAIL       (fi-glk-1) fdo#103167

No impact on the gdg fb leak. Maybe better luck for the others?
-Chris
Ville Syrjälä Dec. 8, 2017, 11:14 a.m. UTC | #3
On Thu, Dec 07, 2017 at 10:00:25PM +0000, Chris Wilson wrote:
> When intel_modeset_setup_plane_state() fails drop the local framebuffer
> reference before jumping to the error, otherwise we leak the framebuffer.

Or we could just
https://patchwork.freedesktop.org/patch/165486/

This is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
nonetheless.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1f7e312d0d0d..1ba570614dbe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9940,11 +9940,10 @@ int intel_get_load_detect_pipe(struct drm_connector *connector,
>  	}
>  
>  	ret = intel_modeset_setup_plane_state(state, crtc, mode, fb, 0, 0);
> +	drm_framebuffer_put(fb);
>  	if (ret)
>  		goto fail;
>  
> -	drm_framebuffer_put(fb);
> -
>  	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
>  	if (ret)
>  		goto fail;
> -- 
> 2.15.1
Chris Wilson Dec. 8, 2017, 11:31 a.m. UTC | #4
Quoting Ville Syrjälä (2017-12-08 11:14:17)
> On Thu, Dec 07, 2017 at 10:00:25PM +0000, Chris Wilson wrote:
> > When intel_modeset_setup_plane_state() fails drop the local framebuffer
> > reference before jumping to the error, otherwise we leak the framebuffer.
> 
> Or we could just
> https://patchwork.freedesktop.org/patch/165486/

Why not both! :)
 
> This is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> nonetheless.

Ta, less controversial patch for more, grand improvement for you :)
-Chris
Chris Wilson Dec. 8, 2017, 11:49 a.m. UTC | #5
Quoting Chris Wilson (2017-12-08 11:31:01)
> Quoting Ville Syrjälä (2017-12-08 11:14:17)
> > On Thu, Dec 07, 2017 at 10:00:25PM +0000, Chris Wilson wrote:
> > > When intel_modeset_setup_plane_state() fails drop the local framebuffer
> > > reference before jumping to the error, otherwise we leak the framebuffer.
> > 
> > Or we could just
> > https://patchwork.freedesktop.org/patch/165486/
> 
> Why not both! :)
>  
> > This is
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > nonetheless.
> 
> Ta, less controversial patch for more, grand improvement for you :)

And pushed, thanks for the review. Let's have a gander at the more
intelligent load-detect patch. :)
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1f7e312d0d0d..1ba570614dbe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9940,11 +9940,10 @@  int intel_get_load_detect_pipe(struct drm_connector *connector,
 	}
 
 	ret = intel_modeset_setup_plane_state(state, crtc, mode, fb, 0, 0);
+	drm_framebuffer_put(fb);
 	if (ret)
 		goto fail;
 
-	drm_framebuffer_put(fb);
-
 	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
 	if (ret)
 		goto fail;