Re: [RFC PATCH] drm/i915: Restore full symmetry in i915_driver_modeset_probe/remove

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Fri, 18 Oct 2019 12:07:10 +0200, Janusz Krzysztofik  
<janusz.krzysztofik@linux.intel.com> wrote:

> Commit 2d6f6f359fd8 ("drm/i915: add i915_driver_modeset_remove()")
> claimed removal of asymmetry in probe() and remove() calls, however, it
> didn't take care of calling intel_irq_uninstall() on driver remove.
> That doesn't hurt as long as we still call it from
> intel_modeset_driver_remove() but in order to have full symmetry we
> should call it again from i915_driver_modeset_remove().
>
> Note that it's safe to call intel_irq_uninstall() twice thanks to
> commit b318b82455bd ("drm/i915: Nuke drm_driver irq vfuncs").  We may
> only want to mention the case we are adding in a related FIXME comment
> provided by that commit.  While being at it, update the name of
> function mentioned as calling it out of sequence as that name has been
> changed meanwhile by commit 78dae1ac35dd ("drm/i915: Propagate
> "_remove" function name suffix down").
>
> Suggested-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
but please see below

>  drivers/gpu/drm/i915/i915_drv.c | 2 ++
>  drivers/gpu/drm/i915/i915_irq.c | 8 ++++----
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c  
> b/drivers/gpu/drm/i915/i915_drv.c
> index dd9613e45723..4ae9bfa96290 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -452,6 +452,8 @@ static void i915_driver_modeset_remove(struct  
> drm_i915_private *i915)
> 	intel_modeset_driver_remove(i915);
> +	intel_irq_uninstall(i915);
> +
>  	intel_bios_driver_remove(i915);

I'm wondering if this is a good location for this call as in
i915_driver_modeset_probe() we call it before intel_vga_register()
so likely cleanup should be done after intel_vga_unregister()

it also looks that we missed to call intel_bios_driver_remove()
on the error path in i915_driver_modeset_probe()

but let's fix it in a separate patch

> 	vga_switcheroo_unregister_client(pdev);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c  
> b/drivers/gpu/drm/i915/i915_irq.c
> index bc83f094065a..0160283860a6 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4515,10 +4515,10 @@ void intel_irq_uninstall(struct drm_i915_private  
> *dev_priv)
>  	int irq = dev_priv->drm.pdev->irq;
> 	/*
> -	 * FIXME we can get called twice during driver load
> -	 * error handling due to intel_modeset_cleanup()
> -	 * calling us out of sequence. Would be nice if
> -	 * it didn't do that...
> +	 * FIXME we can get called twice during driver probe
> +	 * error handling as well as during driver remove due to
> +	 * intel_modeset_driver_remove() calling us out of sequence.
> +	 * Would be nice if it didn't do that...
>  	 */
>  	if (!dev_priv->drm.irq_enabled)
>  		return;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx