Re: [PATCH 3/5] drm/i915: Don't try to reference the fb in get_initial_plane_config()

From: Matt Roper <matthew.d.roper@intel.com>
To: Damien Lespiau <damien.lespiau@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/5] drm/i915: Don't try to reference the fb in get_initial_plane_config()
Date: Thu, 5 Feb 2015 10:24:02 -0800	[thread overview]
Message-ID: <20150205182401.GC25532@intel.com> (raw)
In-Reply-To: <1423156939-15705-4-git-send-email-damien.lespiau@intel.com>

On Thu, Feb 05, 2015 at 05:22:17PM +0000, Damien Lespiau wrote:
> Tvrtko noticed a new warning on boot:
> 
>   WARNING: CPU: 1 PID: 353 at include/linux/kref.h:47 drm_framebuffer_reference+0x6c/0x80 [drm]()
>   Call Trace:
>   [<ffffffff8161f10c>] dump_stack+0x4f/0x7b
>   [<ffffffff81052caa>] warn_slowpath_common+0xaa/0xd0
>   [<ffffffff81052d8a>] warn_slowpath_null+0x1a/0x20
>   [<ffffffffa00d035c>] drm_framebuffer_reference+0x6c/0x80 [drm]
>   [<ffffffffa01c0df7>] update_state_fb.isra.54+0x47/0x50 [i915]
>   [<ffffffffa01ccd5c>] skylake_get_initial_plane_config+0x93c/0x950 [i915]
>   [<ffffffffa01e8721>] intel_modeset_init+0x1551/0x17c0 [i915]
>   [<ffffffffa02476e0>] i915_driver_load+0xed0/0x11e0 [i915]
>   [<ffffffff81627aa1>] ? _raw_spin_unlock_irqrestore+0x51/0x70
>   [<ffffffffa00ca8b7>] drm_dev_register+0x77/0x110 [drm]
>   [<ffffffffa00cda3b>] drm_get_pci_dev+0x11b/0x1f0 [drm]
>   [<ffffffff81098e3d>] ? trace_hardirqs_on+0xd/0x10
>   [<ffffffff81627aa1>] ? _raw_spin_unlock_irqrestore+0x51/0x70
>   [<ffffffffa0145276>] i915_pci_probe+0x56/0x60 [i915]
>   [<ffffffff813ad59c>] pci_device_probe+0x7c/0x100
>   [<ffffffff81466aad>] driver_probe_device+0x16d/0x380
> 
> We cannot take a reference at this point, not before
> intel_framebuffer_init() and the underlying drm_framebuffer_init().
> 
> Introduced in:
> 
>   commit 706dc7b549175e47f23e913b7f1e52874a7d0f56
>   Author: Matt Roper <matthew.d.roper@intel.com>
>   Date:   Tue Feb 3 13:10:04 2015 -0800
> 
>       drm/i915: Ensure plane->state->fb stays in sync with plane->fb
> 
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3301b61..e1e89d0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2364,6 +2364,20 @@ static int skl_format_to_fourcc(int format, bool rgb_order, bool alpha)
>  	}
>  }
>  
> +/* Update plane->state->fb to match plane->fb after driver-internal updates */
> +static void
> +update_state_fb(struct drm_plane *plane)
> +{
> +	if (plane->fb == plane->state->fb)
> +		return;
> +
> +	if (plane->state->fb)
> +		drm_framebuffer_unreference(plane->state->fb);
> +	plane->state->fb = plane->fb;
> +	if (plane->state->fb)
> +		drm_framebuffer_reference(plane->state->fb);
> +}
> +

Was moving this function without changes intentional?  It seems to be
unrelated to the other changes here.

Anyway, #1-4 are

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

I left a couple comments on #5.

Matt

>  static bool
>  intel_alloc_plane_obj(struct intel_crtc *crtc,
>  		      struct intel_initial_plane_config *plane_config)
> @@ -2411,20 +2425,6 @@ out_unref_obj:
>  	return false;
>  }
>  
> -/* Update plane->state->fb to match plane->fb after driver-internal updates */
> -static void
> -update_state_fb(struct drm_plane *plane)
> -{
> -	if (plane->fb == plane->state->fb)
> -		return;
> -
> -	if (plane->state->fb)
> -		drm_framebuffer_unreference(plane->state->fb);
> -	plane->state->fb = plane->fb;
> -	if (plane->state->fb)
> -		drm_framebuffer_reference(plane->state->fb);
> -}
> -
>  static void
>  intel_find_plane_obj(struct intel_crtc *intel_crtc,
>  		     struct intel_initial_plane_config *plane_config)
> @@ -2438,8 +2438,10 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
>  	if (!intel_crtc->base.primary->fb)
>  		return;
>  
> -	if (intel_alloc_plane_obj(intel_crtc, plane_config))
> +	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
> +		update_state_fb(intel_crtc->base.primary);
>  		return;
> +	}
>  
>  	kfree(intel_crtc->base.primary->fb);
>  	intel_crtc->base.primary->fb = NULL;
> @@ -6652,7 +6654,6 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
>  		      plane_config->size);
>  
>  	crtc->base.primary->fb = fb;
> -	update_state_fb(crtc->base.primary);
>  }
>  
>  static void chv_crtc_clock_get(struct intel_crtc *crtc,
> @@ -7690,7 +7691,6 @@ skylake_get_initial_plane_config(struct intel_crtc *crtc,
>  		      plane_config->size);
>  
>  	crtc->base.primary->fb = fb;
> -	update_state_fb(crtc->base.primary);
>  	return;
>  
>  error:
> @@ -7782,7 +7782,6 @@ ironlake_get_initial_plane_config(struct intel_crtc *crtc,
>  		      plane_config->size);
>  
>  	crtc->base.primary->fb = fb;
> -	update_state_fb(crtc->base.primary);
>  }
>  
>  static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
> -- 
> 1.8.3.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx