[PATCH 1/2] drm/dp: Read the tx msg state once after checking for an event

[PATCH 1/2] drm/dp: Read the tx msg state once after checking for an event

[Chris Wilson]

Both as an exercise to document that we are reading the state outside of
the appropriate mutex and to ensure that we only read the value once
before the multiple comparisons, use READ_ONCE.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index d1cbb9c8f806..3bdd314f02b1 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -737,16 +737,16 @@ static void drm_dp_mst_put_payload_id(struct drm_dp_mst_topology_mgr *mgr,
 static bool check_txmsg_state(struct drm_dp_mst_topology_mgr *mgr,
 			      struct drm_dp_sideband_msg_tx *txmsg)
 {
-	bool ret;
+	unsigned int state;
 
 	/*
 	 * All updates to txmsg->state are protected by mgr->qlock, and the two
 	 * cases we check here are terminal states. For those the barriers
 	 * provided by the wake_up/wait_event pair are enough.
 	 */
-	ret = (txmsg->state == DRM_DP_SIDEBAND_TX_RX ||
-	       txmsg->state == DRM_DP_SIDEBAND_TX_TIMEOUT);
-	return ret;
+	state = READ_ONCE(txmsg->state);
+	return (state == DRM_DP_SIDEBAND_TX_RX ||
+		state == DRM_DP_SIDEBAND_TX_TIMEOUT);
 }
 
 static int drm_dp_mst_wait_tx_reply(struct drm_dp_mst_branch *mstb,
-- 
2.11.0

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

[PATCH 2/2] drm/dp: Wait up all outstanding tx waiters

[Chris Wilson]

As we can have multiple tx in the queue, with individual waiters, make
sure that all are woken when any state changes (so that we are sure the
right owner of the txmsg is woken).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 3bdd314f02b1..222eb1a8549b 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -855,7 +855,7 @@ static void drm_dp_destroy_mst_branch_device(struct kref *kref)
 	mutex_unlock(&mstb->mgr->qlock);
 
 	if (wake_tx)
-		wake_up(&mstb->mgr->tx_waitq);
+		wake_up_all(&mstb->mgr->tx_waitq);
 
 	kref_put(kref, drm_dp_free_mst_branch_device);
 }
@@ -1510,7 +1510,7 @@ static void process_single_down_tx_qlock(struct drm_dp_mst_topology_mgr *mgr)
 		if (txmsg->seqno != -1)
 			txmsg->dst->tx_slots[txmsg->seqno] = NULL;
 		txmsg->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
-		wake_up(&mgr->tx_waitq);
+		wake_up_all(&mgr->tx_waitq);
 	}
 }
 
@@ -2258,7 +2258,7 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr)
 		mstb->tx_slots[slot] = NULL;
 		mutex_unlock(&mgr->qlock);
 
-		wake_up(&mgr->tx_waitq);
+		wake_up_all(&mgr->tx_waitq);
 	}
 	return ret;
 }
-- 
2.11.0

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

Re: [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters

[Chris Wilson]

subject s/Wait/Wake/

D'oh
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/dp: Read the tx msg state once after checking for an event

[Daniel Vetter]

On Sat, May 13, 2017 at 11:52:00AM +0100, Chris Wilson wrote:
> Both as an exercise to document that we are reading the state outside of
> the appropriate mutex and to ensure that we only read the value once
> before the multiple comparisons, use READ_ONCE.

I think gcc could also opt to re-evalute and re-load it, resulting to
diverging control flow and hilarity. At least I don't spot any protection
against that in the wait_for macros.

Applied to drm-misc, thanks.
-Daniel

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index d1cbb9c8f806..3bdd314f02b1 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -737,16 +737,16 @@ static void drm_dp_mst_put_payload_id(struct drm_dp_mst_topology_mgr *mgr,
>  static bool check_txmsg_state(struct drm_dp_mst_topology_mgr *mgr,
>  			      struct drm_dp_sideband_msg_tx *txmsg)
>  {
> -	bool ret;
> +	unsigned int state;
>  
>  	/*
>  	 * All updates to txmsg->state are protected by mgr->qlock, and the two
>  	 * cases we check here are terminal states. For those the barriers
>  	 * provided by the wake_up/wait_event pair are enough.
>  	 */
> -	ret = (txmsg->state == DRM_DP_SIDEBAND_TX_RX ||
> -	       txmsg->state == DRM_DP_SIDEBAND_TX_TIMEOUT);
> -	return ret;
> +	state = READ_ONCE(txmsg->state);
> +	return (state == DRM_DP_SIDEBAND_TX_RX ||
> +		state == DRM_DP_SIDEBAND_TX_TIMEOUT);
>  }
>  
>  static int drm_dp_mst_wait_tx_reply(struct drm_dp_mst_branch *mstb,
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters

[Daniel Vetter]

On Sat, May 13, 2017 at 11:52:01AM +0100, Chris Wilson wrote:
> As we can have multiple tx in the queue, with individual waiters, make
> sure that all are woken when any state changes (so that we are sure the
> right owner of the txmsg is woken).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I think in practice we need probe vs. the userspace dp aux interface (or
multiple userspace apps beating on this), and on multiple different mst
sinks, but better safe than sorry.

Applied, thanks.
-Daniel
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 3bdd314f02b1..222eb1a8549b 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -855,7 +855,7 @@ static void drm_dp_destroy_mst_branch_device(struct kref *kref)
>  	mutex_unlock(&mstb->mgr->qlock);
>  
>  	if (wake_tx)
> -		wake_up(&mstb->mgr->tx_waitq);
> +		wake_up_all(&mstb->mgr->tx_waitq);
>  
>  	kref_put(kref, drm_dp_free_mst_branch_device);
>  }
> @@ -1510,7 +1510,7 @@ static void process_single_down_tx_qlock(struct drm_dp_mst_topology_mgr *mgr)
>  		if (txmsg->seqno != -1)
>  			txmsg->dst->tx_slots[txmsg->seqno] = NULL;
>  		txmsg->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
> -		wake_up(&mgr->tx_waitq);
> +		wake_up_all(&mgr->tx_waitq);
>  	}
>  }
>  
> @@ -2258,7 +2258,7 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr)
>  		mstb->tx_slots[slot] = NULL;
>  		mutex_unlock(&mgr->qlock);
>  
> -		wake_up(&mgr->tx_waitq);
> +		wake_up_all(&mgr->tx_waitq);
>  	}
>  	return ret;
>  }
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters

[Ville Syrjälä]

On Mon, May 15, 2017 at 02:04:43PM +0200, Daniel Vetter wrote:
> On Sat, May 13, 2017 at 11:52:01AM +0100, Chris Wilson wrote:
> > As we can have multiple tx in the queue, with individual waiters, make
> > sure that all are woken when any state changes (so that we are sure the
> > right owner of the txmsg is woken).
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> I think in practice we need probe vs. the userspace dp aux interface (or
> multiple userspace apps beating on this), and on multiple different mst
> sinks, but better safe than sorry.

Someone has to figure out what I did wrong in my remote DPCD aux_dev
attempt before we can actually do that:

[1] git://github.com/vsyrjala/linux.git dp_mst_port_aux_dev

> 
> Applied, thanks.
> -Daniel
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 3bdd314f02b1..222eb1a8549b 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -855,7 +855,7 @@ static void drm_dp_destroy_mst_branch_device(struct kref *kref)
> >  	mutex_unlock(&mstb->mgr->qlock);
> >  
> >  	if (wake_tx)
> > -		wake_up(&mstb->mgr->tx_waitq);
> > +		wake_up_all(&mstb->mgr->tx_waitq);
> >  
> >  	kref_put(kref, drm_dp_free_mst_branch_device);
> >  }
> > @@ -1510,7 +1510,7 @@ static void process_single_down_tx_qlock(struct drm_dp_mst_topology_mgr *mgr)
> >  		if (txmsg->seqno != -1)
> >  			txmsg->dst->tx_slots[txmsg->seqno] = NULL;
> >  		txmsg->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
> > -		wake_up(&mgr->tx_waitq);
> > +		wake_up_all(&mgr->tx_waitq);
> >  	}
> >  }
> >  
> > @@ -2258,7 +2258,7 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr)
> >  		mstb->tx_slots[slot] = NULL;
> >  		mutex_unlock(&mgr->qlock);
> >  
> > -		wake_up(&mgr->tx_waitq);
> > +		wake_up_all(&mgr->tx_waitq);
> >  	}
> >  	return ret;
> >  }
> > -- 
> > 2.11.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters

[Daniel Vetter]

On Mon, May 15, 2017 at 03:33:12PM +0300, Ville Syrjälä wrote:
> On Mon, May 15, 2017 at 02:04:43PM +0200, Daniel Vetter wrote:
> > On Sat, May 13, 2017 at 11:52:01AM +0100, Chris Wilson wrote:
> > > As we can have multiple tx in the queue, with individual waiters, make
> > > sure that all are woken when any state changes (so that we are sure the
> > > right owner of the txmsg is woken).
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > I think in practice we need probe vs. the userspace dp aux interface (or
> > multiple userspace apps beating on this), and on multiple different mst
> > sinks, but better safe than sorry.
> 
> Someone has to figure out what I did wrong in my remote DPCD aux_dev
> attempt before we can actually do that:
> 
> [1] git://github.com/vsyrjala/linux.git dp_mst_port_aux_dev

Oh, I didn't realize we don't register the full dp aux for remotes. But we
do register the i2c for remotes, and that's good enough to blow up. i2c
dev nodes is also more likely to be used by userspace for real (through
the ddc tool).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters

[Jani Nikula]

On Mon, 15 May 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, May 15, 2017 at 03:33:12PM +0300, Ville Syrjälä wrote:
>> On Mon, May 15, 2017 at 02:04:43PM +0200, Daniel Vetter wrote:
>> > On Sat, May 13, 2017 at 11:52:01AM +0100, Chris Wilson wrote:
>> > > As we can have multiple tx in the queue, with individual waiters, make
>> > > sure that all are woken when any state changes (so that we are sure the
>> > > right owner of the txmsg is woken).
>> > > 
>> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > 
>> > I think in practice we need probe vs. the userspace dp aux interface (or
>> > multiple userspace apps beating on this), and on multiple different mst
>> > sinks, but better safe than sorry.
>> 
>> Someone has to figure out what I did wrong in my remote DPCD aux_dev
>> attempt before we can actually do that:
>> 
>> [1] git://github.com/vsyrjala/linux.git dp_mst_port_aux_dev
>
> Oh, I didn't realize we don't register the full dp aux for remotes. But we
> do register the i2c for remotes, and that's good enough to blow up. i2c
> dev nodes is also more likely to be used by userspace for real (through
> the ddc tool).

Related https://bugs.freedesktop.org/show_bug.cgi?id=100954

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel