[PATCH 13/15] drm/amdgpu/display: split dp connector registration (v3)
Zuo, Jerry
Jerry.Zuo at amd.com
Tue Feb 25 18:30:52 UTC 2020
[AMD Official Use Only - Internal Distribution Only]
Hi Alex:
It happened when a MST monitor is attached, either in driver load or hotplug later.
Regards,
Jerry
-----Original Message-----
From: Alex Deucher <alexdeucher at gmail.com>
Sent: February 25, 2020 1:23 PM
To: Liu, Zhan <Zhan.Liu at amd.com>
Cc: Wentland, Harry <Harry.Wentland at amd.com>; Zuo, Jerry <Jerry.Zuo at amd.com>; amd-gfx list <amd-gfx at lists.freedesktop.org>; Maling list - DRI developers <dri-devel at lists.freedesktop.org>; Deucher, Alexander <Alexander.Deucher at amd.com>; Broadworth, Mark <Mark.Broadworth at 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.Liu at amd.com> wrote:
>
>
> > -----Original Message-----
> > From: Liu, Zhan
> > Sent: 2020/February/25, Tuesday 10:10 AM
> > To: Alex Deucher <alexdeucher at gmail.com>; Wentland, Harry
> > <Harry.Wentland at amd.com>
> > Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>; Maling list - DRI
> > developers <dri-devel at lists.freedesktop.org>; Deucher, Alexander
> > <Alexander.Deucher at amd.com>; Broadworth, Mark
> > <Mark.Broadworth at amd.com>
> > Subject: RE: [PATCH 13/15] drm/amdgpu/display: split dp connector
> > registration (v3)
> >
> >
> > > -----Original Message-----
> > > From: Alex Deucher <alexdeucher at gmail.com>
> > > Sent: 2020/February/25, Tuesday 9:07 AM
> > > To: Wentland, Harry <Harry.Wentland at amd.com>
> > > Cc: amd-gfx list <amd-gfx at lists.freedesktop.org>; Maling list -
> > > DRI developers <dri-devel at lists.freedesktop.org>; Deucher,
> > > Alexander <Alexander.Deucher at amd.com>; Broadworth, Mark
> > > <Mark.Broadworth at amd.com>; Liu, Zhan <Zhan.Liu at 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 <hwentlan at 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.deucher at 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);
> > > > >
> > > > >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dmesg.log
Type: application/octet-stream
Size: 104650 bytes
Desc: dmesg.log
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20200225/8b6429ee/attachment-0001.obj>
More information about the amd-gfx
mailing list