[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 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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/dp: Read the tx msg state once after checking for an event

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/dp: Read the tx msg state once after checking for an event
URL   : https://patchwork.freedesktop.org/series/24392/
State : success

== Summary ==

Series 24392v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/24392/revisions/1/mbox/

fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:435s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:516s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:495s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:487s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:422s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:416s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:498s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:469s
fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:581s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:577s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:473s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:509s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:441s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:538s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:410s
fi-bxt-t5700 failed to collect. IGT log at Patchwork_4692/fi-bxt-t5700/igt.log

e5217297e5c262bebbf2bfd9438c3561e2f0f065 drm-tip: 2017y-05m-13d-10h-05m-51s UTC integration manifest
055c37a drm/dp: Wait up all outstanding tx waiters
858ec25 drm/dp: Read the tx msg state once after checking for an event

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4692/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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