[PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr

[PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr

[Wayne Lin]

[Why]
While disabling mst topology manager in
drm_dp_mst_topology_mgr_set_mst(), now just reset the mgr->payloads
but doesn't handle the mgr->proposed_vcpis.

[How]
Remove mgr->proposed_vcpis to NULL.

Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index ae5809a1f19a..81e92b260d7a 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3386,6 +3386,7 @@ static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8  dp_link_count)
 int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool mst_state)
 {
 	int ret = 0;
+	int i = 0;
 	struct drm_dp_mst_branch *mstb = NULL;
 
 	mutex_lock(&mgr->lock);
@@ -3446,10 +3447,21 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 		/* this can fail if the device is gone */
 		drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0);
 		ret = 0;
+		mutex_lock(&mgr->payload_lock);
 		memset(mgr->payloads, 0, mgr->max_payloads * sizeof(struct drm_dp_payload));
 		mgr->payload_mask = 0;
 		set_bit(0, &mgr->payload_mask);
+		for (i = 0; i < mgr->max_payloads; i++) {
+			struct drm_dp_vcpi *tmp_vcpi = mgr->proposed_vcpis[i];
+
+			if (tmp_vcpi) {
+				tmp_vcpi->vcpi = 0;
+				tmp_vcpi->num_slots = 0;
+			}
+			mgr->proposed_vcpis[i] = NULL;
+		}
 		mgr->vcpi_mask = 0;
+		mutex_unlock(&mgr->payload_lock);
 	}
 
 out_unlock:
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr

[Wayne Lin]

[Why]
While disabling mst topology manager in
drm_dp_mst_topology_mgr_set_mst(), now just reset the mgr->payloads
but doesn't handle the mgr->proposed_vcpis.

[How]
Remove mgr->proposed_vcpis to NULL.

Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index ae5809a1f19a..81e92b260d7a 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3386,6 +3386,7 @@ static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8  dp_link_count)
 int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool mst_state)
 {
 	int ret = 0;
+	int i = 0;
 	struct drm_dp_mst_branch *mstb = NULL;
 
 	mutex_lock(&mgr->lock);
@@ -3446,10 +3447,21 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 		/* this can fail if the device is gone */
 		drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0);
 		ret = 0;
+		mutex_lock(&mgr->payload_lock);
 		memset(mgr->payloads, 0, mgr->max_payloads * sizeof(struct drm_dp_payload));
 		mgr->payload_mask = 0;
 		set_bit(0, &mgr->payload_mask);
+		for (i = 0; i < mgr->max_payloads; i++) {
+			struct drm_dp_vcpi *tmp_vcpi = mgr->proposed_vcpis[i];
+
+			if (tmp_vcpi) {
+				tmp_vcpi->vcpi = 0;
+				tmp_vcpi->num_slots = 0;
+			}
+			mgr->proposed_vcpis[i] = NULL;
+		}
 		mgr->vcpi_mask = 0;
+		mutex_unlock(&mgr->payload_lock);
 	}
 
 out_unlock:
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr

[Wayne Lin]

[Why]
While disabling mst topology manager in
drm_dp_mst_topology_mgr_set_mst(), now just reset the mgr->payloads
but doesn't handle the mgr->proposed_vcpis.

[How]
Remove mgr->proposed_vcpis to NULL.

Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index ae5809a1f19a..81e92b260d7a 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3386,6 +3386,7 @@ static int drm_dp_get_vc_payload_bw(u8 dp_link_bw, u8  dp_link_count)
 int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool mst_state)
 {
 	int ret = 0;
+	int i = 0;
 	struct drm_dp_mst_branch *mstb = NULL;
 
 	mutex_lock(&mgr->lock);
@@ -3446,10 +3447,21 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 		/* this can fail if the device is gone */
 		drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0);
 		ret = 0;
+		mutex_lock(&mgr->payload_lock);
 		memset(mgr->payloads, 0, mgr->max_payloads * sizeof(struct drm_dp_payload));
 		mgr->payload_mask = 0;
 		set_bit(0, &mgr->payload_mask);
+		for (i = 0; i < mgr->max_payloads; i++) {
+			struct drm_dp_vcpi *tmp_vcpi = mgr->proposed_vcpis[i];
+
+			if (tmp_vcpi) {
+				tmp_vcpi->vcpi = 0;
+				tmp_vcpi->num_slots = 0;
+			}
+			mgr->proposed_vcpis[i] = NULL;
+		}
 		mgr->vcpi_mask = 0;
+		mutex_unlock(&mgr->payload_lock);
 	}
 
 out_unlock:
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr

[Lyude Paul]

I'm, not entirely sure what this patch is trying to accomplish. I'm guessing
maybe we're leaving stale VCPI allocations from the previous topology
enablement and then somehow trying to use those again when allocating
payloads? The patch looks correct at least.

If this patch is fixing an issue, such as displays not turning on with amdgpu,
I'd definitely mention it in more detail here and Cc to stable if applicable.
Also, one nitpick below:

On Mon, 2019-12-02 at 11:57 +0800, Wayne Lin wrote:
> [Why]
> While disabling mst topology manager in
> drm_dp_mst_topology_mgr_set_mst(), now just reset the mgr->payloads
> but doesn't handle the mgr->proposed_vcpis.
> 
> [How]
> Remove mgr->proposed_vcpis to NULL.
> 
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index ae5809a1f19a..81e92b260d7a 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3386,6 +3386,7 @@ static int drm_dp_get_vc_payload_bw(u8 dp_link_bw,
> u8  dp_link_count)
>  int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr,
> bool mst_state)
>  {
>  	int ret = 0;
> +	int i = 0;
>  	struct drm_dp_mst_branch *mstb = NULL;
>  
>  	mutex_lock(&mgr->lock);
> @@ -3446,10 +3447,21 @@ int drm_dp_mst_topology_mgr_set_mst(struct
> drm_dp_mst_topology_mgr *mgr, bool ms
>  		/* this can fail if the device is gone */
>  		drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0);
>  		ret = 0;
> +		mutex_lock(&mgr->payload_lock);
>  		memset(mgr->payloads, 0, mgr->max_payloads * sizeof(struct
> drm_dp_payload));
>  		mgr->payload_mask = 0;
>  		set_bit(0, &mgr->payload_mask);
> +		for (i = 0; i < mgr->max_payloads; i++) {
> +			struct drm_dp_vcpi *tmp_vcpi = mgr->proposed_vcpis[i];
> +
> +			if (tmp_vcpi) {
> +				tmp_vcpi->vcpi = 0;
> +				tmp_vcpi->num_slots = 0;
> +			}
> +			mgr->proposed_vcpis[i] = NULL;
> +		}
>  		mgr->vcpi_mask = 0;
> +		mutex_unlock(&mgr->payload_lock);

bikeshed: I'd just rename tmp_vcpi here to vcpi, but I'll leave that up to you
>  	}
>  
>  out_unlock:
-- 
Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr

[Lyude Paul]

I'm, not entirely sure what this patch is trying to accomplish. I'm guessing
maybe we're leaving stale VCPI allocations from the previous topology
enablement and then somehow trying to use those again when allocating
payloads? The patch looks correct at least.

If this patch is fixing an issue, such as displays not turning on with amdgpu,
I'd definitely mention it in more detail here and Cc to stable if applicable.
Also, one nitpick below:

On Mon, 2019-12-02 at 11:57 +0800, Wayne Lin wrote:
> [Why]
> While disabling mst topology manager in
> drm_dp_mst_topology_mgr_set_mst(), now just reset the mgr->payloads
> but doesn't handle the mgr->proposed_vcpis.
> 
> [How]
> Remove mgr->proposed_vcpis to NULL.
> 
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index ae5809a1f19a..81e92b260d7a 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3386,6 +3386,7 @@ static int drm_dp_get_vc_payload_bw(u8 dp_link_bw,
> u8  dp_link_count)
>  int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr,
> bool mst_state)
>  {
>  	int ret = 0;
> +	int i = 0;
>  	struct drm_dp_mst_branch *mstb = NULL;
>  
>  	mutex_lock(&mgr->lock);
> @@ -3446,10 +3447,21 @@ int drm_dp_mst_topology_mgr_set_mst(struct
> drm_dp_mst_topology_mgr *mgr, bool ms
>  		/* this can fail if the device is gone */
>  		drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0);
>  		ret = 0;
> +		mutex_lock(&mgr->payload_lock);
>  		memset(mgr->payloads, 0, mgr->max_payloads * sizeof(struct
> drm_dp_payload));
>  		mgr->payload_mask = 0;
>  		set_bit(0, &mgr->payload_mask);
> +		for (i = 0; i < mgr->max_payloads; i++) {
> +			struct drm_dp_vcpi *tmp_vcpi = mgr->proposed_vcpis[i];
> +
> +			if (tmp_vcpi) {
> +				tmp_vcpi->vcpi = 0;
> +				tmp_vcpi->num_slots = 0;
> +			}
> +			mgr->proposed_vcpis[i] = NULL;
> +		}
>  		mgr->vcpi_mask = 0;
> +		mutex_unlock(&mgr->payload_lock);

bikeshed: I'd just rename tmp_vcpi here to vcpi, but I'll leave that up to you
>  	}
>  
>  out_unlock:
-- 
Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr

[Lyude Paul]

I'm, not entirely sure what this patch is trying to accomplish. I'm guessing
maybe we're leaving stale VCPI allocations from the previous topology
enablement and then somehow trying to use those again when allocating
payloads? The patch looks correct at least.

If this patch is fixing an issue, such as displays not turning on with amdgpu,
I'd definitely mention it in more detail here and Cc to stable if applicable.
Also, one nitpick below:

On Mon, 2019-12-02 at 11:57 +0800, Wayne Lin wrote:
> [Why]
> While disabling mst topology manager in
> drm_dp_mst_topology_mgr_set_mst(), now just reset the mgr->payloads
> but doesn't handle the mgr->proposed_vcpis.
> 
> [How]
> Remove mgr->proposed_vcpis to NULL.
> 
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index ae5809a1f19a..81e92b260d7a 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3386,6 +3386,7 @@ static int drm_dp_get_vc_payload_bw(u8 dp_link_bw,
> u8  dp_link_count)
>  int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr,
> bool mst_state)
>  {
>  	int ret = 0;
> +	int i = 0;
>  	struct drm_dp_mst_branch *mstb = NULL;
>  
>  	mutex_lock(&mgr->lock);
> @@ -3446,10 +3447,21 @@ int drm_dp_mst_topology_mgr_set_mst(struct
> drm_dp_mst_topology_mgr *mgr, bool ms
>  		/* this can fail if the device is gone */
>  		drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0);
>  		ret = 0;
> +		mutex_lock(&mgr->payload_lock);
>  		memset(mgr->payloads, 0, mgr->max_payloads * sizeof(struct
> drm_dp_payload));
>  		mgr->payload_mask = 0;
>  		set_bit(0, &mgr->payload_mask);
> +		for (i = 0; i < mgr->max_payloads; i++) {
> +			struct drm_dp_vcpi *tmp_vcpi = mgr->proposed_vcpis[i];
> +
> +			if (tmp_vcpi) {
> +				tmp_vcpi->vcpi = 0;
> +				tmp_vcpi->num_slots = 0;
> +			}
> +			mgr->proposed_vcpis[i] = NULL;
> +		}
>  		mgr->vcpi_mask = 0;
> +		mutex_unlock(&mgr->payload_lock);

bikeshed: I'd just rename tmp_vcpi here to vcpi, but I'll leave that up to you
>  	}
>  
>  out_unlock:
-- 
Cheers,
	Lyude Paul

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr

[Lin, Wayne]

> -----Original Message-----
> From: Lyude Paul <lyude@redhat.com>
> Sent: Tuesday, December 3, 2019 8:03 AM
> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org;
> amd-gfx@lists.freedesktop.org
> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
> <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>
> Subject: Re: [PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr
> 
> I'm, not entirely sure what this patch is trying to accomplish. I'm guessing
> maybe we're leaving stale VCPI allocations from the previous topology
> enablement and then somehow trying to use those again when allocating
> payloads? The patch looks correct at least.
> 
Thanks for your time and the comment.

Yes, this patch is trying to address the problem you mentioned.
Once unplug a DP MST capable device, driver will call 
drm_dp_mst_topology_mgr_set_mst() to reset mgr->payloads but it doesn't
reset the mgr->proposed_vcpis. If it doesn't reset the proposed_vcpi, code will
fail at checking port validation once plug in MST device later. 
Once MST capable device plug in again and try to allocate payloads by calling
drm_dp_update_payload_part1(), this function will iterate over all proposed
virtual channels and check each port validation to see if the specified port is still
in the topology. Since there are stale VCPI allocations from the previous topology
enablement in proposed_vcpi[], code flow will fail and reurn EINVAL.

> If this patch is fixing an issue, such as displays not turning on with amdgpu, I'd
> definitely mention it in more detail here and Cc to stable if applicable.

Thanks for your comment. I will Cc to stable@vger.kernel.org and amend the 
message in more detail in next version.

> Also, one nitpick below:

Thanks, I'll modify it.
> 
> On Mon, 2019-12-02 at 11:57 +0800, Wayne Lin wrote:
> > [Why]
> > While disabling mst topology manager in
> > drm_dp_mst_topology_mgr_set_mst(), now just reset the mgr->payloads
> > but doesn't handle the mgr->proposed_vcpis.
> >
> > [How]
> > Remove mgr->proposed_vcpis to NULL.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index ae5809a1f19a..81e92b260d7a 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -3386,6 +3386,7 @@ static int drm_dp_get_vc_payload_bw(u8
> > dp_link_bw,
> > u8  dp_link_count)
> >  int drm_dp_mst_topology_mgr_set_mst(struct
> drm_dp_mst_topology_mgr
> > *mgr, bool mst_state)  {
> >  	int ret = 0;
> > +	int i = 0;
> >  	struct drm_dp_mst_branch *mstb = NULL;
> >
> >  	mutex_lock(&mgr->lock);
> > @@ -3446,10 +3447,21 @@ int drm_dp_mst_topology_mgr_set_mst(struct
> > drm_dp_mst_topology_mgr *mgr, bool ms
> >  		/* this can fail if the device is gone */
> >  		drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0);
> >  		ret = 0;
> > +		mutex_lock(&mgr->payload_lock);
> >  		memset(mgr->payloads, 0, mgr->max_payloads * sizeof(struct
> > drm_dp_payload));
> >  		mgr->payload_mask = 0;
> >  		set_bit(0, &mgr->payload_mask);
> > +		for (i = 0; i < mgr->max_payloads; i++) {
> > +			struct drm_dp_vcpi *tmp_vcpi = mgr->proposed_vcpis[i];
> > +
> > +			if (tmp_vcpi) {
> > +				tmp_vcpi->vcpi = 0;
> > +				tmp_vcpi->num_slots = 0;
> > +			}
> > +			mgr->proposed_vcpis[i] = NULL;
> > +		}
> >  		mgr->vcpi_mask = 0;
> > +		mutex_unlock(&mgr->payload_lock);
> 
> bikeshed: I'd just rename tmp_vcpi here to vcpi, but I'll leave that up to you
> >  	}
> >
> >  out_unlock:
> --
> Cheers,
> 	Lyude Paul
--
BR,
Wayne Lin

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr

[Lin, Wayne]

> -----Original Message-----
> From: Lyude Paul <lyude@redhat.com>
> Sent: Tuesday, December 3, 2019 8:03 AM
> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org;
> amd-gfx@lists.freedesktop.org
> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
> <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>
> Subject: Re: [PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr
> 
> I'm, not entirely sure what this patch is trying to accomplish. I'm guessing
> maybe we're leaving stale VCPI allocations from the previous topology
> enablement and then somehow trying to use those again when allocating
> payloads? The patch looks correct at least.
> 
Thanks for your time and the comment.

Yes, this patch is trying to address the problem you mentioned.
Once unplug a DP MST capable device, driver will call 
drm_dp_mst_topology_mgr_set_mst() to reset mgr->payloads but it doesn't
reset the mgr->proposed_vcpis. If it doesn't reset the proposed_vcpi, code will
fail at checking port validation once plug in MST device later. 
Once MST capable device plug in again and try to allocate payloads by calling
drm_dp_update_payload_part1(), this function will iterate over all proposed
virtual channels and check each port validation to see if the specified port is still
in the topology. Since there are stale VCPI allocations from the previous topology
enablement in proposed_vcpi[], code flow will fail and reurn EINVAL.

> If this patch is fixing an issue, such as displays not turning on with amdgpu, I'd
> definitely mention it in more detail here and Cc to stable if applicable.

Thanks for your comment. I will Cc to stable@vger.kernel.org and amend the 
message in more detail in next version.

> Also, one nitpick below:

Thanks, I'll modify it.
> 
> On Mon, 2019-12-02 at 11:57 +0800, Wayne Lin wrote:
> > [Why]
> > While disabling mst topology manager in
> > drm_dp_mst_topology_mgr_set_mst(), now just reset the mgr->payloads
> > but doesn't handle the mgr->proposed_vcpis.
> >
> > [How]
> > Remove mgr->proposed_vcpis to NULL.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index ae5809a1f19a..81e92b260d7a 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -3386,6 +3386,7 @@ static int drm_dp_get_vc_payload_bw(u8
> > dp_link_bw,
> > u8  dp_link_count)
> >  int drm_dp_mst_topology_mgr_set_mst(struct
> drm_dp_mst_topology_mgr
> > *mgr, bool mst_state)  {
> >  	int ret = 0;
> > +	int i = 0;
> >  	struct drm_dp_mst_branch *mstb = NULL;
> >
> >  	mutex_lock(&mgr->lock);
> > @@ -3446,10 +3447,21 @@ int drm_dp_mst_topology_mgr_set_mst(struct
> > drm_dp_mst_topology_mgr *mgr, bool ms
> >  		/* this can fail if the device is gone */
> >  		drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0);
> >  		ret = 0;
> > +		mutex_lock(&mgr->payload_lock);
> >  		memset(mgr->payloads, 0, mgr->max_payloads * sizeof(struct
> > drm_dp_payload));
> >  		mgr->payload_mask = 0;
> >  		set_bit(0, &mgr->payload_mask);
> > +		for (i = 0; i < mgr->max_payloads; i++) {
> > +			struct drm_dp_vcpi *tmp_vcpi = mgr->proposed_vcpis[i];
> > +
> > +			if (tmp_vcpi) {
> > +				tmp_vcpi->vcpi = 0;
> > +				tmp_vcpi->num_slots = 0;
> > +			}
> > +			mgr->proposed_vcpis[i] = NULL;
> > +		}
> >  		mgr->vcpi_mask = 0;
> > +		mutex_unlock(&mgr->payload_lock);
> 
> bikeshed: I'd just rename tmp_vcpi here to vcpi, but I'll leave that up to you
> >  	}
> >
> >  out_unlock:
> --
> Cheers,
> 	Lyude Paul
--
BR,
Wayne Lin

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr

[Lin, Wayne]

> -----Original Message-----
> From: Lyude Paul <lyude@redhat.com>
> Sent: Tuesday, December 3, 2019 8:03 AM
> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org;
> amd-gfx@lists.freedesktop.org
> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
> <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>
> Subject: Re: [PATCH] drm/dp_mst: Remove VCPI while disabling topology mgr
> 
> I'm, not entirely sure what this patch is trying to accomplish. I'm guessing
> maybe we're leaving stale VCPI allocations from the previous topology
> enablement and then somehow trying to use those again when allocating
> payloads? The patch looks correct at least.
> 
Thanks for your time and the comment.

Yes, this patch is trying to address the problem you mentioned.
Once unplug a DP MST capable device, driver will call 
drm_dp_mst_topology_mgr_set_mst() to reset mgr->payloads but it doesn't
reset the mgr->proposed_vcpis. If it doesn't reset the proposed_vcpi, code will
fail at checking port validation once plug in MST device later. 
Once MST capable device plug in again and try to allocate payloads by calling
drm_dp_update_payload_part1(), this function will iterate over all proposed
virtual channels and check each port validation to see if the specified port is still
in the topology. Since there are stale VCPI allocations from the previous topology
enablement in proposed_vcpi[], code flow will fail and reurn EINVAL.

> If this patch is fixing an issue, such as displays not turning on with amdgpu, I'd
> definitely mention it in more detail here and Cc to stable if applicable.

Thanks for your comment. I will Cc to stable@vger.kernel.org and amend the 
message in more detail in next version.

> Also, one nitpick below:

Thanks, I'll modify it.
> 
> On Mon, 2019-12-02 at 11:57 +0800, Wayne Lin wrote:
> > [Why]
> > While disabling mst topology manager in
> > drm_dp_mst_topology_mgr_set_mst(), now just reset the mgr->payloads
> > but doesn't handle the mgr->proposed_vcpis.
> >
> > [How]
> > Remove mgr->proposed_vcpis to NULL.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index ae5809a1f19a..81e92b260d7a 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -3386,6 +3386,7 @@ static int drm_dp_get_vc_payload_bw(u8
> > dp_link_bw,
> > u8  dp_link_count)
> >  int drm_dp_mst_topology_mgr_set_mst(struct
> drm_dp_mst_topology_mgr
> > *mgr, bool mst_state)  {
> >  	int ret = 0;
> > +	int i = 0;
> >  	struct drm_dp_mst_branch *mstb = NULL;
> >
> >  	mutex_lock(&mgr->lock);
> > @@ -3446,10 +3447,21 @@ int drm_dp_mst_topology_mgr_set_mst(struct
> > drm_dp_mst_topology_mgr *mgr, bool ms
> >  		/* this can fail if the device is gone */
> >  		drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0);
> >  		ret = 0;
> > +		mutex_lock(&mgr->payload_lock);
> >  		memset(mgr->payloads, 0, mgr->max_payloads * sizeof(struct
> > drm_dp_payload));
> >  		mgr->payload_mask = 0;
> >  		set_bit(0, &mgr->payload_mask);
> > +		for (i = 0; i < mgr->max_payloads; i++) {
> > +			struct drm_dp_vcpi *tmp_vcpi = mgr->proposed_vcpis[i];
> > +
> > +			if (tmp_vcpi) {
> > +				tmp_vcpi->vcpi = 0;
> > +				tmp_vcpi->num_slots = 0;
> > +			}
> > +			mgr->proposed_vcpis[i] = NULL;
> > +		}
> >  		mgr->vcpi_mask = 0;
> > +		mutex_unlock(&mgr->payload_lock);
> 
> bikeshed: I'd just rename tmp_vcpi here to vcpi, but I'll leave that up to you
> >  	}
> >
> >  out_unlock:
> --
> Cheers,
> 	Lyude Paul
--
BR,
Wayne Lin

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx