* [PATCH 1/2] drm/dp: Read the tx msg state once after checking for an event @ 2017-05-13 10:52 Chris Wilson 2017-05-13 10:52 ` [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters Chris Wilson ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Chris Wilson @ 2017-05-13 10:52 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters 2017-05-13 10:52 [PATCH 1/2] drm/dp: Read the tx msg state once after checking for an event Chris Wilson @ 2017-05-13 10:52 ` Chris Wilson 2017-05-13 11:06 ` Chris Wilson 2017-05-15 12:04 ` Daniel Vetter 2017-05-13 11:18 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/dp: Read the tx msg state once after checking for an event Patchwork 2017-05-15 12:02 ` [PATCH 1/2] " Daniel Vetter 2 siblings, 2 replies; 9+ messages in thread From: Chris Wilson @ 2017-05-13 10:52 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters 2017-05-13 10:52 ` [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters Chris Wilson @ 2017-05-13 11:06 ` Chris Wilson 2017-05-15 12:04 ` Daniel Vetter 1 sibling, 0 replies; 9+ messages in thread From: Chris Wilson @ 2017-05-13 11:06 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters 2017-05-13 10:52 ` [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters Chris Wilson 2017-05-13 11:06 ` Chris Wilson @ 2017-05-15 12:04 ` Daniel Vetter 2017-05-15 12:33 ` [Intel-gfx] " Ville Syrjälä 1 sibling, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2017-05-15 12:04 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, dri-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters 2017-05-15 12:04 ` Daniel Vetter @ 2017-05-15 12:33 ` Ville Syrjälä 2017-05-15 13:37 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Ville Syrjälä @ 2017-05-15 12:33 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, dri-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters 2017-05-15 12:33 ` [Intel-gfx] " Ville Syrjälä @ 2017-05-15 13:37 ` Daniel Vetter 2017-05-15 15:02 ` [Intel-gfx] " Jani Nikula 0 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2017-05-15 13:37 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, dri-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters 2017-05-15 13:37 ` Daniel Vetter @ 2017-05-15 15:02 ` Jani Nikula 0 siblings, 0 replies; 9+ messages in thread From: Jani Nikula @ 2017-05-15 15:02 UTC (permalink / raw) To: Daniel Vetter, Ville Syrjälä; +Cc: intel-gfx, dri-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/2] drm/dp: Read the tx msg state once after checking for an event 2017-05-13 10:52 [PATCH 1/2] drm/dp: Read the tx msg state once after checking for an event Chris Wilson 2017-05-13 10:52 ` [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters Chris Wilson @ 2017-05-13 11:18 ` Patchwork 2017-05-15 12:02 ` [PATCH 1/2] " Daniel Vetter 2 siblings, 0 replies; 9+ messages in thread From: Patchwork @ 2017-05-13 11:18 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/dp: Read the tx msg state once after checking for an event 2017-05-13 10:52 [PATCH 1/2] drm/dp: Read the tx msg state once after checking for an event Chris Wilson 2017-05-13 10:52 ` [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters Chris Wilson 2017-05-13 11:18 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/dp: Read the tx msg state once after checking for an event Patchwork @ 2017-05-15 12:02 ` Daniel Vetter 2 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2017-05-15 12:02 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, dri-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-05-15 15:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-13 10:52 [PATCH 1/2] drm/dp: Read the tx msg state once after checking for an event Chris Wilson 2017-05-13 10:52 ` [PATCH 2/2] drm/dp: Wait up all outstanding tx waiters Chris Wilson 2017-05-13 11:06 ` Chris Wilson 2017-05-15 12:04 ` Daniel Vetter 2017-05-15 12:33 ` [Intel-gfx] " Ville Syrjälä 2017-05-15 13:37 ` Daniel Vetter 2017-05-15 15:02 ` [Intel-gfx] " Jani Nikula 2017-05-13 11:18 ` ✓ Fi.CI.BAT: success for series starting with [1/2] drm/dp: Read the tx msg state once after checking for an event Patchwork 2017-05-15 12:02 ` [PATCH 1/2] " Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.