| Message ID | 20201127220548.3713-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Disable outputs during unregister | expand |
Quoting Chris Wilson (2020-11-27 22:05:48) > Switch off the scanout during driver unregister, so we can shutdown the > HW immediately for unbind. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Ping? Pretty vital for module unbind and unload. -Chris
On Fri, Nov 27, 2020 at 10:05:48PM +0000, Chris Wilson wrote: > Switch off the scanout during driver unregister, so we can shutdown the > HW immediately for unbind. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 320856b665a1..62d188e5cb8d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > * events. > */ > drm_kms_helper_poll_fini(&dev_priv->drm); > + drm_atomic_helper_shutdown(&dev_priv->drm); Looks like we already have this in remove(). Is that too late? > > intel_gt_driver_unregister(&dev_priv->gt); > acpi_video_unregister(); > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Ville Syrjälä (2020-12-01 16:05:17) > On Fri, Nov 27, 2020 at 10:05:48PM +0000, Chris Wilson wrote: > > Switch off the scanout during driver unregister, so we can shutdown the > > HW immediately for unbind. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 320856b665a1..62d188e5cb8d 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > > * events. > > */ > > drm_kms_helper_poll_fini(&dev_priv->drm); > > + drm_atomic_helper_shutdown(&dev_priv->drm); > > Looks like we already have this in remove(). Is that too late? For the operations we do during unbind, yes. For the core_hotplug/rebind dance, we have to reset the GPU while we still have runtime-pm operational and have pushed the reset to unregister (from experimentation that's as late as we can put it where the GPU works after rebinding and we don't corrupt the system on unbind, with the current hooks). You can guess how well gen3 likes that. But I don't think the right answer is to skip the reset for gen3. Suppose we enable context support for gen3, then the reset would be required as well, and so we would still need the whole display shenanigans to turn it off. Moving the modeset to turn the display off to the end of userspace seems reasonable. -Chris
On Tue, Dec 01, 2020 at 10:38:57PM +0000, Chris Wilson wrote: > Quoting Ville Syrjälä (2020-12-01 16:05:17) > > On Fri, Nov 27, 2020 at 10:05:48PM +0000, Chris Wilson wrote: > > > Switch off the scanout during driver unregister, so we can shutdown the > > > HW immediately for unbind. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > --- > > > drivers/gpu/drm/i915/i915_drv.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index 320856b665a1..62d188e5cb8d 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > > > * events. > > > */ > > > drm_kms_helper_poll_fini(&dev_priv->drm); > > > + drm_atomic_helper_shutdown(&dev_priv->drm); > > > > Looks like we already have this in remove(). Is that too late? > > For the operations we do during unbind, yes. > > For the core_hotplug/rebind dance, we have to reset the GPU while we > still have runtime-pm operational and have pushed the reset to > unregister (from experimentation that's as late as we can put it where > the GPU works after rebinding and we don't corrupt the system on unbind, > with the current hooks). You can guess how well gen3 likes that. > > But I don't think the right answer is to skip the reset for gen3. > Suppose we enable context support for gen3, then the reset would be > required as well, and so we would still need the whole display > shenanigans to turn it off. Moving the modeset to turn the display off > to the end of userspace seems reasonable. Yeah, just a bit odd to have the same call twice in the sequence. Can we remove the second call at least? Also a bit annoying the unload sequence no longer matches the suspend sequence. Well, I guess it was never 100% anyway but I think it was a bit closer before this patch. But the whole thing is rather messy anyway so I guess t's not significantly worse after this. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Quoting Ville Syrjälä (2020-12-04 16:01:11) > On Tue, Dec 01, 2020 at 10:38:57PM +0000, Chris Wilson wrote: > > Quoting Ville Syrjälä (2020-12-01 16:05:17) > > > On Fri, Nov 27, 2020 at 10:05:48PM +0000, Chris Wilson wrote: > > > > Switch off the scanout during driver unregister, so we can shutdown the > > > > HW immediately for unbind. > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > index 320856b665a1..62d188e5cb8d 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > > > > * events. > > > > */ > > > > drm_kms_helper_poll_fini(&dev_priv->drm); > > > > + drm_atomic_helper_shutdown(&dev_priv->drm); > > > > > > Looks like we already have this in remove(). Is that too late? > > > > For the operations we do during unbind, yes. > > > > For the core_hotplug/rebind dance, we have to reset the GPU while we > > still have runtime-pm operational and have pushed the reset to > > unregister (from experimentation that's as late as we can put it where > > the GPU works after rebinding and we don't corrupt the system on unbind, > > with the current hooks). You can guess how well gen3 likes that. > > > > But I don't think the right answer is to skip the reset for gen3. > > Suppose we enable context support for gen3, then the reset would be > > required as well, and so we would still need the whole display > > shenanigans to turn it off. Moving the modeset to turn the display off > > to the end of userspace seems reasonable. > > Yeah, just a bit odd to have the same call twice in the > sequence. Can we remove the second call at least? I think we can, but I am sufficiently paranoid to leave it. I presume if it is a no-op, it will return without touching HW? > Also a bit annoying the unload sequence no longer matches the > suspend sequence. Well, I guess it was never 100% anyway but > I think it was a bit closer before this patch. But the whole > thing is rather messy anyway so I guess t's not significantly > worse after this. Yes, I feel things have been thrown into a bit of disarray by haphazardly fixing unbind. The last* remaining fly in the ointment is rebinding iommu. Once we have that solid (and the system stops randomly eating itself 1-10 minutes after the test passes), we should be in a much better spot to safely remove duplication and refine the flow. * that I am aware of. -Chris
On Fri, Dec 04, 2020 at 04:14:05PM +0000, Chris Wilson wrote: > Quoting Ville Syrjälä (2020-12-04 16:01:11) > > On Tue, Dec 01, 2020 at 10:38:57PM +0000, Chris Wilson wrote: > > > Quoting Ville Syrjälä (2020-12-01 16:05:17) > > > > On Fri, Nov 27, 2020 at 10:05:48PM +0000, Chris Wilson wrote: > > > > > Switch off the scanout during driver unregister, so we can shutdown the > > > > > HW immediately for unbind. > > > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_drv.c | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > > index 320856b665a1..62d188e5cb8d 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > @@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) > > > > > * events. > > > > > */ > > > > > drm_kms_helper_poll_fini(&dev_priv->drm); > > > > > + drm_atomic_helper_shutdown(&dev_priv->drm); > > > > > > > > Looks like we already have this in remove(). Is that too late? > > > > > > For the operations we do during unbind, yes. > > > > > > For the core_hotplug/rebind dance, we have to reset the GPU while we > > > still have runtime-pm operational and have pushed the reset to > > > unregister (from experimentation that's as late as we can put it where > > > the GPU works after rebinding and we don't corrupt the system on unbind, > > > with the current hooks). You can guess how well gen3 likes that. > > > > > > But I don't think the right answer is to skip the reset for gen3. > > > Suppose we enable context support for gen3, then the reset would be > > > required as well, and so we would still need the whole display > > > shenanigans to turn it off. Moving the modeset to turn the display off > > > to the end of userspace seems reasonable. > > > > Yeah, just a bit odd to have the same call twice in the > > sequence. Can we remove the second call at least? > > I think we can, but I am sufficiently paranoid to leave it. > I presume if it is a no-op, it will return without touching HW? One can hope at least. > > > Also a bit annoying the unload sequence no longer matches the > > suspend sequence. Well, I guess it was never 100% anyway but > > I think it was a bit closer before this patch. But the whole > > thing is rather messy anyway so I guess t's not significantly > > worse after this. > > Yes, I feel things have been thrown into a bit of disarray by > haphazardly fixing unbind. > > The last* remaining fly in the ointment is rebinding iommu. Once we have > that solid (and the system stops randomly eating itself 1-10 minutes > after the test passes), we should be in a much better spot to safely > remove duplication and refine the flow. > > * that I am aware of. > -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 320856b665a1..62d188e5cb8d 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -738,6 +738,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) * events. */ drm_kms_helper_poll_fini(&dev_priv->drm); + drm_atomic_helper_shutdown(&dev_priv->drm); intel_gt_driver_unregister(&dev_priv->gt); acpi_video_unregister();
Switch off the scanout during driver unregister, so we can shutdown the HW immediately for unbind. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.c | 1 + 1 file changed, 1 insertion(+)