On Mon, 21 Oct 2019, Jani Nikula <[email protected]> wrote:
> On Mon, 21 Oct 2019, Jani Nikula <[email protected]> wrote:
>> On Fri, 18 Oct 2019, Janusz Krzysztofik <[email protected]>
>> 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.
And for crying out loud, merged without passing CI results.
>
> 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 <[email protected]>
>>> Signed-off-by: Janusz Krzysztofik <[email protected]>
>>> Cc: Michal Wajdeczko <[email protected]>
>>> ---
>>> 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
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
