[Intel-gfx] [RFC PATCH] drm/i915: Restore full symmetry in i915_driver_modeset_probe/remove

[Jani Nikula]

On Mon, 21 Oct 2019, Jani Nikula <jani.nikula at intel.com> wrote:
> On Fri, 18 Oct 2019, Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com> wrote:
>> Commit 2d6f6f359fd8 ("drm/i915: add i915_driver_modeset_remove()")
>
> Please look at that commit and explain to me how it impacts anything
> about intel_irq_uninstall().

Seriously, why was this merged already? It does not fix *anything*. It
cites a commit that does not break anything.

You're calling intel_irq_uninstall() *twice* for no good reason.

The whole probe/remove callchains are a mess, and I'm in the process of
cleaning it up properly. What good does this patch bring for the overall
effort, other than another useless quirk in the call chain to take into
account? Indeed, the only actual functional change it makes in the
remove path is not mentioned in the commit message at all!


BR,
Jani.


>
> BR,
> Jani.
>
>
>> 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 at intel.com>
>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> ---
>>  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);
>>  
>>  	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;

-- 
Jani Nikula, Intel Open Source Graphics Center