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

[Harry Wentland]

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.

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
>