On 5/28/19 6:28 AM, Emily Deng wrote:
> Issue:
> Will have follow error when reload driver:
> [ 3986.567739] sysfs: cannot create duplicate filename
> '/devices/pci:00/:00:07.0/drm_dp_aux_dev'
> [ 3986.567743] CPU: 6 PID: 1767 Comm: modprobe Tainted: G OE
> 5.0.0-rc1-custom #1
> [ 3986.567745] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> Ubuntu-1.8.2-1ubuntu1 04/01/2014
> [ 3986.567746] Call Trace:
> ..
> [ 3986.567808] drm_dp_aux_register_devnode+0xdc/0x140 [drm_kms_helper]
> ..
> [ 3986.569081] kobject_add_internal failed for drm_dp_aux_dev with -EEXIST,
> don't try to register things with the same name in the same directory.
>
> Reproduce sequences:
> 1.modprobe amdgpu
> 2.modprobe -r amdgpu
> 3.modprobe amdgpu
>
> Root cause:
> When unload driver, it don't unregister aux.
>
> Signed-off-by: Emily Deng
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 1 +
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 18
> +-
> .../drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c| 6 +-
> 3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index e48fd35..720955b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -525,6 +525,7 @@ enum amdgpu_connector_dither {
> struct amdgpu_dm_dp_aux {
> struct drm_dp_aux aux;
> struct ddc_service *ddc_service;
> + bool has_aux;
> };
>
> struct amdgpu_i2c_adapter {
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 8fe1685..de369ae 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3760,6 +3760,16 @@ int amdgpu_dm_connector_atomic_get_property(struct
> drm_connector *connector,
> return ret;
> }
>
> +static void amdgpu_dm_connector_unregister(struct drm_connector *connector)
> +{
> + struct amdgpu_dm_connector *amdgpu_dm_connector =
> to_amdgpu_dm_connector(connector);
> +
> + if (amdgpu_dm_connector->dm_dp_aux.has_aux) {
> + drm_dp_aux_unregister(_dm_connector->dm_dp_aux.aux);
> + amdgpu_dm_connector->dm_dp_aux.has_aux = false;
> + }
> +}
This is a good catch, but I'm pretty sure we don't need the has_aux
state here for this.
You should be able to call drm_dp_aux_unregister regardless of whether
drm_dp_aux_register succeeded or not.
Nicholas Kazlauskas
> +
> static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
> {
> struct amdgpu_dm_connector *aconnector =
> to_amdgpu_dm_connector(connector);
> @@ -3788,6 +3798,11 @@ static void amdgpu_dm_connector_destroy(struct
> drm_connector *connector)
> drm_dp_cec_unregister_connector(>dm_dp_aux.aux);
> drm_connector_unregister(connector);
> drm_connector_cleanup(connector);
> + if (aconnector->i2c) {
> + i2c_del_adapter(>i2c->base);
> + kfree(aconnector->i2c);
> + }
> +
> kfree(connector);
> }
>
> @@ -3846,7 +3861,8 @@ static const struct drm_connector_funcs
> amdgpu_dm_connector_funcs = {
> .atomic_duplicate_state = amdgpu_dm_connector_atomic_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> .atomic_set_property = amdgpu_dm_connector_atomic_set_property,
> - .atomic_get_property = amdgpu_dm_connector_atomic_get_property
> + .atomic_get_property = amdgpu_dm_connector_atomic_get_property,
> + .early_unregister = amdgpu_dm_connector_unregister
> };
>
> static int get_modes(struct drm_connector *connector)
> 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 6e205ee..190e92c 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
> @@ -387,12 +387,16 @@ static const struct drm_dp_mst_topology_cbs dm_mst_cbs
> = {
> void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
> struct amdgpu_dm_connector *aconnector)
> {
> + int ret;
> aconnector->dm_dp_aux.aux.name = "dmdc";
> aconnector->dm_dp_aux.aux.dev = dm->adev->dev;
> aconnector->dm_dp_aux.aux.transfer = dm_dp_aux_transfer;
> aconnector->dm_dp_aux.ddc_service = aconnector->dc_link->ddc;
>
> - drm_dp_aux_register(>dm_dp_aux.aux);
> + ret = drm_dp_aux_register(>dm_dp_aux.aux);
> + if (!ret)
> + aconnector->dm_dp_aux.has_aux = true;
> +
> drm_dp_cec_register_connector(>dm_dp_aux.aux,
> aconnector->base.name, dm->adev->dev);
> aconnector->mst_mgr.cbs = _mst_cbs;
>
