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@xxxxxxxxxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxxx>
>>>>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>>>>> Cc: Harry Wentland <harry.wentland@xxxxxxx>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>> 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
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
