Re: [RFC][PATCH 11/11] drm: Sprinkle lockdep asserts for edid/display_info
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??
> >
> > 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
> > Cc: Daniel Vetter
> > Cc: Harry Wentland
> > Signed-off-by: Ville Syrj??l??
> > ---
> > 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_m
Re: [RFC][PATCH 11/11] drm: Sprinkle lockdep asserts for edid/display_info
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ä
>
> 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
> Cc: Daniel Vetter
> Cc: Harry Wentland
> Signed-off-by: Ville Syrjälä
> ---
> 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
>>>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC][PATCH 11/11] drm: Sprinkle lockdep asserts for edid/display_info
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ä
> >>>
> >>> 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.
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
> >>> Cc: Daniel Vetter
> >>> Cc: Harry Wentland
> >>> Signed-off-by: Ville Syrjälä
> >>> ---
> >>> 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC][PATCH 11/11] drm: Sprinkle lockdep asserts for edid/display_info
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ä
>>>
>>> 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
>>> Cc: Daniel Vetter
>>> Cc: Harry Wentland
>>> Signed-off-by: Ville Syrjälä
>>> ---
>>> 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
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC][PATCH 11/11] drm: Sprinkle lockdep asserts for edid/display_info
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ä
> >
> > 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
>
> Otherwise I think this is a real good patch.
>
> Thanks, Daniel
>
> >
> > Cc: Keith Packard
> > Cc: Daniel Vetter
> > Cc: Harry Wentland
> > Signed-off-by: Ville Syrjälä
> > ---
> > 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
--
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC][PATCH 11/11] drm: Sprinkle lockdep asserts for edid/display_info
On Tue, Feb 27, 2018 at 02:57:00PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 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)?
Otherwise I think this is a real good patch.
Thanks, Daniel
>
> Cc: Keith Packard
> Cc: Daniel Vetter
> Cc: Harry Wentland
> Signed-off-by: Ville Syrjälä
> ---
> 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC][PATCH 11/11] drm: Sprinkle lockdep asserts for edid/display_info
From: Ville Syrjälä
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?
Cc: Keith Packard
Cc: Daniel Vetter
Cc: Harry Wentland
Signed-off-by: Ville Syrjälä
---
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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
