Re: [PATCH 0/2] Fix connector probing deadlocks from RPM bugs

[Daniel Vetter]

On Wed, Jul 18, 2018 at 04:56:38PM -0400, Lyude Paul wrote:
> This is a trimmed down version of
> 
> https://patchwork.freedesktop.org/series/46637/
> 
> with all of the review comments addressed.
> 
> The last version of this series had fixes for the i2c and DP aux busses
> to ensure that the GPU would be turned on whenever anything tried to
> access the i2c/aux busses. Unfortunately: one of the fixes apparently
> contained some very incorrect usage of Linux's runtime PM interface in
> order to prevent runtime suspend/resume requests coming from within
> nouveau's suspend/resume callbacks from deadlocking the system.
> 
> Turns out: fixing the i2c/dp aux problem is a lot harder then I
> originally anticipated. We either need to come up with helpers for DRM
> that work around the PM core, which is really not ideal, or come up with
> a new feature for the RPM core that allows us to inhibit suspend/resume
> callbacks. The former seems to be somewhat trivial to implement, but
> coming up with a decent interface for that is going to take a bit more
> time and I'd much rather have issues causing deadlocks get fixed first.
> This means that drm_dp_aux_dev is going to be broken on nouveau for
> laptops with hybrid GPUs using RPM, but it was already broken before
> anyhow.
> 
> So: this just contains the seriously important fixes that will stop
> people's machines from crashing very hard. Hopefully I can come up with
> a solution to the i2c/aux problem soon after fixing some other glaring
> MST bugs nouveau currently has.

Why do we need all this? i915 has full rpm support, including i2c and dp
aux correctly working, and everything else too. Why is nouveau special?

Also, there's a metric pile of other drivers using the existing helpers an
infrastructure, with full rpm support, all seemingly being happy too.

Given all that it smells a bit like nouveau has it's rpm design backwards,
which would indicate that these new helpers should be nouveau-specific
wrappers and not generic. If that's not the case, then I'd like to first
understand why nouveau needs them and why no one else seems to need these.
-Daniel

> 
> Lyude Paul (2):
>   drm/fb_helper: Add drm_fb_helper_output_poll_changed_with_rpm()
>   drm/probe_helper: Add
>     drm_helper_probe_single_connector_modes_with_rpm()
> 
>  drivers/gpu/drm/drm_fb_helper.c             | 23 +++++++++++++++
>  drivers/gpu/drm/drm_probe_helper.c          | 31 +++++++++++++++++++++
>  drivers/gpu/drm/nouveau/dispnv50/disp.c     |  4 +--
>  drivers/gpu/drm/nouveau/nouveau_connector.c |  4 +--
>  include/drm/drm_crtc_helper.h               |  7 +++--
>  include/drm/drm_fb_helper.h                 |  5 ++++
>  6 files changed, 67 insertions(+), 7 deletions(-)
> 
> -- 
> 2.17.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch