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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 29-03-17 om 16:06 schreef 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@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@linux.intel.com>
>>>>> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
>>>>> Cc: Manasi Navare <manasi.d.navare@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

Then drm_helper_hpd_irq_event and intel_hpd_irq_event have to do -EDEADLK handling too,
it could be done but nothing would return -EDEADLK so it's unused code.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel