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@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
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@keithp.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Harry Wentland harry.wentland@amd.com Signed-off-by: Ville Syrj??l?? ville.syrjala@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