[Intel-gfx] [RFC][PATCH 11/11] drm: Sprinkle lockdep asserts for edid/display_info

[Harry Wentland]

On 2018-03-06 12:13 PM, Daniel Vetter wrote:
> On Tue, Mar 06, 2018 at 11:23:23AM -0500, Harry Wentland wrote:
>> On 2018-03-06 07:18 AM, Ville Syrjälä wrote:
>>> On Tue, Mar 06, 2018 at 10:31:27AM +0100, Daniel Vetter wrote:
>>>> On Tue, Feb 27, 2018 at 02:57:00PM +0200, Ville Syrjala wrote:
>>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>>
>>>>> edid and display_info are protected by mode_config.mutex. Add lockdep
>>>>> asserts to make sure we're not accessing things w/o the lock.
>>>>>
>>>>> FIXME: pretty sure this will blow up with amdgpu as they seem
>>>>> to be doing edid updates even from the modeset path. Need to figure
>>>>> out what to do about that. Maybe protect the edid/display info with
>>>>> with connection_mutex instead of mode_config.mutex?
>>>>
>>>> Imo not doing EDID udpates from the modeset path is the right fix. I can't
>>>> think of any reasonable reason to do that at least. Can you point me at
>>>> the relevant amdgpu code pls (I'm lazy, sry)?
>>>
>>> It was some MST thing I believe... (should have written it down)
>>>
>>> amdgpu_dm_atomic_check() -> dm_update_crtcs_state() ->
>>> create_stream_for_sink() -> dm_dp_mst_dc_sink_create() ->
>>> get_edid/update_edid_property
>>>
>>
>> Yeah, it's because the dc_sink carries the EDID and is only created at
>> this point for us. It's bugged me ever since we did this. Might be time
>> to think of a solution to it now.
> 
> But how does this work? Userspace won't do a modeset without the EDID
> present and the modes listed, which means you'll never ever end up in
> atomic check. This really sounds rather wrong.
> 

Not sure if this works correctly with atomic userspace.

I think with legacy userspace we might rely on the get_connector call doing .fill_modes > drm_helper_probe_single_connector_modes > .get_modes > dm_dp_mst_get_modes > drm_dp_mst_get_edid

Harry


> Only idea I can come up with is that you're abusing the regular pageflip
> request as a worker thread, but that's some seriously backwards design.
> -Daniel
> 
>>
>> Harry
>>
>>>>
>>>> Otherwise I think this is a real good patch.
>>>>
>>>> Thanks, Daniel
>>>>
>>>>>
>>>>> Cc: Keith Packard <keithp at keithp.com>
>>>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>>> Cc: Harry Wentland <harry.wentland at amd.com>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_connector.c    | 4 ++++
>>>>>  drivers/gpu/drm/drm_edid.c         | 2 ++
>>>>>  drivers/gpu/drm/drm_probe_helper.c | 2 +-
>>>>>  3 files changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>>>> index 122060792b6f..a9f3536f4e94 100644
>>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>>> @@ -1374,6 +1374,8 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>>>>>  	size_t size = 0;
>>>>>  	int ret;
>>>>>  
>>>>> +	lockdep_assert_held(&dev->mode_config.mutex);
>>>>> +
>>>>>  	/* ignore requests to set edid when overridden */
>>>>>  	if (connector->override_edid)
>>>>>  		return 0;
>>>>> @@ -1770,6 +1772,8 @@ void drm_connector_reset_display_info(struct drm_connector *connector)
>>>>>  {
>>>>>  	struct drm_display_info *info = &connector->display_info;
>>>>>  
>>>>> +	lockdep_assert_held(&connector->dev->mode_config.mutex);
>>>>> +
>>>>>  	memset(info, 0, sizeof(*info));
>>>>>  }
>>>>>  EXPORT_SYMBOL_GPL(drm_connector_reset_display_info);
>>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>>> index 618093c4a039..7f9e9236114b 100644
>>>>> --- a/drivers/gpu/drm/drm_edid.c
>>>>> +++ b/drivers/gpu/drm/drm_edid.c
>>>>> @@ -4440,6 +4440,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
>>>>>  	struct drm_display_info *info = &connector->display_info;
>>>>>  	u32 quirks = edid_get_quirks(edid);
>>>>>  
>>>>> +	lockdep_assert_held(&connector->dev->mode_config.mutex);
>>>>> +
>>>>>  	info->width_mm = edid->width_cm * 10;
>>>>>  	info->height_mm = edid->height_cm * 10;
>>>>>  
>>>>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>>>>> index 7dc7e635d7e4..2a2afcf72788 100644
>>>>> --- a/drivers/gpu/drm/drm_probe_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>>>>> @@ -400,7 +400,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>>>>  	enum drm_connector_status old_status;
>>>>>  	struct drm_modeset_acquire_ctx ctx;
>>>>>  
>>>>> -	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
>>>>> +	lockdep_assert_held(&dev->mode_config.mutex);
>>>>>  
>>>>>  	drm_modeset_acquire_init(&ctx, 0);
>>>>>  
>>>>> -- 
>>>>> 2.13.6
>>>>>
>>>>
>>>> -- 
>>>> Daniel Vetter
>>>> Software Engineer, Intel Corporation
>>>> http://blog.ffwll.ch
>>>
>