Re: [Intel-gfx] [PATCH 2/7] drm/i915: Push i915_sw_fence_wait into the nonblocking atomic commit

2017-08-07 Thread Michel Thierry 

On 8/7/2017 8:33 AM, Daniel Vetter wrote:

On Thu, Aug 03, 2017 at 12:44:40PM -0700, Michel Thierry wrote:

On 7/20/2017 10:57 AM, Daniel Vetter wrote:

Blocking in a worker is ok, that's what the unbound_wq is for. And it
unifies the paths between the blocking and nonblocking commit, giving
me just one path where I have to implement the deadlock avoidance
trickery in the next patch.

I first tried to implement the following patch without this rework, but
force-completing i915_sw_fence creates some serious challenges around
properly cleaning things up. So wasn't a feasible short-term approach.
Another approach would be to simple keep track of all pending atomic
commit work items and manually queue them from the reset code. With the
caveat that double-queue in case we race with the i915_sw_fence must be
avoided. Given all that, taking the cost of a double schedule in atomic
for the short-term fix is the best approach, but can be changed in the
future of course.

v2: Amend commit message (Chris).

Reviewed-by: Maarten Lankhorst 
Cc: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Joonas Lahtinen 
Signed-off-by: Daniel Vetter 
---
   drivers/gpu/drm/i915/intel_display.c | 15 +++
   1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 995522e40ec1..f6bd6282d7f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12394,6 +12394,8 @@ static void intel_atomic_commit_tail(struct 
drm_atomic_state *state)
  unsigned crtc_vblank_mask = 0;
  int i;

+   i915_sw_fence_wait(_state->commit_ready);
+
  drm_atomic_helper_wait_for_dependencies(state);

  if (intel_state->modeset)
@@ -12561,10 +12563,7 @@ intel_atomic_commit_ready(struct i915_sw_fence *fence,

  switch (notify) {
  case FENCE_COMPLETE:
-   if (state->base.commit_work.func)
-   queue_work(system_unbound_wq, >base.commit_work);


I would add a small comment here, because later-on if someone has doubts
(and use git-blame), it won't be visible that something changed (the case
and break were added by the same commit).


Hm, not sure what comment I should put here? Suggestions? Only thing I
could come up with was

/* we do blocking waits in the worker, nothing to do here */

But not sure that adds the information you're looking for.


That sounds good to me, or maybe
"any blocking waits already handled in the worker"

But I think both are ok.

-Michel






  break;
-
  case FENCE_FREE:
  {
  struct intel_atomic_helper *helper =
@@ -12668,14 +12667,14 @@ static int intel_atomic_commit(struct drm_device *dev,
  }

  drm_atomic_state_get(state);
-   INIT_WORK(>commit_work,
- nonblock ? intel_atomic_commit_work : NULL);
+   INIT_WORK(>commit_work, intel_atomic_commit_work);

  i915_sw_fence_commit(_state->commit_ready);
-   if (!nonblock) {
-   i915_sw_fence_wait(_state->commit_ready);
+   if (nonblock)
+   queue_work(system_unbound_wq, >commit_work);
+   else
  intel_atomic_commit_tail(state);
-   }
+

  return 0;
   }


Reviewed-by: Michel Thierry 



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

Re: [Intel-gfx] [PATCH 2/7] drm/i915: Push i915_sw_fence_wait into the nonblocking atomic commit

2017-08-07 Thread Daniel Vetter 
On Thu, Aug 03, 2017 at 12:44:40PM -0700, Michel Thierry wrote:
> On 7/20/2017 10:57 AM, Daniel Vetter wrote:
> > Blocking in a worker is ok, that's what the unbound_wq is for. And it
> > unifies the paths between the blocking and nonblocking commit, giving
> > me just one path where I have to implement the deadlock avoidance
> > trickery in the next patch.
> > 
> > I first tried to implement the following patch without this rework, but
> > force-completing i915_sw_fence creates some serious challenges around
> > properly cleaning things up. So wasn't a feasible short-term approach.
> > Another approach would be to simple keep track of all pending atomic
> > commit work items and manually queue them from the reset code. With the
> > caveat that double-queue in case we race with the i915_sw_fence must be
> > avoided. Given all that, taking the cost of a double schedule in atomic
> > for the short-term fix is the best approach, but can be changed in the
> > future of course.
> > 
> > v2: Amend commit message (Chris).
> > 
> > Reviewed-by: Maarten Lankhorst 
> > Cc: Chris Wilson 
> > Cc: Mika Kuoppala 
> > Cc: Joonas Lahtinen 
> > Signed-off-by: Daniel Vetter 
> > ---
> >   drivers/gpu/drm/i915/intel_display.c | 15 +++
> >   1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 995522e40ec1..f6bd6282d7f7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12394,6 +12394,8 @@ static void intel_atomic_commit_tail(struct 
> > drm_atomic_state *state)
> >  unsigned crtc_vblank_mask = 0;
> >  int i;
> > 
> > +   i915_sw_fence_wait(_state->commit_ready);
> > +
> >  drm_atomic_helper_wait_for_dependencies(state);
> > 
> >  if (intel_state->modeset)
> > @@ -12561,10 +12563,7 @@ intel_atomic_commit_ready(struct i915_sw_fence 
> > *fence,
> > 
> >  switch (notify) {
> >  case FENCE_COMPLETE:
> > -   if (state->base.commit_work.func)
> > -   queue_work(system_unbound_wq, 
> > >base.commit_work);
> 
> I would add a small comment here, because later-on if someone has doubts
> (and use git-blame), it won't be visible that something changed (the case
> and break were added by the same commit).

Hm, not sure what comment I should put here? Suggestions? Only thing I
could come up with was

/* we do blocking waits in the worker, nothing to do here */

But not sure that adds the information you're looking for.
-Daniel

> 
> >  break;
> > -
> >  case FENCE_FREE:
> >  {
> >  struct intel_atomic_helper *helper =
> > @@ -12668,14 +12667,14 @@ static int intel_atomic_commit(struct drm_device 
> > *dev,
> >  }
> > 
> >  drm_atomic_state_get(state);
> > -   INIT_WORK(>commit_work,
> > - nonblock ? intel_atomic_commit_work : NULL);
> > +   INIT_WORK(>commit_work, intel_atomic_commit_work);
> > 
> >  i915_sw_fence_commit(_state->commit_ready);
> > -   if (!nonblock) {
> > -   i915_sw_fence_wait(_state->commit_ready);
> > +   if (nonblock)
> > +   queue_work(system_unbound_wq, >commit_work);
> > +   else
> >  intel_atomic_commit_tail(state);
> > -   }
> > +
> > 
> >  return 0;
> >   }
> 
> Reviewed-by: Michel Thierry 

-- 
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/7] drm/i915: Push i915_sw_fence_wait into the nonblocking atomic commit

2017-08-03 Thread Michel Thierry 

On 7/20/2017 10:57 AM, Daniel Vetter wrote:

Blocking in a worker is ok, that's what the unbound_wq is for. And it
unifies the paths between the blocking and nonblocking commit, giving
me just one path where I have to implement the deadlock avoidance
trickery in the next patch.

I first tried to implement the following patch without this rework, but
force-completing i915_sw_fence creates some serious challenges around
properly cleaning things up. So wasn't a feasible short-term approach.
Another approach would be to simple keep track of all pending atomic
commit work items and manually queue them from the reset code. With the
caveat that double-queue in case we race with the i915_sw_fence must be
avoided. Given all that, taking the cost of a double schedule in atomic
for the short-term fix is the best approach, but can be changed in the
future of course.

v2: Amend commit message (Chris).

Reviewed-by: Maarten Lankhorst 
Cc: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Joonas Lahtinen 
Signed-off-by: Daniel Vetter 
---
  drivers/gpu/drm/i915/intel_display.c | 15 +++
  1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 995522e40ec1..f6bd6282d7f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12394,6 +12394,8 @@ static void intel_atomic_commit_tail(struct 
drm_atomic_state *state)
 unsigned crtc_vblank_mask = 0;
 int i;

+   i915_sw_fence_wait(_state->commit_ready);
+
 drm_atomic_helper_wait_for_dependencies(state);

 if (intel_state->modeset)
@@ -12561,10 +12563,7 @@ intel_atomic_commit_ready(struct i915_sw_fence *fence,

 switch (notify) {
 case FENCE_COMPLETE:
-   if (state->base.commit_work.func)
-   queue_work(system_unbound_wq, >base.commit_work);


I would add a small comment here, because later-on if someone has doubts 
(and use git-blame), it won't be visible that something changed (the 
case and break were added by the same commit).



 break;
-
 case FENCE_FREE:
 {
 struct intel_atomic_helper *helper =
@@ -12668,14 +12667,14 @@ static int intel_atomic_commit(struct drm_device *dev,
 }

 drm_atomic_state_get(state);
-   INIT_WORK(>commit_work,
- nonblock ? intel_atomic_commit_work : NULL);
+   INIT_WORK(>commit_work, intel_atomic_commit_work);

 i915_sw_fence_commit(_state->commit_ready);
-   if (!nonblock) {
-   i915_sw_fence_wait(_state->commit_ready);
+   if (nonblock)
+   queue_work(system_unbound_wq, >commit_work);
+   else
 intel_atomic_commit_tail(state);
-   }
+

 return 0;
  }


Reviewed-by: Michel Thierry 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/7] drm/i915: Push i915_sw_fence_wait into the nonblocking atomic commit

2017-07-20 Thread Daniel Vetter 
Blocking in a worker is ok, that's what the unbound_wq is for. And it
unifies the paths between the blocking and nonblocking commit, giving
me just one path where I have to implement the deadlock avoidance
trickery in the next patch.

I first tried to implement the following patch without this rework, but
force-completing i915_sw_fence creates some serious challenges around
properly cleaning things up. So wasn't a feasible short-term approach.
Another approach would be to simple keep track of all pending atomic
commit work items and manually queue them from the reset code. With the
caveat that double-queue in case we race with the i915_sw_fence must be
avoided. Given all that, taking the cost of a double schedule in atomic
for the short-term fix is the best approach, but can be changed in the
future of course.

v2: Amend commit message (Chris).

Reviewed-by: Maarten Lankhorst 
Cc: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Joonas Lahtinen 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/i915/intel_display.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 995522e40ec1..f6bd6282d7f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12394,6 +12394,8 @@ static void intel_atomic_commit_tail(struct 
drm_atomic_state *state)
unsigned crtc_vblank_mask = 0;
int i;
 
+   i915_sw_fence_wait(_state->commit_ready);
+
drm_atomic_helper_wait_for_dependencies(state);
 
if (intel_state->modeset)
@@ -12561,10 +12563,7 @@ intel_atomic_commit_ready(struct i915_sw_fence *fence,
 
switch (notify) {
case FENCE_COMPLETE:
-   if (state->base.commit_work.func)
-   queue_work(system_unbound_wq, >base.commit_work);
break;
-
case FENCE_FREE:
{
struct intel_atomic_helper *helper =
@@ -12668,14 +12667,14 @@ static int intel_atomic_commit(struct drm_device *dev,
}
 
drm_atomic_state_get(state);
-   INIT_WORK(>commit_work,
- nonblock ? intel_atomic_commit_work : NULL);
+   INIT_WORK(>commit_work, intel_atomic_commit_work);
 
i915_sw_fence_commit(_state->commit_ready);
-   if (!nonblock) {
-   i915_sw_fence_wait(_state->commit_ready);
+   if (nonblock)
+   queue_work(system_unbound_wq, >commit_work);
+   else
intel_atomic_commit_tail(state);
-   }
+
 
return 0;
 }
-- 
2.13.2

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

[Intel-gfx] [PATCH 2/7] drm/i915: Push i915_sw_fence_wait into the nonblocking atomic commit

2017-07-20 Thread Daniel Vetter 
Blocking in a worker is ok, that's what the unbound_wq is for. And it
unifies the paths between the blocking and nonblocking commit, giving
me just one path where I have to implement the deadlock avoidance
trickery in the next patch.

I first tried to implement the following patch without this rework, but
force-completing i915_sw_fence creates some serious challenges around
properly cleaning things up. So wasn't a feasible short-term approach.
Another approach would be to simple keep track of all pending atomic
commit work items and manually queue them from the reset code. With the
caveat that double-queue in case we race with the i915_sw_fence must be
avoided. Given all that, taking the cost of a double schedule in atomic
for the short-term fix is the best approach, but can be changed in the
future of course.

v2: Amend commit message (Chris).

Reviewed-by: Maarten Lankhorst 
Cc: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Joonas Lahtinen 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/i915/intel_display.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 995522e40ec1..f6bd6282d7f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12394,6 +12394,8 @@ static void intel_atomic_commit_tail(struct 
drm_atomic_state *state)
unsigned crtc_vblank_mask = 0;
int i;
 
+   i915_sw_fence_wait(_state->commit_ready);
+
drm_atomic_helper_wait_for_dependencies(state);
 
if (intel_state->modeset)
@@ -12561,10 +12563,7 @@ intel_atomic_commit_ready(struct i915_sw_fence *fence,
 
switch (notify) {
case FENCE_COMPLETE:
-   if (state->base.commit_work.func)
-   queue_work(system_unbound_wq, >base.commit_work);
break;
-
case FENCE_FREE:
{
struct intel_atomic_helper *helper =
@@ -12668,14 +12667,14 @@ static int intel_atomic_commit(struct drm_device *dev,
}
 
drm_atomic_state_get(state);
-   INIT_WORK(>commit_work,
- nonblock ? intel_atomic_commit_work : NULL);
+   INIT_WORK(>commit_work, intel_atomic_commit_work);
 
i915_sw_fence_commit(_state->commit_ready);
-   if (!nonblock) {
-   i915_sw_fence_wait(_state->commit_ready);
+   if (nonblock)
+   queue_work(system_unbound_wq, >commit_work);
+   else
intel_atomic_commit_tail(state);
-   }
+
 
return 0;
 }
-- 
2.13.2

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