[PATCH (repost) 0/5] drm/nouveau+amdgpu: add DP CEC-Tunneling-over-AUX (repost)

[PATCH (repost) 0/5] drm/nouveau+amdgpu: add DP CEC-Tunneling-over-AUX (repost)

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Repost because I wasn't a member of the nouveau mailinglist the
first time around and this series was blocked. I also updated this
cover letter for the part about the amdgpu patch after receiving
feedback from Alex Deucher. The patches are unchanged (except for
adding Alex' Acked-by to the amdgpu patch).

Now that the DisplayPort CEC-Tunneling-over-AUX drm+i915 support
has been merged in the mainline kernel it is time to roll this
out to nouveau and amdgpu as well.

The first patch is required for this: it adds checks that the drm_dp_cec
functions are called with a working aux implementation. These checks
weren't necessary for the i915, but nouveau and amdgpu require them.

The next two patches update a comment in drm_dp_cec.c and fix a bug
in sideband AUX handling that I found while researching CEC Tunneling
over an MST hub. It's there to prevent others from stumbling over the
same bug in the future.

The fourth patch adds support for CEC to the nouveau driver.

The last patch adds support for CEC to the amdgpu driver. CEC is
only supported for the new DC modesetting code (thanks to Alex Deucher
for explaining this to me). I have no plans to add CEC support to the
old modesetting code (amd/amdgpu/dce*.c). If someone wants to, then
please contact me. I can't test this myself, but I can assist.

Two notes on CEC-Tunneling-over-AUX:

1) You need to be very careful about which USB-C/DP/mini-DP to HDMI
   adapters you buy. Only a few support this feature correctly today.
   Known chipsets that support this are Parade PS175 & PS176 and
   MegaChips 2900. Unfortunately almost all Parade-based adapters
   do not hook up the HDMI CEC pin to the chip, making them useless
   for this. The Parade reference design was only recently changed
   to hook up this pin, so perhaps this situation will change for
   new Parade-based adapters.

   Adapters based on the new MegaChips 2900 fare much better: it
   appears that their reference design *does* hook up the CEC pin.
   Club3D has adapters using this device for USB-C, DP and mini-DP
   to HDMI, and they all work fine.

   If anyone finds other adapters that work besides those I list
   in https://hverkuil.home.xs4all.nl/cec-status.txt, please let
   me know and I'll add them to the list.

   Linux is the first OS that supports this feature, so I am
   not surprised that HW support for this has been poor. Hopefully
   this will change going forward. BTW, note the irony that CEC is
   now supported for DP-to-HDMI adapters, but not for the native
   HDMI ports on NVIDIA/AMD/Intel GPUs.

2) CEC-Tunneling does not work (yet?) if there is an MST hub in
   between. I'm still researching this but this might be a limitation 
   of MST.

Regards,

        Hans


Hans Verkuil (5):
  drm_dp_cec: check that aux has a transfer function
  drm_dp_cec: add note about good MegaChips 2900 CEC support
  drm_dp_mst_topology: fix broken
    drm_dp_sideband_parse_remote_dpcd_read()
  drm/nouveau: add DisplayPort CEC-Tunneling-over-AUX support
  drm/amdgpu: add DisplayPort CEC-Tunneling-over-AUX support

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 13 +++++++++++--
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c    |  2 ++
 drivers/gpu/drm/drm_dp_cec.c                   | 18 +++++++++++++++++-
 drivers/gpu/drm/drm_dp_mst_topology.c          |  1 +
 drivers/gpu/drm/nouveau/nouveau_connector.c    | 17 +++++++++++++++--
 5 files changed, 46 insertions(+), 5 deletions(-)

-- 
2.18.0

[PATCH (repost) 1/5] drm_dp_cec: check that aux has a transfer function

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

If aux->transfer == NULL, then just return without doing
anything. In that case the function is likely called for
a non-(e)DP connector.

This never happened for the i915 driver, but the nouveau and amdgpu
drivers need this check.

The alternative would be to add this check in those drivers before
every drm_dp_cec call, but it makes sense to check it in the
drm_dp_cec functions to prevent a kernel oops.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/drm_dp_cec.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
index 988513346e9c..1407b13a8d5d 100644
--- a/drivers/gpu/drm/drm_dp_cec.c
+++ b/drivers/gpu/drm/drm_dp_cec.c
@@ -238,6 +238,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
 	u8 cec_irq;
 	int ret;
 
+	/* No transfer function was set, so not a DP connector */
+	if (!aux->transfer)
+		return;
+
 	mutex_lock(&aux->cec.lock);
 	if (!aux->cec.adap)
 		goto unlock;
@@ -293,6 +297,10 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const struct edid *edid)
 	unsigned int num_las = 1;
 	u8 cap;
 
+	/* No transfer function was set, so not a DP connector */
+	if (!aux->transfer)
+		return;
+
 #ifndef CONFIG_MEDIA_CEC_RC
 	/*
 	 * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
@@ -361,6 +369,10 @@ EXPORT_SYMBOL(drm_dp_cec_set_edid);
  */
 void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
 {
+	/* No transfer function was set, so not a DP connector */
+	if (!aux->transfer)
+		return;
+
 	cancel_delayed_work_sync(&aux->cec.unregister_work);
 
 	mutex_lock(&aux->cec.lock);
@@ -404,6 +416,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux, const char *name,
 				   struct device *parent)
 {
 	WARN_ON(aux->cec.adap);
+	if (WARN_ON(!aux->transfer))
+		return;
 	aux->cec.name = name;
 	aux->cec.parent = parent;
 	INIT_DELAYED_WORK(&aux->cec.unregister_work,
-- 
2.18.0

[PATCH (repost) 2/5] drm_dp_cec: add note about good MegaChips 2900 CEC support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

A big problem with DP CEC-Tunneling-over-AUX is that it is tricky
to find adapters with a chipset that supports this AND where the
manufacturer actually connected the HDMI CEC line to the chipset.

Add a mention of the MegaChips 2900 chipset which seems to support
this feature well.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/drm_dp_cec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
index 1407b13a8d5d..8a718f85079a 100644
--- a/drivers/gpu/drm/drm_dp_cec.c
+++ b/drivers/gpu/drm/drm_dp_cec.c
@@ -16,7 +16,9 @@
  * here. Quite a few active (mini-)DP-to-HDMI or USB-C-to-HDMI adapters
  * have a converter chip that supports CEC-Tunneling-over-AUX (usually the
  * Parade PS176), but they do not wire up the CEC pin, thus making CEC
- * useless.
+ * useless. Note that MegaChips 2900-based adapters appear to have good
+ * support for CEC tunneling. Those adapters that I have tested using
+ * this chipset all have the CEC line connected.
  *
  * Sadly there is no way for this driver to know this. What happens is
  * that a /dev/cecX device is created that is isolated and unable to see
-- 
2.18.0

[PATCH (repost) 3/5] drm_dp_mst_topology: fix broken drm_dp_sideband_parse_remote_dpcd_read()

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When parsing the reply of a DP_REMOTE_DPCD_READ DPCD command the
result is wrong due to a missing idx increment.

This was never noticed since DP_REMOTE_DPCD_READ is currently not
used, but if you enable it, then it is all wrong.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 7780567aa669..5ff1d79b86c4 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -439,6 +439,7 @@ static bool drm_dp_sideband_parse_remote_dpcd_read(struct drm_dp_sideband_msg_rx
 	if (idx > raw->curlen)
 		goto fail_len;
 	repmsg->u.remote_dpcd_read_ack.num_bytes = raw->msg[idx];
+	idx++;
 	if (idx > raw->curlen)
 		goto fail_len;
 
-- 
2.18.0

[PATCH (repost) 4/5] drm/nouveau: add DisplayPort CEC-Tunneling-over-AUX support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add DisplayPort CEC-Tunneling-over-AUX support to nouveau.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 51932c72334e..eb4f766b5958 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -400,8 +400,10 @@ nouveau_connector_destroy(struct drm_connector *connector)
 	kfree(nv_connector->edid);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
-	if (nv_connector->aux.transfer)
+	if (nv_connector->aux.transfer) {
+		drm_dp_cec_unregister_connector(&nv_connector->aux);
 		drm_dp_aux_unregister(&nv_connector->aux);
+	}
 	kfree(connector);
 }
 
@@ -608,6 +610,7 @@ nouveau_connector_detect(struct drm_connector *connector, bool force)
 
 		nouveau_connector_set_encoder(connector, nv_encoder);
 		conn_status = connector_status_connected;
+		drm_dp_cec_set_edid(&nv_connector->aux, nv_connector->edid);
 		goto out;
 	}
 
@@ -1108,11 +1111,14 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
 
 	if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) {
 		NV_DEBUG(drm, "service %s\n", name);
+		drm_dp_cec_irq(&nv_connector->aux);
 		if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP)))
 			nv50_mstm_service(nv_encoder->dp.mstm);
 	} else {
 		bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG);
 
+		if (!plugged)
+			drm_dp_cec_unset_edid(&nv_connector->aux);
 		NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name);
 		if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) {
 			if (!plugged)
@@ -1302,7 +1308,6 @@ nouveau_connector_create(struct drm_device *dev, int index)
 			kfree(nv_connector);
 			return ERR_PTR(ret);
 		}
-
 		funcs = &nouveau_connector_funcs;
 		break;
 	default:
@@ -1356,6 +1361,14 @@ nouveau_connector_create(struct drm_device *dev, int index)
 		break;
 	}
 
+	switch (type) {
+	case DRM_MODE_CONNECTOR_DisplayPort:
+	case DRM_MODE_CONNECTOR_eDP:
+		drm_dp_cec_register_connector(&nv_connector->aux,
+					      connector->name, dev->dev);
+		break;
+	}
+
 	ret = nvif_notify_init(&disp->disp.object, nouveau_connector_hotplug,
 			       true, NV04_DISP_NTFY_CONN,
 			       &(struct nvif_notify_conn_req_v0) {
-- 
2.18.0

[PATCH (repost) 5/5] drm/amdgpu: add DisplayPort CEC-Tunneling-over-AUX support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add DisplayPort CEC-Tunneling-over-AUX support to amdgpu.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 13 +++++++++++--
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c |  2 ++
 2 files changed, 13 insertions(+), 2 deletions(-)

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 34f34823bab5..77898c95bef6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -898,6 +898,7 @@ amdgpu_dm_update_connector_after_detect(struct amdgpu_dm_connector *aconnector)
 		aconnector->dc_sink = sink;
 		if (sink->dc_edid.length == 0) {
 			aconnector->edid = NULL;
+			drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
 		} else {
 			aconnector->edid =
 				(struct edid *) sink->dc_edid.raw_edid;
@@ -905,10 +906,13 @@ amdgpu_dm_update_connector_after_detect(struct amdgpu_dm_connector *aconnector)
 
 			drm_connector_update_edid_property(connector,
 					aconnector->edid);
+			drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
+					    aconnector->edid);
 		}
 		amdgpu_dm_add_sink_to_freesync_module(connector, aconnector->edid);
 
 	} else {
+		drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
 		amdgpu_dm_remove_sink_from_freesync_module(connector);
 		drm_connector_update_edid_property(connector, NULL);
 		aconnector->num_modes = 0;
@@ -1059,12 +1063,16 @@ static void handle_hpd_rx_irq(void *param)
 			drm_kms_helper_hotplug_event(dev);
 		}
 	}
+
 	if ((dc_link->cur_link_settings.lane_count != LANE_COUNT_UNKNOWN) ||
-	    (dc_link->type == dc_connection_mst_branch))
+	    (dc_link->type == dc_connection_mst_branch)) {
 		dm_handle_hpd_rx_irq(aconnector);
+	}
 
-	if (dc_link->type != dc_connection_mst_branch)
+	if (dc_link->type != dc_connection_mst_branch) {
+		drm_dp_cec_irq(&aconnector->dm_dp_aux.aux);
 		mutex_unlock(&aconnector->hpd_lock);
+	}
 }
 
 static void register_hpd_handlers(struct amdgpu_device *adev)
@@ -2732,6 +2740,7 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
 		dm->backlight_dev = NULL;
 	}
 #endif
+	drm_dp_cec_unregister_connector(&aconnector->dm_dp_aux.aux);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 	kfree(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 9a300732ba37..18a3a6e5ffa0 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
@@ -496,6 +496,8 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
 	aconnector->dm_dp_aux.ddc_service = aconnector->dc_link->ddc;
 
 	drm_dp_aux_register(&aconnector->dm_dp_aux.aux);
+	drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
+				      aconnector->base.name, dm->adev->dev);
 	aconnector->mst_mgr.cbs = &dm_mst_cbs;
 	drm_dp_mst_topology_mgr_init(
 		&aconnector->mst_mgr,
-- 
2.18.0

Re: [PATCH (repost) 1/5] drm_dp_cec: check that aux has a transfer function

[Lyude Paul]

On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> If aux->transfer == NULL, then just return without doing
> anything. In that case the function is likely called for
> a non-(e)DP connector.
> 
> This never happened for the i915 driver, but the nouveau and amdgpu
> drivers need this check.
Could you give a backtrace from where you're hitting this issue with nouveau and
amdgpu? It doesn't make a whole ton of sense to have connectors registering DP
aux busses if they aren't actually DP, that should probably just be fixed...

> 
> The alternative would be to add this check in those drivers before
> every drm_dp_cec call, but it makes sense to check it in the
> drm_dp_cec functions to prevent a kernel oops.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/gpu/drm/drm_dp_cec.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> index 988513346e9c..1407b13a8d5d 100644
> --- a/drivers/gpu/drm/drm_dp_cec.c
> +++ b/drivers/gpu/drm/drm_dp_cec.c
> @@ -238,6 +238,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
>  	u8 cec_irq;
>  	int ret;
>  
> +	/* No transfer function was set, so not a DP connector */
> +	if (!aux->transfer)
> +		return;
> +
>  	mutex_lock(&aux->cec.lock);
>  	if (!aux->cec.adap)
>  		goto unlock;
> @@ -293,6 +297,10 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
> struct edid *edid)
>  	unsigned int num_las = 1;
>  	u8 cap;
>  
> +	/* No transfer function was set, so not a DP connector */
> +	if (!aux->transfer)
> +		return;
> +
>  #ifndef CONFIG_MEDIA_CEC_RC
>  	/*
>  	 * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
> @@ -361,6 +369,10 @@ EXPORT_SYMBOL(drm_dp_cec_set_edid);
>   */
>  void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
>  {
> +	/* No transfer function was set, so not a DP connector */
> +	if (!aux->transfer)
> +		return;
> +
>  	cancel_delayed_work_sync(&aux->cec.unregister_work);
>  
>  	mutex_lock(&aux->cec.lock);
> @@ -404,6 +416,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
> const char *name,
>  				   struct device *parent)
>  {
>  	WARN_ON(aux->cec.adap);
> +	if (WARN_ON(!aux->transfer))
> +		return;
>  	aux->cec.name = name;
>  	aux->cec.parent = parent;
>  	INIT_DELAYED_WORK(&aux->cec.unregister_work,

Re: [Nouveau] [PATCH (repost) 2/5] drm_dp_cec: add note about good MegaChips 2900 CEC support

[Lyude Paul]

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> A big problem with DP CEC-Tunneling-over-AUX is that it is tricky
> to find adapters with a chipset that supports this AND where the
> manufacturer actually connected the HDMI CEC line to the chipset.
> 
> Add a mention of the MegaChips 2900 chipset which seems to support
> this feature well.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/gpu/drm/drm_dp_cec.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> index 1407b13a8d5d..8a718f85079a 100644
> --- a/drivers/gpu/drm/drm_dp_cec.c
> +++ b/drivers/gpu/drm/drm_dp_cec.c
> @@ -16,7 +16,9 @@
>   * here. Quite a few active (mini-)DP-to-HDMI or USB-C-to-HDMI adapters
>   * have a converter chip that supports CEC-Tunneling-over-AUX (usually the
>   * Parade PS176), but they do not wire up the CEC pin, thus making CEC
> - * useless.
> + * useless. Note that MegaChips 2900-based adapters appear to have good
> + * support for CEC tunneling. Those adapters that I have tested using
> + * this chipset all have the CEC line connected.
>   *
>   * Sadly there is no way for this driver to know this. What happens is
>   * that a /dev/cecX device is created that is isolated and unable to see

Re: [PATCH (repost) 4/5] drm/nouveau: add DisplayPort CEC-Tunneling-over-AUX support

[Lyude Paul]

On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Add DisplayPort CEC-Tunneling-over-AUX support to nouveau.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/gpu/drm/nouveau/nouveau_connector.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
> b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index 51932c72334e..eb4f766b5958 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -400,8 +400,10 @@ nouveau_connector_destroy(struct drm_connector
> *connector)
>  	kfree(nv_connector->edid);
>  	drm_connector_unregister(connector);
>  	drm_connector_cleanup(connector);
> -	if (nv_connector->aux.transfer)
> +	if (nv_connector->aux.transfer) {
With the comments I made about this on patch 1 addressed/resolved:

Reviewed-by: Lyude Paul <lyude@redhat.com>

> +		drm_dp_cec_unregister_connector(&nv_connector->aux);
>  		drm_dp_aux_unregister(&nv_connector->aux);
> +	}
>  	kfree(connector);
>  }
>  
> @@ -608,6 +610,7 @@ nouveau_connector_detect(struct drm_connector *connector,
> bool force)
>  
>  		nouveau_connector_set_encoder(connector, nv_encoder);
>  		conn_status = connector_status_connected;
> +		drm_dp_cec_set_edid(&nv_connector->aux, nv_connector->edid);
>  		goto out;
>  	}
>  
> @@ -1108,11 +1111,14 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
>  
>  	if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) {
>  		NV_DEBUG(drm, "service %s\n", name);
> +		drm_dp_cec_irq(&nv_connector->aux);
>  		if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP)))
>  			nv50_mstm_service(nv_encoder->dp.mstm);
>  	} else {
>  		bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG);
>  
> +		if (!plugged)
> +			drm_dp_cec_unset_edid(&nv_connector->aux);
>  		NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name);
>  		if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) {
>  			if (!plugged)
> @@ -1302,7 +1308,6 @@ nouveau_connector_create(struct drm_device *dev, int
> index)
>  			kfree(nv_connector);
>  			return ERR_PTR(ret);
>  		}
> -
>  		funcs = &nouveau_connector_funcs;
>  		break;
>  	default:
> @@ -1356,6 +1361,14 @@ nouveau_connector_create(struct drm_device *dev, int
> index)
>  		break;
>  	}
>  
> +	switch (type) {
> +	case DRM_MODE_CONNECTOR_DisplayPort:
> +	case DRM_MODE_CONNECTOR_eDP:
> +		drm_dp_cec_register_connector(&nv_connector->aux,
> +					      connector->name, dev->dev);
> +		break;
> +	}
> +
>  	ret = nvif_notify_init(&disp->disp.object, nouveau_connector_hotplug,
>  			       true, NV04_DISP_NTFY_CONN,
>  			       &(struct nvif_notify_conn_req_v0) {

Re: [PATCH (repost) 3/5] drm_dp_mst_topology: fix broken drm_dp_sideband_parse_remote_dpcd_read()

[Lyude Paul]

Reviewed-by: Lyude Paul <lyude@redhat.com>

We really need to add support for using this into the MST helpers. A good way to
test this would probably be to hook up an aux device to the DP AUX adapters we
create for each MST topology

On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> When parsing the reply of a DP_REMOTE_DPCD_READ DPCD command the
> result is wrong due to a missing idx increment.
> 
> This was never noticed since DP_REMOTE_DPCD_READ is currently not
> used, but if you enable it, then it is all wrong.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 7780567aa669..5ff1d79b86c4 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -439,6 +439,7 @@ static bool drm_dp_sideband_parse_remote_dpcd_read(struct
> drm_dp_sideband_msg_rx
>  	if (idx > raw->curlen)
>  		goto fail_len;
>  	repmsg->u.remote_dpcd_read_ack.num_bytes = raw->msg[idx];
> +	idx++;
>  	if (idx > raw->curlen)
>  		goto fail_len;
>  

Re: [PATCH (repost) 3/5] drm_dp_mst_topology: fix broken drm_dp_sideband_parse_remote_dpcd_read()

[Hans Verkuil]

On 08/20/2018 08:59 PM, Lyude Paul wrote:
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> 
> We really need to add support for using this into the MST helpers. A good way to
> test this would probably be to hook up an aux device to the DP AUX adapters we
> create for each MST topology

If you are interested, I have code for that in my MST test branch:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=cec-nv-amd-mst

It's the "drm_dp_mst_topology: use correct AUX channel" patch.

I don't have plans to post this patch since CEC for MST isn't working
(still trying to figure out why not), but you are free to pick it up
if you want.

Regards,

	Hans

> 
> On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> When parsing the reply of a DP_REMOTE_DPCD_READ DPCD command the
>> result is wrong due to a missing idx increment.
>>
>> This was never noticed since DP_REMOTE_DPCD_READ is currently not
>> used, but if you enable it, then it is all wrong.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/gpu/drm/drm_dp_mst_topology.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index 7780567aa669..5ff1d79b86c4 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -439,6 +439,7 @@ static bool drm_dp_sideband_parse_remote_dpcd_read(struct
>> drm_dp_sideband_msg_rx
>>  	if (idx > raw->curlen)
>>  		goto fail_len;
>>  	repmsg->u.remote_dpcd_read_ack.num_bytes = raw->msg[idx];
>> +	idx++;
>>  	if (idx > raw->curlen)
>>  		goto fail_len;
>>  
> 

Re: [PATCH (repost) 1/5] drm_dp_cec: check that aux has a transfer function

[Hans Verkuil]

On 08/20/2018 08:51 PM, Lyude Paul wrote:
> On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> If aux->transfer == NULL, then just return without doing
>> anything. In that case the function is likely called for
>> a non-(e)DP connector.
>>
>> This never happened for the i915 driver, but the nouveau and amdgpu
>> drivers need this check.
> Could you give a backtrace from where you're hitting this issue with nouveau and
> amdgpu? It doesn't make a whole ton of sense to have connectors registering DP
> aux busses if they aren't actually DP, that should probably just be fixed...

The difference between the i915 driver and the nouveau (and amdgpu) driver is
that in the i915 driver the drm_dp_cec_set_edid/unset_edid/irq functions are
called from code that is exclusively for DisplayPort connectors.

For nouveau/amdgpu they are called from code that is shared between DisplayPort
and HDMI, so aux->transfer may be NULL.

Rather than either testing for the connector type or for a non-NULL aux->transfer
every time I call a drm_dp_cec_* function, it is better to just test for
aux->transfer in the drm_dp_cec_* functions themselves. It's more robust.

So there isn't a bug or anything like that, it's just so that these drm_dp_cec
functions can handle a slightly different driver design safely.

The registration and unregistration of the cec devices is always DP specific,
and an attempt to register a cec device for a non-DP connector will now fail
with a WARN_ON.

Regards,

	Hans

> 
>>
>> The alternative would be to add this check in those drivers before
>> every drm_dp_cec call, but it makes sense to check it in the
>> drm_dp_cec functions to prevent a kernel oops.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/gpu/drm/drm_dp_cec.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
>> index 988513346e9c..1407b13a8d5d 100644
>> --- a/drivers/gpu/drm/drm_dp_cec.c
>> +++ b/drivers/gpu/drm/drm_dp_cec.c
>> @@ -238,6 +238,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
>>  	u8 cec_irq;
>>  	int ret;
>>  
>> +	/* No transfer function was set, so not a DP connector */
>> +	if (!aux->transfer)
>> +		return;
>> +
>>  	mutex_lock(&aux->cec.lock);
>>  	if (!aux->cec.adap)
>>  		goto unlock;
>> @@ -293,6 +297,10 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux, const
>> struct edid *edid)
>>  	unsigned int num_las = 1;
>>  	u8 cap;
>>  
>> +	/* No transfer function was set, so not a DP connector */
>> +	if (!aux->transfer)
>> +		return;
>> +
>>  #ifndef CONFIG_MEDIA_CEC_RC
>>  	/*
>>  	 * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
>> @@ -361,6 +369,10 @@ EXPORT_SYMBOL(drm_dp_cec_set_edid);
>>   */
>>  void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
>>  {
>> +	/* No transfer function was set, so not a DP connector */
>> +	if (!aux->transfer)
>> +		return;
>> +
>>  	cancel_delayed_work_sync(&aux->cec.unregister_work);
>>  
>>  	mutex_lock(&aux->cec.lock);
>> @@ -404,6 +416,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux *aux,
>> const char *name,
>>  				   struct device *parent)
>>  {
>>  	WARN_ON(aux->cec.adap);
>> +	if (WARN_ON(!aux->transfer))
>> +		return;
>>  	aux->cec.name = name;
>>  	aux->cec.parent = parent;
>>  	INIT_DELAYED_WORK(&aux->cec.unregister_work,
> 

Re: [PATCH (repost) 3/5] drm_dp_mst_topology: fix broken drm_dp_sideband_parse_remote_dpcd_read()

[Lyude Paul]

On Mon, 2018-08-20 at 22:43 +0200, Hans Verkuil wrote:
> On 08/20/2018 08:59 PM, Lyude Paul wrote:
> > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > 
> > We really need to add support for using this into the MST helpers. A good
> > way to
> > test this would probably be to hook up an aux device to the DP AUX adapters
> > we
> > create for each MST topology
> 
> If you are interested, I have code for that in my MST test branch:
> 
> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=cec-nv-amd-mst
> 
> It's the "drm_dp_mst_topology: use correct AUX channel" patch.
> 
> I don't have plans to post this patch since CEC for MST isn't working
> (still trying to figure out why not), but you are free to pick it up
> if you want.
Maybe someday but don't count on it yet! I've got a lot of stuff on my plate atm
:)

> 
> Regards,
> 
> 	Hans
> 
> > 
> > On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
> > > From: Hans Verkuil <hans.verkuil@cisco.com>
> > > 
> > > When parsing the reply of a DP_REMOTE_DPCD_READ DPCD command the
> > > result is wrong due to a missing idx increment.
> > > 
> > > This was never noticed since DP_REMOTE_DPCD_READ is currently not
> > > used, but if you enable it, then it is all wrong.
> > > 
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 7780567aa669..5ff1d79b86c4 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -439,6 +439,7 @@ static bool
> > > drm_dp_sideband_parse_remote_dpcd_read(struct
> > > drm_dp_sideband_msg_rx
> > >  	if (idx > raw->curlen)
> > >  		goto fail_len;
> > >  	repmsg->u.remote_dpcd_read_ack.num_bytes = raw->msg[idx];
> > > +	idx++;
> > >  	if (idx > raw->curlen)
> > >  		goto fail_len;
> > >  
> 
> 

Re: [PATCH (repost) 1/5] drm_dp_cec: check that aux has a transfer function

[Lyude Paul]

On Mon, 2018-08-20 at 22:47 +0200, Hans Verkuil wrote:
> On 08/20/2018 08:51 PM, Lyude Paul wrote:
> > On Fri, 2018-08-17 at 16:11 +0200, Hans Verkuil wrote:
> > > From: Hans Verkuil <hans.verkuil@cisco.com>
> > > 
> > > If aux->transfer == NULL, then just return without doing
> > > anything. In that case the function is likely called for
> > > a non-(e)DP connector.
> > > 
> > > This never happened for the i915 driver, but the nouveau and amdgpu
> > > drivers need this check.
> > 
> > Could you give a backtrace from where you're hitting this issue with nouveau
> > and
> > amdgpu? It doesn't make a whole ton of sense to have connectors registering
> > DP
> > aux busses if they aren't actually DP, that should probably just be fixed...
> 
Agh, nouveau and amdgpu making questionable decisions :s... but I suppose that
makes sense.

Reviewed-by: Lyude Paul <lyude@redhat.com>

> The difference between the i915 driver and the nouveau (and amdgpu) driver is
> that in the i915 driver the drm_dp_cec_set_edid/unset_edid/irq functions are
> called from code that is exclusively for DisplayPort connectors.
> 
> For nouveau/amdgpu they are called from code that is shared between
> DisplayPort
> and HDMI, so aux->transfer may be NULL.
> 
> Rather than either testing for the connector type or for a non-NULL aux-
> >transfer
> every time I call a drm_dp_cec_* function, it is better to just test for
> aux->transfer in the drm_dp_cec_* functions themselves. It's more robust.
> 
> So there isn't a bug or anything like that, it's just so that these drm_dp_cec
> functions can handle a slightly different driver design safely.
> 
> The registration and unregistration of the cec devices is always DP specific,
> and an attempt to register a cec device for a non-DP connector will now fail
> with a WARN_ON.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > > 
> > > The alternative would be to add this check in those drivers before
> > > every drm_dp_cec call, but it makes sense to check it in the
> > > drm_dp_cec functions to prevent a kernel oops.
> > > 
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_cec.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_cec.c b/drivers/gpu/drm/drm_dp_cec.c
> > > index 988513346e9c..1407b13a8d5d 100644
> > > --- a/drivers/gpu/drm/drm_dp_cec.c
> > > +++ b/drivers/gpu/drm/drm_dp_cec.c
> > > @@ -238,6 +238,10 @@ void drm_dp_cec_irq(struct drm_dp_aux *aux)
> > >  	u8 cec_irq;
> > >  	int ret;
> > >  
> > > +	/* No transfer function was set, so not a DP connector */
> > > +	if (!aux->transfer)
> > > +		return;
> > > +
> > >  	mutex_lock(&aux->cec.lock);
> > >  	if (!aux->cec.adap)
> > >  		goto unlock;
> > > @@ -293,6 +297,10 @@ void drm_dp_cec_set_edid(struct drm_dp_aux *aux,
> > > const
> > > struct edid *edid)
> > >  	unsigned int num_las = 1;
> > >  	u8 cap;
> > >  
> > > +	/* No transfer function was set, so not a DP connector */
> > > +	if (!aux->transfer)
> > > +		return;
> > > +
> > >  #ifndef CONFIG_MEDIA_CEC_RC
> > >  	/*
> > >  	 * CEC_CAP_RC is part of CEC_CAP_DEFAULTS, but it is stripped by
> > > @@ -361,6 +369,10 @@ EXPORT_SYMBOL(drm_dp_cec_set_edid);
> > >   */
> > >  void drm_dp_cec_unset_edid(struct drm_dp_aux *aux)
> > >  {
> > > +	/* No transfer function was set, so not a DP connector */
> > > +	if (!aux->transfer)
> > > +		return;
> > > +
> > >  	cancel_delayed_work_sync(&aux->cec.unregister_work);
> > >  
> > >  	mutex_lock(&aux->cec.lock);
> > > @@ -404,6 +416,8 @@ void drm_dp_cec_register_connector(struct drm_dp_aux
> > > *aux,
> > > const char *name,
> > >  				   struct device *parent)
> > >  {
> > >  	WARN_ON(aux->cec.adap);
> > > +	if (WARN_ON(!aux->transfer))
> > > +		return;
> > >  	aux->cec.name = name;
> > >  	aux->cec.parent = parent;
> > >  	INIT_DELAYED_WORK(&aux->cec.unregister_work,
> 
> 

Re: [PATCH (repost) 5/5] drm/amdgpu: add DisplayPort CEC-Tunneling-over-AUX support

[Harry Wentland]

On 2018-08-17 10:11 AM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Add DisplayPort CEC-Tunneling-over-AUX support to amdgpu.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 13 +++++++++++--
>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c |  2 ++
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> 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 34f34823bab5..77898c95bef6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -898,6 +898,7 @@ amdgpu_dm_update_connector_after_detect(struct amdgpu_dm_connector *aconnector)
>  		aconnector->dc_sink = sink;
>  		if (sink->dc_edid.length == 0) {
>  			aconnector->edid = NULL;
> +			drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
>  		} else {
>  			aconnector->edid =
>  				(struct edid *) sink->dc_edid.raw_edid;
> @@ -905,10 +906,13 @@ amdgpu_dm_update_connector_after_detect(struct amdgpu_dm_connector *aconnector)
>  
>  			drm_connector_update_edid_property(connector,
>  					aconnector->edid);
> +			drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
> +					    aconnector->edid);
>  		}
>  		amdgpu_dm_add_sink_to_freesync_module(connector, aconnector->edid);
>  
>  	} else {
> +		drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
>  		amdgpu_dm_remove_sink_from_freesync_module(connector);
>  		drm_connector_update_edid_property(connector, NULL);
>  		aconnector->num_modes = 0;
> @@ -1059,12 +1063,16 @@ static void handle_hpd_rx_irq(void *param)
>  			drm_kms_helper_hotplug_event(dev);
>  		}
>  	}
> +
>  	if ((dc_link->cur_link_settings.lane_count != LANE_COUNT_UNKNOWN) ||
> -	    (dc_link->type == dc_connection_mst_branch))
> +	    (dc_link->type == dc_connection_mst_branch)) {
>  		dm_handle_hpd_rx_irq(aconnector);
> +	}

These lines don't really add anything functional.

Either way, this patch is
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

>  
> -	if (dc_link->type != dc_connection_mst_branch)
> +	if (dc_link->type != dc_connection_mst_branch) {
> +		drm_dp_cec_irq(&aconnector->dm_dp_aux.aux);
>  		mutex_unlock(&aconnector->hpd_lock);
> +	}
>  }
>  
>  static void register_hpd_handlers(struct amdgpu_device *adev)
> @@ -2732,6 +2740,7 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
>  		dm->backlight_dev = NULL;
>  	}
>  #endif
> +	drm_dp_cec_unregister_connector(&aconnector->dm_dp_aux.aux);
>  	drm_connector_unregister(connector);
>  	drm_connector_cleanup(connector);
>  	kfree(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 9a300732ba37..18a3a6e5ffa0 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
> @@ -496,6 +496,8 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
>  	aconnector->dm_dp_aux.ddc_service = aconnector->dc_link->ddc;
>  
>  	drm_dp_aux_register(&aconnector->dm_dp_aux.aux);
> +	drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> +				      aconnector->base.name, dm->adev->dev);
>  	aconnector->mst_mgr.cbs = &dm_mst_cbs;
>  	drm_dp_mst_topology_mgr_init(
>  		&aconnector->mst_mgr,
> 

Re: [PATCH (repost) 5/5] drm/amdgpu: add DisplayPort CEC-Tunneling-over-AUX support

[Hans Verkuil]

On 08/23/2018 08:38 PM, Harry Wentland wrote:
> On 2018-08-17 10:11 AM, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Add DisplayPort CEC-Tunneling-over-AUX support to amdgpu.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>> ---
>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 13 +++++++++++--
>>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c |  2 ++
>>  2 files changed, 13 insertions(+), 2 deletions(-)
>>
>> 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 34f34823bab5..77898c95bef6 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -898,6 +898,7 @@ amdgpu_dm_update_connector_after_detect(struct amdgpu_dm_connector *aconnector)
>>  		aconnector->dc_sink = sink;
>>  		if (sink->dc_edid.length == 0) {
>>  			aconnector->edid = NULL;
>> +			drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
>>  		} else {
>>  			aconnector->edid =
>>  				(struct edid *) sink->dc_edid.raw_edid;
>> @@ -905,10 +906,13 @@ amdgpu_dm_update_connector_after_detect(struct amdgpu_dm_connector *aconnector)
>>  
>>  			drm_connector_update_edid_property(connector,
>>  					aconnector->edid);
>> +			drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
>> +					    aconnector->edid);
>>  		}
>>  		amdgpu_dm_add_sink_to_freesync_module(connector, aconnector->edid);
>>  
>>  	} else {
>> +		drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
>>  		amdgpu_dm_remove_sink_from_freesync_module(connector);
>>  		drm_connector_update_edid_property(connector, NULL);
>>  		aconnector->num_modes = 0;
>> @@ -1059,12 +1063,16 @@ static void handle_hpd_rx_irq(void *param)
>>  			drm_kms_helper_hotplug_event(dev);
>>  		}
>>  	}
>> +
>>  	if ((dc_link->cur_link_settings.lane_count != LANE_COUNT_UNKNOWN) ||
>> -	    (dc_link->type == dc_connection_mst_branch))
>> +	    (dc_link->type == dc_connection_mst_branch)) {
>>  		dm_handle_hpd_rx_irq(aconnector);
>> +	}
> 
> These lines don't really add anything functional.

Oops, a left-over from debugging code. I'll remove this 'change' and post a v2
with all the Acks/reviewed-bys.

Any idea who would typically merge a patch series like this?

Regards,

	Hans

> 
> Either way, this patch is
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> 
> Harry
> 
>>  
>> -	if (dc_link->type != dc_connection_mst_branch)
>> +	if (dc_link->type != dc_connection_mst_branch) {
>> +		drm_dp_cec_irq(&aconnector->dm_dp_aux.aux);
>>  		mutex_unlock(&aconnector->hpd_lock);
>> +	}
>>  }
>>  
>>  static void register_hpd_handlers(struct amdgpu_device *adev)
>> @@ -2732,6 +2740,7 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
>>  		dm->backlight_dev = NULL;
>>  	}
>>  #endif
>> +	drm_dp_cec_unregister_connector(&aconnector->dm_dp_aux.aux);
>>  	drm_connector_unregister(connector);
>>  	drm_connector_cleanup(connector);
>>  	kfree(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 9a300732ba37..18a3a6e5ffa0 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
>> @@ -496,6 +496,8 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
>>  	aconnector->dm_dp_aux.ddc_service = aconnector->dc_link->ddc;
>>  
>>  	drm_dp_aux_register(&aconnector->dm_dp_aux.aux);
>> +	drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
>> +				      aconnector->base.name, dm->adev->dev);
>>  	aconnector->mst_mgr.cbs = &dm_mst_cbs;
>>  	drm_dp_mst_topology_mgr_init(
>>  		&aconnector->mst_mgr,
>>

Re: [PATCH (repost) 5/5] drm/amdgpu: add DisplayPort CEC-Tunneling-over-AUX support

[Alex Deucher]

On Fri, Aug 24, 2018 at 3:20 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 08/23/2018 08:38 PM, Harry Wentland wrote:
> > On 2018-08-17 10:11 AM, Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> Add DisplayPort CEC-Tunneling-over-AUX support to amdgpu.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> Acked-by: Alex Deucher <alexander.deucher@amd.com>
> >> ---
> >>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 13 +++++++++++--
> >>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c |  2 ++
> >>  2 files changed, 13 insertions(+), 2 deletions(-)
> >>
> >> 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 34f34823bab5..77898c95bef6 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> @@ -898,6 +898,7 @@ amdgpu_dm_update_connector_after_detect(struct amdgpu_dm_connector *aconnector)
> >>              aconnector->dc_sink = sink;
> >>              if (sink->dc_edid.length == 0) {
> >>                      aconnector->edid = NULL;
> >> +                    drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
> >>              } else {
> >>                      aconnector->edid =
> >>                              (struct edid *) sink->dc_edid.raw_edid;
> >> @@ -905,10 +906,13 @@ amdgpu_dm_update_connector_after_detect(struct amdgpu_dm_connector *aconnector)
> >>
> >>                      drm_connector_update_edid_property(connector,
> >>                                      aconnector->edid);
> >> +                    drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
> >> +                                        aconnector->edid);
> >>              }
> >>              amdgpu_dm_add_sink_to_freesync_module(connector, aconnector->edid);
> >>
> >>      } else {
> >> +            drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
> >>              amdgpu_dm_remove_sink_from_freesync_module(connector);
> >>              drm_connector_update_edid_property(connector, NULL);
> >>              aconnector->num_modes = 0;
> >> @@ -1059,12 +1063,16 @@ static void handle_hpd_rx_irq(void *param)
> >>                      drm_kms_helper_hotplug_event(dev);
> >>              }
> >>      }
> >> +
> >>      if ((dc_link->cur_link_settings.lane_count != LANE_COUNT_UNKNOWN) ||
> >> -        (dc_link->type == dc_connection_mst_branch))
> >> +        (dc_link->type == dc_connection_mst_branch)) {
> >>              dm_handle_hpd_rx_irq(aconnector);
> >> +    }
> >
> > These lines don't really add anything functional.
>
> Oops, a left-over from debugging code. I'll remove this 'change' and post a v2
> with all the Acks/reviewed-bys.
>
> Any idea who would typically merge a patch series like this?

I (or anyone else with drm-misc rights) can push them for you, however
drm-misc is a committer tree so if you'd like access to apply patches
yourself, you could do that too.  Request access here:
https://www.freedesktop.org/wiki/AccountRequests/

Alex

>
> Regards,
>
>         Hans
>
> >
> > Either way, this patch is
> > Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> >
> > Harry
> >
> >>
> >> -    if (dc_link->type != dc_connection_mst_branch)
> >> +    if (dc_link->type != dc_connection_mst_branch) {
> >> +            drm_dp_cec_irq(&aconnector->dm_dp_aux.aux);
> >>              mutex_unlock(&aconnector->hpd_lock);
> >> +    }
> >>  }
> >>
> >>  static void register_hpd_handlers(struct amdgpu_device *adev)
> >> @@ -2732,6 +2740,7 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
> >>              dm->backlight_dev = NULL;
> >>      }
> >>  #endif
> >> +    drm_dp_cec_unregister_connector(&aconnector->dm_dp_aux.aux);
> >>      drm_connector_unregister(connector);
> >>      drm_connector_cleanup(connector);
> >>      kfree(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 9a300732ba37..18a3a6e5ffa0 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
> >> @@ -496,6 +496,8 @@ void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
> >>      aconnector->dm_dp_aux.ddc_service = aconnector->dc_link->ddc;
> >>
> >>      drm_dp_aux_register(&aconnector->dm_dp_aux.aux);
> >> +    drm_dp_cec_register_connector(&aconnector->dm_dp_aux.aux,
> >> +                                  aconnector->base.name, dm->adev->dev);
> >>      aconnector->mst_mgr.cbs = &dm_mst_cbs;
> >>      drm_dp_mst_topology_mgr_init(
> >>              &aconnector->mst_mgr,
> >>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH (repost) 5/5] drm/amdgpu: add DisplayPort CEC-Tunneling-over-AUX support

[Hans Verkuil]

On 08/24/2018 04:59 PM, Alex Deucher wrote:
> On Fri, Aug 24, 2018 at 3:20 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 08/23/2018 08:38 PM, Harry Wentland wrote:
>>> On 2018-08-17 10:11 AM, Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> Add DisplayPort CEC-Tunneling-over-AUX support to amdgpu.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> Acked-by: Alex Deucher <alexander.deucher@amd.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 13 +++++++++++--
>>>>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c |  2 ++
>>>>  2 files changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> 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 34f34823bab5..77898c95bef6 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -898,6 +898,7 @@ amdgpu_dm_update_connector_after_detect(struct amdgpu_dm_connector *aconnector)
>>>>              aconnector->dc_sink = sink;
>>>>              if (sink->dc_edid.length == 0) {
>>>>                      aconnector->edid = NULL;
>>>> +                    drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
>>>>              } else {
>>>>                      aconnector->edid =
>>>>                              (struct edid *) sink->dc_edid.raw_edid;
>>>> @@ -905,10 +906,13 @@ amdgpu_dm_update_connector_after_detect(struct amdgpu_dm_connector *aconnector)
>>>>
>>>>                      drm_connector_update_edid_property(connector,
>>>>                                      aconnector->edid);
>>>> +                    drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
>>>> +                                        aconnector->edid);
>>>>              }
>>>>              amdgpu_dm_add_sink_to_freesync_module(connector, aconnector->edid);
>>>>
>>>>      } else {
>>>> +            drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
>>>>              amdgpu_dm_remove_sink_from_freesync_module(connector);
>>>>              drm_connector_update_edid_property(connector, NULL);
>>>>              aconnector->num_modes = 0;
>>>> @@ -1059,12 +1063,16 @@ static void handle_hpd_rx_irq(void *param)
>>>>                      drm_kms_helper_hotplug_event(dev);
>>>>              }
>>>>      }
>>>> +
>>>>      if ((dc_link->cur_link_settings.lane_count != LANE_COUNT_UNKNOWN) ||
>>>> -        (dc_link->type == dc_connection_mst_branch))
>>>> +        (dc_link->type == dc_connection_mst_branch)) {
>>>>              dm_handle_hpd_rx_irq(aconnector);
>>>> +    }
>>>
>>> These lines don't really add anything functional.
>>
>> Oops, a left-over from debugging code. I'll remove this 'change' and post a v2
>> with all the Acks/reviewed-bys.
>>
>> Any idea who would typically merge a patch series like this?
> 
> I (or anyone else with drm-misc rights) can push them for you, however
> drm-misc is a committer tree so if you'd like access to apply patches
> yourself, you could do that too.  Request access here:
> https://www.freedesktop.org/wiki/AccountRequests/

OK, I pushed this series to drm-next. It's the first time I'm using dim & drm-misc
so let me know if I did anything silly.

Regards,

	Hans