[Intel-gfx] [PATCH] drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes.

[Daniel Vetter]

On Wed, Mar 29, 2017 at 03:51:08PM +0200, Maarten Lankhorst wrote:
> Op 29-03-17 om 15:31 schreef Boris Brezillon:
> > On Wed, 29 Mar 2017 15:26:45 +0200
> > Daniel Vetter <daniel at ffwll.ch> wrote:
> >
> >> On Wed, Mar 29, 2017 at 12:16:50PM +0200, Maarten Lankhorst wrote:
> >>> mode_valid() and get_modes() called
> >>> from drm_helper_probe_single_connector_modes()
> >>> may need to look at connector->state because what a valid mode is may
> >>> depend on connector properties being set. For example some HDMI modes
> >>> might be rejected when a connector property forces the connector
> >>> into DVI mode.
> >>>
> >>> Some implementations of detect() already lock all state,
> >>> so we have to pass an acquire_ctx to them to prevent a deadlock.
> >>>
> >>> This means changing the function signature of detect() slightly,
> >>> and passing the acquire_ctx for locking multiple crtc's.
> >>> It might be NULL, in which case expensive operations should be avoided.
> >>>
> >>> intel_dp.c however ignores the force flag, so still lock
> >>> connection_mutex there if needed.
> >>>
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>> Cc: Boris Brezillon <boris.brezillon at free-electrons.com>
> >>> Cc: Manasi Navare <manasi.d.navare at intel.com>  
> >> Hm only noticed this now, but mixing up force with the acquire_ctx sounds
> >> like very bad interface design. Yes, the only user of the new hook works
> >> like that, but that's really accidental I think. I think just having the
> >> ctx everywhere (for atomic drivers at least) would be a lot safer. This is
> >> extremely surprising (and undocumented suprise at that).
> > Yes, I was about to say the same thing: the interface is not very
> > clear, and I don't understand why ctx = NULL implies force = false.
> 
> They're the same thing I fear. I could perhaps call it force_ctx instead,
> but non-zero ctx implies force, and other way around. Though I guess we could
> relax it, and have force = true imply ctx, but not the other way around.
> 
> Would that be ok?

Why can't we supply a ctx always? I didn't see any reason not to in the
code ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch