On Tue, Feb 25, 2020 at 1:32 PM Alex Deucher <alexdeuc...@gmail.com> wrote: > > On Tue, Feb 25, 2020 at 1:30 PM Zuo, Jerry <jerry....@amd.com> wrote: > > > > [AMD Official Use Only - Internal Distribution Only] > > > > Hi Alex: > > > > It happened when a MST monitor is attached, either in driver load or > > hotplug later. > > I think I found the issue. I'll send a patch shortly.
Attaching two patches. I think patch 1 should fix it. Patch 2 is the same patch as before. I'm not sure drm_dp_mst_connector_late_register() is necessary since no other driver calls it. Alex > > Alex > > > > > > Regards, > > Jerry > > > > -----Original Message----- > > From: Alex Deucher <alexdeuc...@gmail.com> > > Sent: February 25, 2020 1:23 PM > > To: Liu, Zhan <zhan....@amd.com> > > Cc: Wentland, Harry <harry.wentl...@amd.com>; Zuo, Jerry > > <jerry....@amd.com>; amd-gfx list <amd-...@lists.freedesktop.org>; Maling > > list - DRI developers <dri-devel@lists.freedesktop.org>; Deucher, Alexander > > <alexander.deuc...@amd.com>; Broadworth, Mark <mark.broadwo...@amd.com> > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector > > registration (v3) > > > > On Tue, Feb 25, 2020 at 1:20 PM Liu, Zhan <zhan....@amd.com> wrote: > > > > > > > > > > -----Original Message----- > > > > From: Liu, Zhan > > > > Sent: 2020/February/25, Tuesday 10:10 AM > > > > To: Alex Deucher <alexdeuc...@gmail.com>; Wentland, Harry > > > > <harry.wentl...@amd.com> > > > > Cc: amd-gfx list <amd-...@lists.freedesktop.org>; Maling list - DRI > > > > developers <dri-devel@lists.freedesktop.org>; Deucher, Alexander > > > > <alexander.deuc...@amd.com>; Broadworth, Mark > > > > <mark.broadwo...@amd.com> > > > > Subject: RE: [PATCH 13/15] drm/amdgpu/display: split dp connector > > > > registration (v3) > > > > > > > > > > > > > -----Original Message----- > > > > > From: Alex Deucher <alexdeuc...@gmail.com> > > > > > Sent: 2020/February/25, Tuesday 9:07 AM > > > > > To: Wentland, Harry <harry.wentl...@amd.com> > > > > > Cc: amd-gfx list <amd-...@lists.freedesktop.org>; Maling list - > > > > > DRI developers <dri-devel@lists.freedesktop.org>; Deucher, > > > > > Alexander <alexander.deuc...@amd.com>; Broadworth, Mark > > > > > <mark.broadwo...@amd.com>; Liu, Zhan <zhan....@amd.com> > > > > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector > > > > > registration (v3) > > > > > > > > > > On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland <hwent...@amd.com> > > > > > wrote: > > > > > > > > > > > > On 2020-02-07 4:17 p.m., Alex Deucher wrote: > > > > > > > Split into init and register functions to avoid a segfault in > > > > > > > some configs when the load/unload callbacks are removed. > > > > > > > > > > > > > > > > > > > Looks like MST is completely broken with this change with a NULL > > > > > > pointer dereference in drm_dp_aux_register. > > > > > > > > > > > > > v2: > > > > > > > - add back accidently dropped has_aux setting > > > > > > > - set dev in late_register > > > > > > > > > > > > > > v3: > > > > > > > - fix dp cec ordering > > > > > > > > > > > > > > Signed-off-by: Alex Deucher <alexander.deuc...@amd.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 16 > > > > > ++++++++++++++++ > > > > > > > drivers/gpu/drm/amd/amdgpu/atombios_dp.c | 10 ++-------- > > > > > > > .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 7 ++++++- > > > > > > > 3 files changed, 24 insertions(+), 9 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > > > > > > > index ec1501e3a63a..f355d9a752d2 100644 > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > > > > > > > @@ -1461,6 +1461,20 @@ static enum drm_mode_status > > > > > amdgpu_connector_dp_mode_valid(struct drm_connector > > > > > > > return MODE_OK; > > > > > > > } > > > > > > > > > > > > > > +static int > > > > > > > +amdgpu_connector_late_register(struct drm_connector *connector) { > > > > > > > + struct amdgpu_connector *amdgpu_connector = > > > > > to_amdgpu_connector(connector); > > > > > > > + int r = 0; > > > > > > > + > > > > > > > + if (amdgpu_connector->ddc_bus->has_aux) { > > > > > > > + amdgpu_connector->ddc_bus->aux.dev = > > > > > > > + amdgpu_connector- > > > > > >base.kdev; > > > > > > > + r = > > > > > > > drm_dp_aux_register(&amdgpu_connector->ddc_bus->aux); > > > > > > > + } > > > > > > > + > > > > > > > + return r; > > > > > > > +} > > > > > > > + > > > > > > > static const struct drm_connector_helper_funcs > > > > > amdgpu_connector_dp_helper_funcs = { > > > > > > > .get_modes = amdgpu_connector_dp_get_modes, > > > > > > > .mode_valid = amdgpu_connector_dp_mode_valid, @@ -1475,6 > > > > > > > +1489,7 @@ static const struct drm_connector_funcs > > > > > amdgpu_connector_dp_funcs = { > > > > > > > .early_unregister = amdgpu_connector_unregister, > > > > > > > .destroy = amdgpu_connector_destroy, > > > > > > > .force = amdgpu_connector_dvi_force, > > > > > > > + .late_register = amdgpu_connector_late_register, > > > > > > > }; > > > > > > > > > > > > > > static const struct drm_connector_funcs > > > > > > > amdgpu_connector_edp_funcs = { @@ -1485,6 +1500,7 @@ static > > > > const > > > > > > > struct drm_connector_funcs > > > > > amdgpu_connector_edp_funcs = { > > > > > > > .early_unregister = amdgpu_connector_unregister, > > > > > > > .destroy = amdgpu_connector_destroy, > > > > > > > .force = amdgpu_connector_dvi_force, > > > > > > > + .late_register = amdgpu_connector_late_register, > > > > > > > }; > > > > > > > > > > > > > > void > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c > > > > > > > b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c > > > > > > > index ea702a64f807..9b74cfdba7b8 100644 > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c > > > > > > > @@ -186,16 +186,10 @@ amdgpu_atombios_dp_aux_transfer(struct > > > > > > > drm_dp_aux *aux, struct drm_dp_aux_msg *m > > > > > > > > > > > > > > void amdgpu_atombios_dp_aux_init(struct amdgpu_connector > > > > > > > *amdgpu_connector) { > > > > > > > - int ret; > > > > > > > - > > > > > > > amdgpu_connector->ddc_bus->rec.hpd = amdgpu_connector- > > > > > >hpd.hpd; > > > > > > > - amdgpu_connector->ddc_bus->aux.dev = amdgpu_connector- > > > > > >base.kdev; > > > > > > > amdgpu_connector->ddc_bus->aux.transfer = > > > > > amdgpu_atombios_dp_aux_transfer; > > > > > > > - ret = drm_dp_aux_register(&amdgpu_connector->ddc_bus->aux); > > > > > > > - if (!ret) > > > > > > > - amdgpu_connector->ddc_bus->has_aux = true; > > > > > > > - > > > > > > > - WARN(ret, "drm_dp_aux_register_i2c_bus() failed with error > > > > > > > %d\n", > > > > > ret); > > > > > > > + drm_dp_aux_init(&amdgpu_connector->ddc_bus->aux); > > > > > > > + amdgpu_connector->ddc_bus->has_aux = true; > > > > > > > } > > > > > > > > > > > > > > /***** general DP utility functions *****/ diff --git > > > > > > > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > > > > > > index 3959c942c88b..d5b9e72f2649 100644 > > > > > > > --- > > > > > a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > > > > > > +++ > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > > > > > > > @@ -155,6 +155,11 @@ > > > > > amdgpu_dm_mst_connector_late_register(struct drm_connector > > > > > *connector) > > > > > > > struct amdgpu_dm_connector *amdgpu_dm_connector = > > > > > > > to_amdgpu_dm_connector(connector); > > > > > > > struct drm_dp_mst_port *port = > > > > > > > amdgpu_dm_connector->port; > > > > > > > + int r; > > > > > > > + > > > > > > > + r = > > > > > > > + drm_dp_aux_register(&amdgpu_dm_connector->dm_dp_aux.aux); > > > > > > > > > > > > This calls drm_dp_aux_register_devnode which is also called > > > > > > later in drm_dp_mst_connector_late_register. Wonder if that's a > > > > > > problem. > > > > > > > > > > Does this patch help? I'm not too familiar with the MST code and > > > > > I don't have an MST monitor. > > > > > > > > I have an MST monitor and I can give it a spin. I'll get back to you > > > > later. > > > > > > + Jerry who is following up on this issue > > > > > > Thank you Alex for your patch. Unfortunately, it doesn't solve the issue. > > > > > > Jerry is following up on this ticket, and I've added him to this email > > > thread. > > > > Can you send me the full dmesg output? Also, does it happen as soon as the > > driver loads or sometime later? > > > > Alex > > > > > > > > > > Zhan > > > > > > > > > > > Zhan > > > > > > > > > > > > > > Alex > > > > > > > > > > > > > > > > > > > > > > Harry > > > > > > > > > > > > > + if (r) > > > > > > > + return r; > > > > > > > > > > > > > > #if defined(CONFIG_DEBUG_FS) > > > > > > > connector_debugfs_init(amdgpu_dm_connector); > > > > > > > @@ -484,7 +489,7 @@ void > > > > amdgpu_dm_initialize_dp_connector(struct > > > > > amdgpu_display_manager *dm, > > > > > > > aconnector->dm_dp_aux.aux.transfer = dm_dp_aux_transfer; > > > > > > > aconnector->dm_dp_aux.ddc_service = > > > > > > > aconnector->dc_link->ddc; > > > > > > > > > > > > > > - drm_dp_aux_register(&aconnector->dm_dp_aux.aux); > > > > > > > + drm_dp_aux_init(&aconnector->dm_dp_aux.aux); > > > > > > > drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux, > > > > > > > &aconnector->base); > > > > > > > > > > > > > >
From ccf3ad1f1afcc3d32399bdc584b619199582a84e Mon Sep 17 00:00:00 2001 From: Alex Deucher <alexander.deuc...@amd.com> Date: Tue, 25 Feb 2020 13:39:44 -0500 Subject: [PATCH 2/2] drm/amdgpu/display: don't call drm_dp_mst_connector_late_register (v2) Nothing else calls it. Not sure it's necessary. v2: remove unused port variable. Signed-off-by: Alex Deucher <alexander.deuc...@amd.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index d7a7dc0c256e..0ef0eeb16778 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c @@ -154,7 +154,6 @@ amdgpu_dm_mst_connector_late_register(struct drm_connector *connector) { struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector); - struct drm_dp_mst_port *port = amdgpu_dm_connector->port; int r; amdgpu_dm_connector->dm_dp_aux.aux.dev = connector->kdev; @@ -166,7 +165,7 @@ amdgpu_dm_mst_connector_late_register(struct drm_connector *connector) connector_debugfs_init(amdgpu_dm_connector); #endif - return drm_dp_mst_connector_late_register(connector, port); + return r; } static void -- 2.24.1
From 4eee6f2de66c885199084113e2e60c6a28859e37 Mon Sep 17 00:00:00 2001 From: Alex Deucher <alexander.deuc...@amd.com> Date: Tue, 25 Feb 2020 13:36:26 -0500 Subject: [PATCH 1/2] drm/amdgpu/display: move the setting of the aux dev We need to move this into late init to avoid it being NULL once we switch the init order. Signed-off-by: Alex Deucher <alexander.deuc...@amd.com> --- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index d5b9e72f2649..d7a7dc0c256e 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c @@ -157,6 +157,7 @@ amdgpu_dm_mst_connector_late_register(struct drm_connector *connector) struct drm_dp_mst_port *port = amdgpu_dm_connector->port; int r; + amdgpu_dm_connector->dm_dp_aux.aux.dev = connector->kdev; r = drm_dp_aux_register(&amdgpu_dm_connector->dm_dp_aux.aux); if (r) return r; @@ -485,7 +486,6 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm, struct amdgpu_dm_connector *aconnector) { aconnector->dm_dp_aux.aux.name = "dmdc"; - aconnector->dm_dp_aux.aux.dev = aconnector->base.kdev; aconnector->dm_dp_aux.aux.transfer = dm_dp_aux_transfer; aconnector->dm_dp_aux.ddc_service = aconnector->dc_link->ddc; -- 2.24.1
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel