On Tue, Mar 06, 2018 at 01:32:21PM -0500, Harry Wentland wrote:
>
>
> 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
Atomic userspace users the exact same ioctls for connector probing, no
change there.
That leaves me wondering why exactly you're doing this in atomic_check.
Just nuke it?
-Daniel
>
> 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
> >>>
> >
--
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
