[13/15] drm/amdgpu/display: split dp connector registration (v3)

Commit Message

Alex Deucher Feb. 7, 2020, 9:17 p.m. UTC

Split into init and register functions to avoid a segfault
in some configs when the load/unload callbacks are removed.

v2:
- add back accidently dropped has_aux setting
- set dev in late_register

v3:
- fix dp cec ordering

Signed-off-by: Alex Deucher <[e-mail]>
---
 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(-)

Comments

Alex Deucher Feb. 11, 2020, 4:13 p.m. UTC | #1

Ping?

Alex

On Fri, Feb 7, 2020 at 4:17 PM Alex Deucher <[e-mail]> wrote:
>
> Split into init and register functions to avoid a segfault
> in some configs when the load/unload callbacks are removed.
>
> v2:
> - add back accidently dropped has_aux setting
> - set dev in late_register
>
> v3:
> - fix dp cec ordering
>
> Signed-off-by: Alex Deucher <[e-mail]>
> ---
>  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);
> +       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);
>
> --
> 2.24.1
>

Alex Deucher Feb. 13, 2020, 5:03 p.m. UTC | #2

Anyone want to take a shot at this one?

Alex

On Fri, Feb 7, 2020 at 4:17 PM Alex Deucher <[e-mail]> wrote:
>
> Split into init and register functions to avoid a segfault
> in some configs when the load/unload callbacks are removed.
>
> v2:
> - add back accidently dropped has_aux setting
> - set dev in late_register
>
> v3:
> - fix dp cec ordering
>
> Signed-off-by: Alex Deucher <[e-mail]>
> ---
>  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);
> +       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);
>
> --
> 2.24.1
>

Daniel Vetter Feb. 14, 2020, 7:39 a.m. UTC | #3

On Fri, Feb 07, 2020 at 04:17:13PM -0500, Alex Deucher wrote:
> Split into init and register functions to avoid a segfault
> in some configs when the load/unload callbacks are removed.
> 
> v2:
> - add back accidently dropped has_aux setting
> - set dev in late_register
> 
> v3:
> - fix dp cec ordering

Why did you move this back out of the late_register callback when going
from v2->v3? In i915 we register the cec stuff from ->late_register, like
anything else userspace visible. Maybe follow-up patch (the idea behind
removing the ->load callback is to close all the driver load races,
instead of only open("/dev/dri/0"), which is protected by
drm_global_mutex). On this:

Reviewed-by: Daniel Vetter <[e-mail]>

Cheers, Daniel

> 
> Signed-off-by: Alex Deucher <[e-mail]>
> ---
>  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);
> +	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);
>  
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Alex Deucher Feb. 14, 2020, 5:39 p.m. UTC | #4

On Fri, Feb 14, 2020 at 2:39 AM Daniel Vetter <[e-mail]> wrote:
>
> On Fri, Feb 07, 2020 at 04:17:13PM -0500, Alex Deucher wrote:
> > Split into init and register functions to avoid a segfault
> > in some configs when the load/unload callbacks are removed.
> >
> > v2:
> > - add back accidently dropped has_aux setting
> > - set dev in late_register
> >
> > v3:
> > - fix dp cec ordering
>
> Why did you move this back out of the late_register callback when going
> from v2->v3? In i915 we register the cec stuff from ->late_register, like

I got a bunch of complaints from the cec code when I had it switched
the other way.  They went away when I moved it back.  I don't remember
the exact messages off hand.

Alex

> anything else userspace visible. Maybe follow-up patch (the idea behind
> removing the ->load callback is to close all the driver load races,
> instead of only open("/dev/dri/0"), which is protected by
> drm_global_mutex). On this:
>
> Reviewed-by: Daniel Vetter <[e-mail]>
>
> Cheers, Daniel
>
> >
> > Signed-off-by: Alex Deucher <[e-mail]>
> > ---
> >  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);
> > +     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);
> >
> > --
> > 2.24.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Daniel Vetter Feb. 14, 2020, 6:35 p.m. UTC | #5

On Fri, Feb 14, 2020 at 12:39:22PM -0500, Alex Deucher wrote:
> On Fri, Feb 14, 2020 at 2:39 AM Daniel Vetter <[e-mail]> wrote:
> >
> > On Fri, Feb 07, 2020 at 04:17:13PM -0500, Alex Deucher wrote:
> > > Split into init and register functions to avoid a segfault
> > > in some configs when the load/unload callbacks are removed.
> > >
> > > v2:
> > > - add back accidently dropped has_aux setting
> > > - set dev in late_register
> > >
> > > v3:
> > > - fix dp cec ordering
> >
> > Why did you move this back out of the late_register callback when going
> > from v2->v3? In i915 we register the cec stuff from ->late_register, like
> 
> I got a bunch of complaints from the cec code when I had it switched
> the other way.  They went away when I moved it back.  I don't remember
> the exact messages off hand.

Would be interesting to learn want went wrong, just in case there's a core
bug here somewhere that prevents drivers from tdtr. But definitely no
reason to hold off this patch.
-Daniel

> 
> Alex
> 
> > anything else userspace visible. Maybe follow-up patch (the idea behind
> > removing the ->load callback is to close all the driver load races,
> > instead of only open("/dev/dri/0"), which is protected by
> > drm_global_mutex). On this:
> >
> > Reviewed-by: Daniel Vetter <[e-mail]>
> >
> > Cheers, Daniel
> >
> > >
> > > Signed-off-by: Alex Deucher <[e-mail]>
> > > ---
> > >  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);
> > > +     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);
> > >
> > > --
> > > 2.24.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

Alex Deucher Feb. 14, 2020, 8:07 p.m. UTC | #6

On Fri, Feb 14, 2020 at 1:35 PM Daniel Vetter <[e-mail]> wrote:
>
> On Fri, Feb 14, 2020 at 12:39:22PM -0500, Alex Deucher wrote:
> > On Fri, Feb 14, 2020 at 2:39 AM Daniel Vetter <[e-mail]> wrote:
> > >
> > > On Fri, Feb 07, 2020 at 04:17:13PM -0500, Alex Deucher wrote:
> > > > Split into init and register functions to avoid a segfault
> > > > in some configs when the load/unload callbacks are removed.
> > > >
> > > > v2:
> > > > - add back accidently dropped has_aux setting
> > > > - set dev in late_register
> > > >
> > > > v3:
> > > > - fix dp cec ordering
> > >
> > > Why did you move this back out of the late_register callback when going
> > > from v2->v3? In i915 we register the cec stuff from ->late_register, like
> >
> > I got a bunch of complaints from the cec code when I had it switched
> > the other way.  They went away when I moved it back.  I don't remember
> > the exact messages off hand.
>
> Would be interesting to learn want went wrong, just in case there's a core
> bug here somewhere that prevents drivers from tdtr. But definitely no
> reason to hold off this patch.

I'll repo it next week and send it out for posterity.  Thanks for the review.

Alex

> -Daniel
>
> >
> > Alex
> >
> > > anything else userspace visible. Maybe follow-up patch (the idea behind
> > > removing the ->load callback is to close all the driver load races,
> > > instead of only open("/dev/dri/0"), which is protected by
> > > drm_global_mutex). On this:
> > >
> > > Reviewed-by: Daniel Vetter <[e-mail]>
> > >
> > > Cheers, Daniel
> > >
> > > >
> > > > Signed-off-by: Alex Deucher <[e-mail]>
> > > > ---
> > > >  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);
> > > > +     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);
> > > >
> > > > --
> > > > 2.24.1
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > [e-mail]
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Harry Wentland Feb. 24, 2020, 9:09 p.m. UTC | #7

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 <[e-mail]>
> ---
>  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.

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);
>  
>

Alex Deucher Feb. 25, 2020, 2:06 p.m. UTC | #8

On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland <[e-mail]> 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 <[e-mail]>
> > ---
> >  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.

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);
> >
> >

Liu, Zhan Feb. 25, 2020, 3:10 p.m. UTC | #9

> -----Original Message-----
> From: Alex Deucher <[e-mail]>
> Sent: 2020/February/25, Tuesday 9:07 AM
> To: Wentland, Harry <[e-mail]>
> Cc: amd-gfx list <[e-mail]>; Maling list - DRI
> developers <[e-mail]>; Deucher, Alexander
> <[e-mail]>; Broadworth, Mark
> <[e-mail]>; Liu, Zhan <[e-mail]>
> Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector
> registration (v3)
> 
> On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland <[e-mail]>
> 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 <[e-mail]>
> > > ---
> > >  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.

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);
> > >
> > >

Liu, Zhan Feb. 25, 2020, 6:20 p.m. UTC | #10

> -----Original Message-----
> From: Liu, Zhan
> Sent: 2020/February/25, Tuesday 10:10 AM
> To: Alex Deucher <[e-mail]>; Wentland, Harry
> <[e-mail]>
> Cc: amd-gfx list <[e-mail]>; Maling list - DRI
> developers <[e-mail]>; Deucher, Alexander
> <[e-mail]>; Broadworth, Mark
> <[e-mail]>
> Subject: RE: [PATCH 13/15] drm/amdgpu/display: split dp connector
> registration (v3)
> 
> 
> > -----Original Message-----
> > From: Alex Deucher <[e-mail]>
> > Sent: 2020/February/25, Tuesday 9:07 AM
> > To: Wentland, Harry <[e-mail]>
> > Cc: amd-gfx list <[e-mail]>; Maling list - DRI
> > developers <[e-mail]>; Deucher, Alexander
> > <[e-mail]>; Broadworth, Mark
> > <[e-mail]>; Liu, Zhan <[e-mail]>
> > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector
> > registration (v3)
> >
> > On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland <[e-mail]>
> > 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 <[e-mail]>
> > > > ---
> > > >  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.

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);
> > > >
> > > >

Alex Deucher Feb. 25, 2020, 6:23 p.m. UTC | #11

On Tue, Feb 25, 2020 at 1:20 PM Liu, Zhan <[e-mail]> wrote:
>
>
> > -----Original Message-----
> > From: Liu, Zhan
> > Sent: 2020/February/25, Tuesday 10:10 AM
> > To: Alex Deucher <[e-mail]>; Wentland, Harry
> > <[e-mail]>
> > Cc: amd-gfx list <[e-mail]>; Maling list - DRI
> > developers <[e-mail]>; Deucher, Alexander
> > <[e-mail]>; Broadworth, Mark
> > <[e-mail]>
> > Subject: RE: [PATCH 13/15] drm/amdgpu/display: split dp connector
> > registration (v3)
> >
> >
> > > -----Original Message-----
> > > From: Alex Deucher <[e-mail]>
> > > Sent: 2020/February/25, Tuesday 9:07 AM
> > > To: Wentland, Harry <[e-mail]>
> > > Cc: amd-gfx list <[e-mail]>; Maling list - DRI
> > > developers <[e-mail]>; Deucher, Alexander
> > > <[e-mail]>; Broadworth, Mark
> > > <[e-mail]>; Liu, Zhan <[e-mail]>
> > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector
> > > registration (v3)
> > >
> > > On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland <[e-mail]>
> > > 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 <[e-mail]>
> > > > > ---
> > > > >  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);
> > > > >
> > > > >

Zuo, Jerry Feb. 25, 2020, 6:30 p.m. UTC | #12

[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 <[e-mail]> 
Sent: February 25, 2020 1:23 PM
To: Liu, Zhan <[e-mail]>
Cc: Wentland, Harry <[e-mail]>; Zuo, Jerry <[e-mail]>; amd-gfx list <[e-mail]>; Maling list - DRI developers <[e-mail]>; Deucher, Alexander <[e-mail]>; Broadworth, Mark <[e-mail]>
Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector registration (v3)

On Tue, Feb 25, 2020 at 1:20 PM Liu, Zhan <[e-mail]> wrote:
>
>
> > -----Original Message-----
> > From: Liu, Zhan
> > Sent: 2020/February/25, Tuesday 10:10 AM
> > To: Alex Deucher <[e-mail]>; Wentland, Harry 
> > <[e-mail]>
> > Cc: amd-gfx list <[e-mail]>; Maling list - DRI 
> > developers <[e-mail]>; Deucher, Alexander 
> > <[e-mail]>; Broadworth, Mark 
> > <[e-mail]>
> > Subject: RE: [PATCH 13/15] drm/amdgpu/display: split dp connector 
> > registration (v3)
> >
> >
> > > -----Original Message-----
> > > From: Alex Deucher <[e-mail]>
> > > Sent: 2020/February/25, Tuesday 9:07 AM
> > > To: Wentland, Harry <[e-mail]>
> > > Cc: amd-gfx list <[e-mail]>; Maling list - 
> > > DRI developers <[e-mail]>; Deucher, 
> > > Alexander <[e-mail]>; Broadworth, Mark 
> > > <[e-mail]>; Liu, Zhan <[e-mail]>
> > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector 
> > > registration (v3)
> > >
> > > On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland <[e-mail]>
> > > 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 <[e-mail]>
> > > > > ---
> > > > >  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);
> > > > >
> > > > >

Alex Deucher Feb. 25, 2020, 6:32 p.m. UTC | #13

On Tue, Feb 25, 2020 at 1:30 PM Zuo, Jerry <[e-mail]> 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.

Alex


>
> Regards,
> Jerry
>
> -----Original Message-----
> From: Alex Deucher <[e-mail]>
> Sent: February 25, 2020 1:23 PM
> To: Liu, Zhan <[e-mail]>
> Cc: Wentland, Harry <[e-mail]>; Zuo, Jerry <[e-mail]>; amd-gfx list <[e-mail]>; Maling list - DRI developers <[e-mail]>; Deucher, Alexander <[e-mail]>; Broadworth, Mark <[e-mail]>
> Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector registration (v3)
>
> On Tue, Feb 25, 2020 at 1:20 PM Liu, Zhan <[e-mail]> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Liu, Zhan
> > > Sent: 2020/February/25, Tuesday 10:10 AM
> > > To: Alex Deucher <[e-mail]>; Wentland, Harry
> > > <[e-mail]>
> > > Cc: amd-gfx list <[e-mail]>; Maling list - DRI
> > > developers <[e-mail]>; Deucher, Alexander
> > > <[e-mail]>; Broadworth, Mark
> > > <[e-mail]>
> > > Subject: RE: [PATCH 13/15] drm/amdgpu/display: split dp connector
> > > registration (v3)
> > >
> > >
> > > > -----Original Message-----
> > > > From: Alex Deucher <[e-mail]>
> > > > Sent: 2020/February/25, Tuesday 9:07 AM
> > > > To: Wentland, Harry <[e-mail]>
> > > > Cc: amd-gfx list <[e-mail]>; Maling list -
> > > > DRI developers <[e-mail]>; Deucher,
> > > > Alexander <[e-mail]>; Broadworth, Mark
> > > > <[e-mail]>; Liu, Zhan <[e-mail]>
> > > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector
> > > > registration (v3)
> > > >
> > > > On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland <[e-mail]>
> > > > 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 <[e-mail]>
> > > > > > ---
> > > > > >  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);
> > > > > >
> > > > > >

Alex Deucher Feb. 25, 2020, 6:42 p.m. UTC | #14

On Tue, Feb 25, 2020 at 1:32 PM Alex Deucher <[e-mail]> wrote:
>
> On Tue, Feb 25, 2020 at 1:30 PM Zuo, Jerry <[e-mail]> 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 <[e-mail]>
> > Sent: February 25, 2020 1:23 PM
> > To: Liu, Zhan <[e-mail]>
> > Cc: Wentland, Harry <[e-mail]>; Zuo, Jerry <[e-mail]>; amd-gfx list <[e-mail]>; Maling list - DRI developers <[e-mail]>; Deucher, Alexander <[e-mail]>; Broadworth, Mark <[e-mail]>
> > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector registration (v3)
> >
> > On Tue, Feb 25, 2020 at 1:20 PM Liu, Zhan <[e-mail]> wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Liu, Zhan
> > > > Sent: 2020/February/25, Tuesday 10:10 AM
> > > > To: Alex Deucher <[e-mail]>; Wentland, Harry
> > > > <[e-mail]>
> > > > Cc: amd-gfx list <[e-mail]>; Maling list - DRI
> > > > developers <[e-mail]>; Deucher, Alexander
> > > > <[e-mail]>; Broadworth, Mark
> > > > <[e-mail]>
> > > > Subject: RE: [PATCH 13/15] drm/amdgpu/display: split dp connector
> > > > registration (v3)
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Alex Deucher <[e-mail]>
> > > > > Sent: 2020/February/25, Tuesday 9:07 AM
> > > > > To: Wentland, Harry <[e-mail]>
> > > > > Cc: amd-gfx list <[e-mail]>; Maling list -
> > > > > DRI developers <[e-mail]>; Deucher,
> > > > > Alexander <[e-mail]>; Broadworth, Mark
> > > > > <[e-mail]>; Liu, Zhan <[e-mail]>
> > > > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector
> > > > > registration (v3)
> > > > >
> > > > > On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland <[e-mail]>
> > > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > >  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);
> > > > > > >
> > > > > > >

Zuo, Jerry Feb. 25, 2020, 7:07 p.m. UTC | #15

[AMD Official Use Only - Internal Distribution Only]

Hi Alex:

    The patch set is verified on Ubuntu, and I'll try it out on Chrome as well. 

    Thanks a lot!

Regards,
Jerry

-----Original Message-----
From: Alex Deucher <[e-mail]>
Sent: February 25, 2020 1:42 PM
To: Zuo, Jerry <[e-mail]>
Cc: Liu, Zhan <[e-mail]>; Wentland, Harry <[e-mail]>; amd-gfx list <[e-mail]>; Maling list - DRI developers <[e-mail]>; Deucher, Alexander <[e-mail]>; Broadworth, Mark <[e-mail]>
Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector registration (v3)

On Tue, Feb 25, 2020 at 1:32 PM Alex Deucher <[e-mail]> wrote:
>
> On Tue, Feb 25, 2020 at 1:30 PM Zuo, Jerry <[e-mail]> 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 <[e-mail]>
> > Sent: February 25, 2020 1:23 PM
> > To: Liu, Zhan <[e-mail]>
> > Cc: Wentland, Harry <[e-mail]>; Zuo, Jerry 
> > <[e-mail]>; amd-gfx list <[e-mail]>; 
> > Maling list - DRI developers <[e-mail]>;
> > Deucher, Alexander <[e-mail]>; Broadworth, Mark 
> > <[e-mail]>
> > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector 
> > registration (v3)
> >
> > On Tue, Feb 25, 2020 at 1:20 PM Liu, Zhan <[e-mail]> wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Liu, Zhan
> > > > Sent: 2020/February/25, Tuesday 10:10 AM
> > > > To: Alex Deucher <[e-mail]>; Wentland, Harry 
> > > > <[e-mail]>
> > > > Cc: amd-gfx list <[e-mail]>; Maling list - 
> > > > DRI developers <[e-mail]>; Deucher, 
> > > > Alexander <[e-mail]>; Broadworth, Mark 
> > > > <[e-mail]>
> > > > Subject: RE: [PATCH 13/15] drm/amdgpu/display: split dp 
> > > > connector registration (v3)
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Alex Deucher <[e-mail]>
> > > > > Sent: 2020/February/25, Tuesday 9:07 AM
> > > > > To: Wentland, Harry <[e-mail]>
> > > > > Cc: amd-gfx list <[e-mail]>; Maling list
> > > > > - DRI developers <[e-mail]>; Deucher, 
> > > > > Alexander <[e-mail]>; Broadworth, Mark 
> > > > > <[e-mail]>; Liu, Zhan <[e-mail]>
> > > > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp 
> > > > > connector registration (v3)
> > > > >
> > > > > On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland 
> > > > > <[e-mail]>
> > > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > >  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_type
> > > > > > > s.c
> > > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_type
> > > > > > > s.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);
> > > > > > >
> > > > > > >

Zuo, Jerry Feb. 26, 2020, 3:36 p.m. UTC | #16

[AMD Official Use Only - Internal Distribution Only]

Hi Alex:

     The patch set works. Please let me know when you push the change to drm-next.

     Thanks a lot.

Regards,
Jerry

-----Original Message-----
From: Alex Deucher <[e-mail]>
Sent: February 25, 2020 1:42 PM
To: Zuo, Jerry <[e-mail]>
Cc: Liu, Zhan <[e-mail]>; Wentland, Harry <[e-mail]>; amd-gfx list <[e-mail]>; Maling list - DRI developers <[e-mail]>; Deucher, Alexander <[e-mail]>; Broadworth, Mark <[e-mail]>
Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector registration (v3)

On Tue, Feb 25, 2020 at 1:32 PM Alex Deucher <[e-mail]> wrote:
>
> On Tue, Feb 25, 2020 at 1:30 PM Zuo, Jerry <[e-mail]> 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 <[e-mail]>
> > Sent: February 25, 2020 1:23 PM
> > To: Liu, Zhan <[e-mail]>
> > Cc: Wentland, Harry <[e-mail]>; Zuo, Jerry 
> > <[e-mail]>; amd-gfx list <[e-mail]>; 
> > Maling list - DRI developers <[e-mail]>;
> > Deucher, Alexander <[e-mail]>; Broadworth, Mark 
> > <[e-mail]>
> > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector 
> > registration (v3)
> >
> > On Tue, Feb 25, 2020 at 1:20 PM Liu, Zhan <[e-mail]> wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Liu, Zhan
> > > > Sent: 2020/February/25, Tuesday 10:10 AM
> > > > To: Alex Deucher <[e-mail]>; Wentland, Harry 
> > > > <[e-mail]>
> > > > Cc: amd-gfx list <[e-mail]>; Maling list - 
> > > > DRI developers <[e-mail]>; Deucher, 
> > > > Alexander <[e-mail]>; Broadworth, Mark 
> > > > <[e-mail]>
> > > > Subject: RE: [PATCH 13/15] drm/amdgpu/display: split dp 
> > > > connector registration (v3)
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Alex Deucher <[e-mail]>
> > > > > Sent: 2020/February/25, Tuesday 9:07 AM
> > > > > To: Wentland, Harry <[e-mail]>
> > > > > Cc: amd-gfx list <[e-mail]>; Maling list
> > > > > - DRI developers <[e-mail]>; Deucher, 
> > > > > Alexander <[e-mail]>; Broadworth, Mark 
> > > > > <[e-mail]>; Liu, Zhan <[e-mail]>
> > > > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp 
> > > > > connector registration (v3)
> > > > >
> > > > > On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland 
> > > > > <[e-mail]>
> > > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > >  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_type
> > > > > > > s.c
> > > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_type
> > > > > > > s.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);
> > > > > > >
> > > > > > >

Alex Deucher Feb. 26, 2020, 3:39 p.m. UTC | #17

On Wed, Feb 26, 2020 at 10:36 AM Zuo, Jerry <[e-mail]> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi Alex:
>
>      The patch set works. Please let me know when you push the change to drm-next.

Can someone give me and R-b or A-b on the patches?

Thanks,

Alex

>
>      Thanks a lot.
>
> Regards,
> Jerry
>
> -----Original Message-----
> From: Alex Deucher <[e-mail]>
> Sent: February 25, 2020 1:42 PM
> To: Zuo, Jerry <[e-mail]>
> Cc: Liu, Zhan <[e-mail]>; Wentland, Harry <[e-mail]>; amd-gfx list <[e-mail]>; Maling list - DRI developers <[e-mail]>; Deucher, Alexander <[e-mail]>; Broadworth, Mark <[e-mail]>
> Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector registration (v3)
>
> On Tue, Feb 25, 2020 at 1:32 PM Alex Deucher <[e-mail]> wrote:
> >
> > On Tue, Feb 25, 2020 at 1:30 PM Zuo, Jerry <[e-mail]> 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 <[e-mail]>
> > > Sent: February 25, 2020 1:23 PM
> > > To: Liu, Zhan <[e-mail]>
> > > Cc: Wentland, Harry <[e-mail]>; Zuo, Jerry
> > > <[e-mail]>; amd-gfx list <[e-mail]>;
> > > Maling list - DRI developers <[e-mail]>;
> > > Deucher, Alexander <[e-mail]>; Broadworth, Mark
> > > <[e-mail]>
> > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector
> > > registration (v3)
> > >
> > > On Tue, Feb 25, 2020 at 1:20 PM Liu, Zhan <[e-mail]> wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Liu, Zhan
> > > > > Sent: 2020/February/25, Tuesday 10:10 AM
> > > > > To: Alex Deucher <[e-mail]>; Wentland, Harry
> > > > > <[e-mail]>
> > > > > Cc: amd-gfx list <[e-mail]>; Maling list -
> > > > > DRI developers <[e-mail]>; Deucher,
> > > > > Alexander <[e-mail]>; Broadworth, Mark
> > > > > <[e-mail]>
> > > > > Subject: RE: [PATCH 13/15] drm/amdgpu/display: split dp
> > > > > connector registration (v3)
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Alex Deucher <[e-mail]>
> > > > > > Sent: 2020/February/25, Tuesday 9:07 AM
> > > > > > To: Wentland, Harry <[e-mail]>
> > > > > > Cc: amd-gfx list <[e-mail]>; Maling list
> > > > > > - DRI developers <[e-mail]>; Deucher,
> > > > > > Alexander <[e-mail]>; Broadworth, Mark
> > > > > > <[e-mail]>; Liu, Zhan <[e-mail]>
> > > > > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp
> > > > > > connector registration (v3)
> > > > > >
> > > > > > On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland
> > > > > > <[e-mail]>
> > > > > > 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 <[e-mail]>
> > > > > > > > ---
> > > > > > > >  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_type
> > > > > > > > s.c
> > > > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_type
> > > > > > > > s.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);
> > > > > > > >
> > > > > > > >

Zuo, Jerry Feb. 26, 2020, 3:41 p.m. UTC | #18

[AMD Official Use Only - Internal Distribution Only]

Reviewed-by: Jerry (Fangzhi) Zuo <[e-mail]>

-----Original Message-----
From: Alex Deucher <[e-mail]> 
Sent: February 26, 2020 10:40 AM
To: Zuo, Jerry <[e-mail]>
Cc: Liu, Zhan <[e-mail]>; Wentland, Harry <[e-mail]>; amd-gfx list <[e-mail]>; Maling list - DRI developers <[e-mail]>; Deucher, Alexander <[e-mail]>; Broadworth, Mark <[e-mail]>
Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector registration (v3)

On Wed, Feb 26, 2020 at 10:36 AM Zuo, Jerry <[e-mail]> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi Alex:
>
>      The patch set works. Please let me know when you push the change to drm-next.

Can someone give me and R-b or A-b on the patches?

Thanks,

Alex

>
>      Thanks a lot.
>
> Regards,
> Jerry
>
> -----Original Message-----
> From: Alex Deucher <[e-mail]>
> Sent: February 25, 2020 1:42 PM
> To: Zuo, Jerry <[e-mail]>
> Cc: Liu, Zhan <[e-mail]>; Wentland, Harry 
> <[e-mail]>; amd-gfx list 
> <[e-mail]>; Maling list - DRI developers 
> <[e-mail]>; Deucher, Alexander 
> <[e-mail]>; Broadworth, Mark 
> <[e-mail]>
> Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector 
> registration (v3)
>
> On Tue, Feb 25, 2020 at 1:32 PM Alex Deucher <[e-mail]> wrote:
> >
> > On Tue, Feb 25, 2020 at 1:30 PM Zuo, Jerry <[e-mail]> 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 <[e-mail]>
> > > Sent: February 25, 2020 1:23 PM
> > > To: Liu, Zhan <[e-mail]>
> > > Cc: Wentland, Harry <[e-mail]>; Zuo, Jerry 
> > > <[e-mail]>; amd-gfx list <[e-mail]>; 
> > > Maling list - DRI developers <[e-mail]>;
> > > Deucher, Alexander <[e-mail]>; Broadworth, Mark 
> > > <[e-mail]>
> > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp connector 
> > > registration (v3)
> > >
> > > On Tue, Feb 25, 2020 at 1:20 PM Liu, Zhan <[e-mail]> wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Liu, Zhan
> > > > > Sent: 2020/February/25, Tuesday 10:10 AM
> > > > > To: Alex Deucher <[e-mail]>; Wentland, Harry 
> > > > > <[e-mail]>
> > > > > Cc: amd-gfx list <[e-mail]>; Maling list 
> > > > > - DRI developers <[e-mail]>; Deucher, 
> > > > > Alexander <[e-mail]>; Broadworth, Mark 
> > > > > <[e-mail]>
> > > > > Subject: RE: [PATCH 13/15] drm/amdgpu/display: split dp 
> > > > > connector registration (v3)
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Alex Deucher <[e-mail]>
> > > > > > Sent: 2020/February/25, Tuesday 9:07 AM
> > > > > > To: Wentland, Harry <[e-mail]>
> > > > > > Cc: amd-gfx list <[e-mail]>; Maling 
> > > > > > list
> > > > > > - DRI developers <[e-mail]>; Deucher, 
> > > > > > Alexander <[e-mail]>; Broadworth, Mark 
> > > > > > <[e-mail]>; Liu, Zhan <[e-mail]>
> > > > > > Subject: Re: [PATCH 13/15] drm/amdgpu/display: split dp 
> > > > > > connector registration (v3)
> > > > > >
> > > > > > On Mon, Feb 24, 2020 at 4:09 PM Harry Wentland 
> > > > > > <[e-mail]>
> > > > > > 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 <[e-mail]>
> > > > > > > > ---
> > > > > > > >  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_ty
> > > > > > > > pe
> > > > > > > > s.c
> > > > > > > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_ty
> > > > > > > > pe s.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.au
> > > > > > > > + x)
> > > > > > > > + ;
> > > > > > >
> > > > > > > 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);
> > > > > > > >
> > > > > > > >

Patch

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);
+	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);