Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-26 Thread shuang . he
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: 
shuang...@intel.com)
Task id: 5820
-Summary-
Platform  Delta  drm-intel-nightly  Series Applied
PNV -1  281/281  280/281
ILK  308/308  308/308
SNB -1  326/326  325/326
IVB  380/380  380/380
BYT  294/294  294/294
HSW  387/421  387/421
BDW -1  316/316  315/316
-Detailed-
Platform  Testdrm-intel-nightly  Series 
Applied
 PNV  igt_gem_userptr_blits_minor-unsync-normal  DMESG_WARN(2)PASS(1)  
DMESG_WARN(1)PASS(1)
 SNB  igt_kms_plane_plane-position-hole-pipe-B-plane-1  
DMESG_WARN(12)PASS(3)  DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog  PASS(8)  DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-25 Thread shuang . he
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: 
shuang...@intel.com)
Task id: 5819
-Summary-
Platform  Delta  drm-intel-nightly  Series Applied
PNV -1  281/281  280/281
ILK  308/308  308/308
SNB -1  326/326  325/326
IVB  380/380  380/380
BYT  258/258  258/258
HSW -1  421/421  420/421
BDW -1  316/316  315/316
-Detailed-
Platform  Testdrm-intel-nightly  Series 
Applied
 PNV  igt_gem_userptr_blits_minor-normal-sync  DMESG_WARN(2)PASS(1)  
DMESG_WARN(1)PASS(1)
 SNB  igt_kms_plane_plane-panning-bottom-right-pipe-B-plane-1  
DMESG_WARN(12)PASS(3)  DMESG_WARN(1)PASS(1)
*HSW  igt_gem_storedw_batches_loop_normal  PASS(2)  DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog  PASS(7)  DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-25 Thread Jani Nikula
On Wed, 25 Feb 2015, Daniel Vetter dan...@ffwll.ch wrote:
 On Tue, Feb 24, 2015 at 01:42:39PM -0800, Matt Roper wrote:
 On Tue, Feb 24, 2015 at 01:37:54PM -0800, Rodrigo Vivi wrote:
  This return 0 without setting atomic bits on fb == crtc-cursor-fb
  where causing frontbuffer false positives.
  
  According to Daniel:
  
  The original regression seems to have been introduced in the original
  check/commit split:
  
  commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
  Author: Gustavo Padovan gustavo.pado...@collabora.co.uk
  Date:   Wed Sep 24 14:20:24 2014 -0300
  
  drm/i915: move check of intel_crtc_cursor_set_obj() out
  
  Which already cause other trouble, resulting in the check getting moved in
  
  commit e391ea882b1a04fb3f559287ac694652a3cd9da9
  Author: Gustavo Padovan gustavo.pado...@collabora.co.uk
  Date:   Wed Sep 24 14:20:25 2014 -0300
  
  drm/i915: Fix not checking cursor and object sizes
  
  The frontbuffer tracking itself only was broken when we shifted it into
  the check/commit logic with:
  
  commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
  Author: Matt Roper matthew.d.ro...@intel.com
  Date:   Wed Dec 24 07:59:06 2014 -0800
  
  drm/i915: Refactor work that can sleep out of commit (v7)
  
  v2: When putting more debug prints I notice the solution was simpler
  than I thought. AMS design is solid, just this return was wrong.
  Sorry for the noise.
  
  v3: Remove the entire chunck that would probably
  be removed by gcc anyway. (by Daniel)
  
  Cc: Jani Nikula jani.nik...@intel.com
  Cc: Gustavo Padovan gustavo.pado...@collabora.co.uk
  Cc: Matt Roper matthew.d.ro...@intel.com
  Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
 
 Not sure I understand Daniel's remark about gcc optimization, but the
 change still looks good to me.

 Yeah gcc is not clever enought to optimize this away since we can't tell
 it that fb-modifier is invariant forever once assigned.
 
 Reviewed-by: Matt Roper matthew.d.ro...@intel.com

 Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch

 Jani, this one is for 4.0-rc.

Pushed to drm-intel-fixes, thanks for the patch and reviews.

I had to adjust the context for backporting to v4.0-rc1, please check
that drm-intel-fixes makes sense *and* that my merge resolution back to
drm-intel-nightly makes sense. Thanks.

BR,
Jani.



 Thanks, Daniel

 
  ---
   drivers/gpu/drm/i915/intel_display.c | 3 ---
   1 file changed, 3 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/intel_display.c 
  b/drivers/gpu/drm/i915/intel_display.c
  index 8ccf033..900dcaa 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -12197,9 +12197,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
 return -ENOMEM;
 }
   
  -  if (fb == crtc-cursor-fb)
  -  return 0;
  -
 if (fb-modifier[0] != DRM_FORMAT_MOD_NONE) {
 DRM_DEBUG_KMS(cursor cannot be tiled\n);
 ret = -EINVAL;
  -- 
  2.1.0
  
 
 -- 
 Matt Roper
 Graphics Software Engineer
 IoTG Platform Enabling  Development
 Intel Corporation
 (916) 356-2795
 ___
 Intel-gfx mailing list
 Intel-gfx@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx

 -- 
 Daniel Vetter
 Software Engineer, Intel Corporation
 +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-24 Thread Matt Roper
On Tue, Feb 24, 2015 at 01:37:54PM -0800, Rodrigo Vivi wrote:
 This return 0 without setting atomic bits on fb == crtc-cursor-fb
 where causing frontbuffer false positives.
 
 According to Daniel:
 
 The original regression seems to have been introduced in the original
 check/commit split:
 
 commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
 Author: Gustavo Padovan gustavo.pado...@collabora.co.uk
 Date:   Wed Sep 24 14:20:24 2014 -0300
 
 drm/i915: move check of intel_crtc_cursor_set_obj() out
 
 Which already cause other trouble, resulting in the check getting moved in
 
 commit e391ea882b1a04fb3f559287ac694652a3cd9da9
 Author: Gustavo Padovan gustavo.pado...@collabora.co.uk
 Date:   Wed Sep 24 14:20:25 2014 -0300
 
 drm/i915: Fix not checking cursor and object sizes
 
 The frontbuffer tracking itself only was broken when we shifted it into
 the check/commit logic with:
 
 commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
 Author: Matt Roper matthew.d.ro...@intel.com
 Date:   Wed Dec 24 07:59:06 2014 -0800
 
 drm/i915: Refactor work that can sleep out of commit (v7)
 
 v2: When putting more debug prints I notice the solution was simpler
 than I thought. AMS design is solid, just this return was wrong.
 Sorry for the noise.
 
 v3: Remove the entire chunck that would probably
 be removed by gcc anyway. (by Daniel)
 
 Cc: Jani Nikula jani.nik...@intel.com
 Cc: Gustavo Padovan gustavo.pado...@collabora.co.uk
 Cc: Matt Roper matthew.d.ro...@intel.com
 Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com

Not sure I understand Daniel's remark about gcc optimization, but the
change still looks good to me.

Reviewed-by: Matt Roper matthew.d.ro...@intel.com

 ---
  drivers/gpu/drm/i915/intel_display.c | 3 ---
  1 file changed, 3 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 8ccf033..900dcaa 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -12197,9 +12197,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
   return -ENOMEM;
   }
  
 - if (fb == crtc-cursor-fb)
 - return 0;
 -
   if (fb-modifier[0] != DRM_FORMAT_MOD_NONE) {
   DRM_DEBUG_KMS(cursor cannot be tiled\n);
   ret = -EINVAL;
 -- 
 2.1.0
 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling  Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-24 Thread Daniel Vetter
On Tue, Feb 24, 2015 at 10:44:19AM -0800, Matt Roper wrote:
 On Tue, Feb 24, 2015 at 10:36:32AM -0800, Rodrigo Vivi wrote:
  Atomic bits needs to be set when cursor check function is returning 0
  and intel_crtc is active.
  
  v2: When putting more debug prints I notice the solution was simpler
  than I thought. AMS design is solid, just this return was wrong.
  Sorry for the noise.
  
  Cc: Matt Roper matthew.d.ro...@intel.com
  Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
 
 Reviewed-by: Matt Roper matthew.d.ro...@intel.com
 
  ---
   drivers/gpu/drm/i915/intel_display.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
  
  diff --git a/drivers/gpu/drm/i915/intel_display.c 
  b/drivers/gpu/drm/i915/intel_display.c
  index 8ccf033..5fb83ba 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -12198,7 +12198,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
  }
   
  if (fb == crtc-cursor-fb)
  -   return 0;
  +   goto finish;

Shouldn't we just delete this check entirely? gcc will do it anyway for
us. When you do that you can also append the history I've dug out for
this.

The original regression seems to have been introduced in the original
check/commit split:

commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
Author: Gustavo Padovan gustavo.pado...@collabora.co.uk
Date:   Wed Sep 24 14:20:24 2014 -0300

drm/i915: move check of intel_crtc_cursor_set_obj() out

Which already cause other trouble, resulting in the check getting moved in

commit e391ea882b1a04fb3f559287ac694652a3cd9da9
Author: Gustavo Padovan gustavo.pado...@collabora.co.uk
Date:   Wed Sep 24 14:20:25 2014 -0300

drm/i915: Fix not checking cursor and object sizes

The frontbuffer tracking itself only was broken when we shifted it into
the check/commit logic with:

commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
Author: Matt Roper matthew.d.ro...@intel.com
Date:   Wed Dec 24 07:59:06 2014 -0800

drm/i915: Refactor work that can sleep out of commit (v7)

Cheers, Daniel
   
  if (fb-modifier[0] != DRM_FORMAT_MOD_NONE) {
  DRM_DEBUG_KMS(cursor cannot be tiled\n);
  -- 
  2.1.0
  
 
 -- 
 Matt Roper
 Graphics Software Engineer
 IoTG Platform Enabling  Development
 Intel Corporation
 (916) 356-2795
 ___
 Intel-gfx mailing list
 Intel-gfx@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-24 Thread Rodrigo Vivi
This return 0 without setting atomic bits on fb == crtc-cursor-fb
where causing frontbuffer false positives.

According to Daniel:

The original regression seems to have been introduced in the original
check/commit split:

commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
Author: Gustavo Padovan gustavo.pado...@collabora.co.uk
Date:   Wed Sep 24 14:20:24 2014 -0300

drm/i915: move check of intel_crtc_cursor_set_obj() out

Which already cause other trouble, resulting in the check getting moved in

commit e391ea882b1a04fb3f559287ac694652a3cd9da9
Author: Gustavo Padovan gustavo.pado...@collabora.co.uk
Date:   Wed Sep 24 14:20:25 2014 -0300

drm/i915: Fix not checking cursor and object sizes

The frontbuffer tracking itself only was broken when we shifted it into
the check/commit logic with:

commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
Author: Matt Roper matthew.d.ro...@intel.com
Date:   Wed Dec 24 07:59:06 2014 -0800

drm/i915: Refactor work that can sleep out of commit (v7)

v2: When putting more debug prints I notice the solution was simpler
than I thought. AMS design is solid, just this return was wrong.
Sorry for the noise.

v3: Remove the entire chunck that would probably
be removed by gcc anyway. (by Daniel)

Cc: Jani Nikula jani.nik...@intel.com
Cc: Gustavo Padovan gustavo.pado...@collabora.co.uk
Cc: Matt Roper matthew.d.ro...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 8ccf033..900dcaa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12197,9 +12197,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
return -ENOMEM;
}
 
-   if (fb == crtc-cursor-fb)
-   return 0;
-
if (fb-modifier[0] != DRM_FORMAT_MOD_NONE) {
DRM_DEBUG_KMS(cursor cannot be tiled\n);
ret = -EINVAL;
-- 
2.1.0

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


[Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-24 Thread Rodrigo Vivi
Atomic bits needs to be set when cursor check function is returning 0
and intel_crtc is active.

v2: When putting more debug prints I notice the solution was simpler
than I thought. AMS design is solid, just this return was wrong.
Sorry for the noise.

Cc: Matt Roper matthew.d.ro...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 8ccf033..5fb83ba 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12198,7 +12198,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
}
 
if (fb == crtc-cursor-fb)
-   return 0;
+   goto finish;
 
if (fb-modifier[0] != DRM_FORMAT_MOD_NONE) {
DRM_DEBUG_KMS(cursor cannot be tiled\n);
-- 
2.1.0

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


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-24 Thread Rodrigo Vivi
On Mon, Feb 23, 2015 at 6:13 PM, Matt Roper matthew.d.ro...@intel.com wrote:
 On Mon, Feb 23, 2015 at 05:52:24PM -0800, Rodrigo Vivi wrote:
 Hi Daniel,

 It seems that one check with one good commit followed by many zeroed
 intel_crtc-atomic commits is again in place.

 Can you elaborate on what you mean by this?  With atomic it's possible
 to have a check with no commit after it (if the check or prepare fail,
 or if it's a 'test only' operation), but if you're seeing commits
 without corresponding checks, that sounds like a bug.

Ah so yes, this is the bug, when moving a cursor we have many commits
without checks. So it is only one check followed by many commits... so
it commits with intel_crtc-atomic all zeroed by a previous
finish_commit.


 Can you provide a dmesg with drm.debug turned on so we can see what's
 going on?  Or add some dump_stack()'s so that we can see the backtrace
 that led us to this situation.

I lost those logs, but if you put one print in check and one in commit
you also should be able to see that since I heard about this false
positive from many people.


 Actually, I wonder if the problem is actually the opposite of what you
 say above.  Now that I look at this again, we only zero out
 intel_crtc-atomic in intel_finish_crtc_commit which is part of the
 commit path.

From what I heard and what I saw on the logs I thought it was ok to
have 1 check than as many commits as possible without the check.
If that was the case we would have a fail that is to zero the
structure on finish_commit. But seems this isn't the case. Sorry.

 So if you had a check that never got a corresponding
 commit, there might still be garbage left over in there.

No it is the opposite. Commits with no check. causing commit of
intel_crtc-atomic zeroed.

 Ultimately we
 should be handling all of this with real intel_crtc_state handling which
 we don't quite have yet.  Maybe in the meantime we need to be clearing
 out intel_crtc-atomic at the beginning of a top-level atomic
 transaction?  I'll send a patch that makes this change for you to try
 shortly.

Thanks! I thought about this but got afraid that clearing this on top
of check could cause a race since one plane doing a check could clean
the atomic being commited on cursor movement, unless we hat that for
planes, but then we would have to iterate over all planes on
finish_commit.

We can also put this patch that fix the only known issue so far with a
FIXME comment while we don't have the final fix.



 Matt


 So I'm getting that annoying false positives on latest -nightly.

 Shouldn't we just merge this patch while atomic modeset design doesn't
 get fixed at all?

 Unfortunately nothing comes to my mind than moving all
 intel_crtc-atomic set to commit time and let pre_commit just with
 pm_get...
 Ohter than that just a full redesign of atomic modeset.

 Thanks,
 Rodrigo.


 On Fri, Feb 13, 2015 at 12:48 AM, Daniel Vetter dan...@ffwll.ch wrote:
  On Thu, Feb 12, 2015 at 05:17:04PM -0800, Rodrigo Vivi wrote:
  No, we had solved old frontbuffer false positives... some missing
  flush somewhere at that time...
 
  So, I added a bunch of printk and I insist that it is conceptually
  wrong to set intel_crtc_atomic_commit on check times when you do
  memset(intel_crtc-atomic, 0, sizeof(intel_crtc-atomic));
  on every finish_commit.
 
  With exception of atomic.disabled_planes I believe the rest shouldn't
  work in the way it is implemented because you can have one check
  followed by many commits, but after the first commit all atomic
  variables are zeroed, except the disabled_planes that is set outside
  check...
 
  Ok here's the trouble: Every commit should have at exactly one check for
  the new state objects. Unfortunately in the transition that seems to have
  been lost for some cases.
 
  For instance: on every cursor movement atomic.fb_bits was 0x000 when
  it should be 0x002. This is why this patch solved the false positive,
  i.e. setting it on commit instead on check time we get it propperly
  set. One of the problems is the false positive but also it breaks
  entirely SW tracking on VLV/CHV
 
  I believe wait_for flips, update_fbc, watermarks, etc should keep the
  value got on check for the commit or the check should be done at
  commit plane instead of on check.
 
  I started doing a patch here to move all atomic sets from check to
  commit functions but gave up on middle when I noticed the
  prepare_commit would almost get empty...
 
  All state precomputation must be done in check, at commit time you have a
  lot less information since the old state is somewhat gone. You can still
  get at it, but as soon as we add an async flip queue that will get really
  ugly. The current placement is imo the correct one. Instead we need to
  figure out where we're doing a -commit without properly calling -check
  beforehand.
 
  Another idea was to make a atomic set per plane and just memset(0) on
  begin of every check... But this would require reliable 

Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-24 Thread Matt Roper
On Tue, Feb 24, 2015 at 09:32:25AM -0800, Rodrigo Vivi wrote:
 On Mon, Feb 23, 2015 at 6:13 PM, Matt Roper matthew.d.ro...@intel.com wrote:
  On Mon, Feb 23, 2015 at 05:52:24PM -0800, Rodrigo Vivi wrote:
  Hi Daniel,
 
  It seems that one check with one good commit followed by many zeroed
  intel_crtc-atomic commits is again in place.
 
  Can you elaborate on what you mean by this?  With atomic it's possible
  to have a check with no commit after it (if the check or prepare fail,
  or if it's a 'test only' operation), but if you're seeing commits
  without corresponding checks, that sounds like a bug.
 
 Ah so yes, this is the bug, when moving a cursor we have many commits
 without checks. So it is only one check followed by many commits... so
 it commits with intel_crtc-atomic all zeroed by a previous
 finish_commit.
 
 
  Can you provide a dmesg with drm.debug turned on so we can see what's
  going on?  Or add some dump_stack()'s so that we can see the backtrace
  that led us to this situation.
 
 I lost those logs, but if you put one print in check and one in commit
 you also should be able to see that since I heard about this false
 positive from many people.

Hmm.  Are all of these people using a specific platform?  I only have
access to IVB hardware at the moment, so it could be that the problem
lies on codepaths I just don't exercise on my system.  Most of the
atomic stuff is pretty platform-agnostic, but maybe there's something I
overlooked.

 
  Actually, I wonder if the problem is actually the opposite of what you
  say above.  Now that I look at this again, we only zero out
  intel_crtc-atomic in intel_finish_crtc_commit which is part of the
  commit path.
 
 From what I heard and what I saw on the logs I thought it was ok to
 have 1 check than as many commits as possible without the check.
 If that was the case we would have a fail that is to zero the
 structure on finish_commit. But seems this isn't the case. Sorry.

I guess I should clarify terminology a little bit since this gets kind
of confusing...

For atomic, we have a top-level atomic_state structure that contains
everything that we want to update as part of an atomic transaction
(multiple planes, crtc's, etc.).  At the moment, we only support a
subset of atomic, so you'll only ever get one crtc being modified, but
there could be multiple planes (primary, cursor, 1 or more sprite
planes).  There's a sub-state structure (plane_state, crtc_state, etc.)
for each DRM object being updated.

At the top level of atomic handling we do check and then commit with
the top-level atomic state.  If you trace down through the codeflow, the
top-level check here will actually call a plane-specific check function
on each plane state during the top-level check, and then the top-level
commit function will call the plane-specific commit function on each
plane involved.

So when I say 1 or more checks, followed by exactly one commit I'm
referring to the top-level check that deals with the top-level atomic
state.  If you're looking at the plane state specifically, you might see
more of an n:n relationship if your transaction is updating multiple
planes together (one check call per plane being updated, possibly
followed by one commit call for each of them), but the clearing of
intel_crtc-atomic should still happen at the end, after all of the
individual planes have been committed.

  So if you had a check that never got a corresponding
  commit, there might still be garbage left over in there.
 
 No it is the opposite. Commits with no check. causing commit of
 intel_crtc-atomic zeroed.
 
  Ultimately we
  should be handling all of this with real intel_crtc_state handling which
  we don't quite have yet.  Maybe in the meantime we need to be clearing
  out intel_crtc-atomic at the beginning of a top-level atomic
  transaction?  I'll send a patch that makes this change for you to try
  shortly.
 
 Thanks! I thought about this but got afraid that clearing this on top
 of check could cause a race since one plane doing a check could clean
 the atomic being commited on cursor movement, unless we hat that for
 planes, but then we would have to iterate over all planes on
 finish_commit.
 
 We can also put this patch that fix the only known issue so far with a
 FIXME comment while we don't have the final fix.

Well, based on what you say above, it sounds like it probably won't
solve your issue, so we'll need to explore some more.  I still haven't
managed to reproduce this myself, so I'm not sure whether I'm working on
a hardware platform that won't trigger the bug, or whether I'm just
unlucky.

Is there a specific igt test you've been using that triggers this
reliably for you?


Matt

 
 
 
  Matt
 
 
  So I'm getting that annoying false positives on latest -nightly.
 
  Shouldn't we just merge this patch while atomic modeset design doesn't
  get fixed at all?
 
  Unfortunately nothing comes to my mind than moving all
  intel_crtc-atomic set to commit time and let 

Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-13 Thread Daniel Vetter
On Thu, Feb 12, 2015 at 05:17:04PM -0800, Rodrigo Vivi wrote:
 No, we had solved old frontbuffer false positives... some missing
 flush somewhere at that time...
 
 So, I added a bunch of printk and I insist that it is conceptually
 wrong to set intel_crtc_atomic_commit on check times when you do
 memset(intel_crtc-atomic, 0, sizeof(intel_crtc-atomic));
 on every finish_commit.
 
 With exception of atomic.disabled_planes I believe the rest shouldn't
 work in the way it is implemented because you can have one check
 followed by many commits, but after the first commit all atomic
 variables are zeroed, except the disabled_planes that is set outside
 check...

Ok here's the trouble: Every commit should have at exactly one check for
the new state objects. Unfortunately in the transition that seems to have
been lost for some cases.

 For instance: on every cursor movement atomic.fb_bits was 0x000 when
 it should be 0x002. This is why this patch solved the false positive,
 i.e. setting it on commit instead on check time we get it propperly
 set. One of the problems is the false positive but also it breaks
 entirely SW tracking on VLV/CHV
 
 I believe wait_for flips, update_fbc, watermarks, etc should keep the
 value got on check for the commit or the check should be done at
 commit plane instead of on check.
 
 I started doing a patch here to move all atomic sets from check to
 commit functions but gave up on middle when I noticed the
 prepare_commit would almost get empty...

All state precomputation must be done in check, at commit time you have a
lot less information since the old state is somewhat gone. You can still
get at it, but as soon as we add an async flip queue that will get really
ugly. The current placement is imo the correct one. Instead we need to
figure out where we're doing a -commit without properly calling -check
beforehand.

 Another idea was to make a atomic set per plane and just memset(0) on
 begin of every check... But this would require reliable access to the
 plane being updated on finish_commit... I believe loop on all planes
 would be messy and cause other issues...
 
 So, I'll be out returning only next wed. Please let me know if you
 have any suggestion of best changes to do that I can implement the
 changes.

Since you've done this testing I've landed Matt's patches to switch legacy
plane entry points over to atomic. Which means cursor updates should now
be done properly using atomic, always. But even then the old transitional
plane helpers should have called the check functions ... So not sure where
exactly we're loosing that check call.

Matt Roper might have more insights.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-12 Thread Rodrigo Vivi
No, we had solved old frontbuffer false positives... some missing
flush somewhere at that time...

So, I added a bunch of printk and I insist that it is conceptually
wrong to set intel_crtc_atomic_commit on check times when you do
memset(intel_crtc-atomic, 0, sizeof(intel_crtc-atomic));
on every finish_commit.

With exception of atomic.disabled_planes I believe the rest shouldn't
work in the way it is implemented because you can have one check
followed by many commits, but after the first commit all atomic
variables are zeroed, except the disabled_planes that is set outside
check...

For instance: on every cursor movement atomic.fb_bits was 0x000 when
it should be 0x002. This is why this patch solved the false positive,
i.e. setting it on commit instead on check time we get it propperly
set. One of the problems is the false positive but also it breaks
entirely SW tracking on VLV/CHV

I believe wait_for flips, update_fbc, watermarks, etc should keep the
value got on check for the commit or the check should be done at
commit plane instead of on check.

I started doing a patch here to move all atomic sets from check to
commit functions but gave up on middle when I noticed the
prepare_commit would almost get empty...

Another idea was to make a atomic set per plane and just memset(0) on
begin of every check... But this would require reliable access to the
plane being updated on finish_commit... I believe loop on all planes
would be messy and cause other issues...

So, I'll be out returning only next wed. Please let me know if you
have any suggestion of best changes to do that I can implement the
changes.

Thanks,
Rodrigo.


On Tue, Feb 3, 2015 at 11:40 AM, Daniel Vetter dan...@ffwll.ch wrote:
 On Tue, Feb 03, 2015 at 10:46:49AM -0800, Rodrigo Vivi wrote:
 On Tue, Feb 3, 2015 at 8:21 AM, Matt Roper matthew.d.ro...@intel.com wrote:
  On Tue, Feb 03, 2015 at 12:57:31PM +0100, Daniel Vetter wrote:
  On Mon, Feb 02, 2015 at 03:38:16PM -0800, Rodrigo Vivi wrote:
   frontbuffer bits must be updated during commit times not on atomica 
   prepare
   one, otherwise we have a risk of false positive.
  
   Cc Daniel Vetter daniel.vet...@ffwll.ch
   Cc: Sonika Jindal sonika.jin...@intel.com
   Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
 
  atomic.fb_bits isn't used at all right now, instead the
  begin_crtc_commit function recomputes them. That looks wrong.
 
  We build up the collection of bits in atomic.fb_bits while going through
  the atomic pipeline for each plane, then do a single call to
 
  intel_frontbuffer_flip(dev, intel_crtc-atomic.fb_bits);
 
  in intel_finish_crtc_commit to flush them all out together (and if
  check/prepare fail, we never actually get to that flush).
 
  Also for async commits we need
  to do the proper 2-stage flip stuff that current page_flip code does using
  frontbuffer_flip_prepare/complete.
 
  I need to look at the frontbuffer stuff again...maybe we don't need
  atomic.fb_bits at all and should just use intel_frontbuffer_flip_prepare
  in the places we set the bits now and intel_frontbuffer_flip_complete
  where we're calling the flip mentioned above?
 
 
  Matt
 
 
  This patch here should have 0 effect (presuming I'm reading code
  correctly), so what kinf of bug exactly are you seeing?

 Yeah, after I sent the patch I was thinking about that: that it should
 have 0 effect,
 but the symptom that this apparently fixed here is that PSR wasn't
 starting at all without
 doing something like going to fbcon and come back to X.
 Something similar what Sonika had told on that psr-skl thread where
 she couldn't get psr working on login screen.
 After this patch I didn't' have to change back and forth to fbcon to
 make psr work.

 Maybe just a coincidence, but anyway I believe the way it is nowadays
 it is wrong. I believe frontbuffer bits and calls should be done at
 commit step, not under prepare.

 Hm, I think we've had this issue for a long time, even on older
 frontbuffer tracking code. I think it's time to throw lots of debug printk
 at the problem and figure out why psr is disabled in this case.
 -Daniel
 --
 Daniel Vetter
 Software Engineer, Intel Corporation
 +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-03 Thread Matt Roper
On Tue, Feb 03, 2015 at 12:57:31PM +0100, Daniel Vetter wrote:
 On Mon, Feb 02, 2015 at 03:38:16PM -0800, Rodrigo Vivi wrote:
  frontbuffer bits must be updated during commit times not on atomica prepare
  one, otherwise we have a risk of false positive.
 
  Cc Daniel Vetter daniel.vet...@ffwll.ch
  Cc: Sonika Jindal sonika.jin...@intel.com
  Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
 
 atomic.fb_bits isn't used at all right now, instead the
 begin_crtc_commit function recomputes them. That looks wrong. 

We build up the collection of bits in atomic.fb_bits while going through
the atomic pipeline for each plane, then do a single call to

intel_frontbuffer_flip(dev, intel_crtc-atomic.fb_bits);

in intel_finish_crtc_commit to flush them all out together (and if
check/prepare fail, we never actually get to that flush).

 Also for async commits we need
 to do the proper 2-stage flip stuff that current page_flip code does using
 frontbuffer_flip_prepare/complete.

I need to look at the frontbuffer stuff again...maybe we don't need
atomic.fb_bits at all and should just use intel_frontbuffer_flip_prepare
in the places we set the bits now and intel_frontbuffer_flip_complete
where we're calling the flip mentioned above?


Matt

 
 This patch here should have 0 effect (presuming I'm reading code
 correctly), so what kinf of bug exactly are you seeing?
 
 Adding MattAnder.
 -Daniel
 -- 
 Daniel Vetter
 Software Engineer, Intel Corporation
 +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling  Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-03 Thread Daniel Vetter
On Mon, Feb 02, 2015 at 03:38:16PM -0800, Rodrigo Vivi wrote:
 frontbuffer bits must be updated during commit times not on atomica prepare
 one, otherwise we have a risk of false positive.

 Cc Daniel Vetter daniel.vet...@ffwll.ch
 Cc: Sonika Jindal sonika.jin...@intel.com
 Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com

atomic.fb_bits isn't used at all right now, instead the begin_crtc_commit
function recomputes them. That looks wrong. Also for async commits we need
to do the proper 2-stage flip stuff that current page_flip code does using
frontbuffer_flip_prepare/complete.

This patch here should have 0 effect (presuming I'm reading code
correctly), so what kinf of bug exactly are you seeing?

Adding MattAnder.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-03 Thread Rodrigo Vivi
On Tue, Feb 3, 2015 at 8:21 AM, Matt Roper matthew.d.ro...@intel.com wrote:
 On Tue, Feb 03, 2015 at 12:57:31PM +0100, Daniel Vetter wrote:
 On Mon, Feb 02, 2015 at 03:38:16PM -0800, Rodrigo Vivi wrote:
  frontbuffer bits must be updated during commit times not on atomica prepare
  one, otherwise we have a risk of false positive.
 
  Cc Daniel Vetter daniel.vet...@ffwll.ch
  Cc: Sonika Jindal sonika.jin...@intel.com
  Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com

 atomic.fb_bits isn't used at all right now, instead the
 begin_crtc_commit function recomputes them. That looks wrong.

 We build up the collection of bits in atomic.fb_bits while going through
 the atomic pipeline for each plane, then do a single call to

 intel_frontbuffer_flip(dev, intel_crtc-atomic.fb_bits);

 in intel_finish_crtc_commit to flush them all out together (and if
 check/prepare fail, we never actually get to that flush).

 Also for async commits we need
 to do the proper 2-stage flip stuff that current page_flip code does using
 frontbuffer_flip_prepare/complete.

 I need to look at the frontbuffer stuff again...maybe we don't need
 atomic.fb_bits at all and should just use intel_frontbuffer_flip_prepare
 in the places we set the bits now and intel_frontbuffer_flip_complete
 where we're calling the flip mentioned above?


 Matt


 This patch here should have 0 effect (presuming I'm reading code
 correctly), so what kinf of bug exactly are you seeing?

Yeah, after I sent the patch I was thinking about that: that it should
have 0 effect,
but the symptom that this apparently fixed here is that PSR wasn't
starting at all without
doing something like going to fbcon and come back to X.
Something similar what Sonika had told on that psr-skl thread where
she couldn't get psr working on login screen.
After this patch I didn't' have to change back and forth to fbcon to
make psr work.

Maybe just a coincidence, but anyway I believe the way it is nowadays
it is wrong. I believe frontbuffer bits and calls should be done at
commit step, not under prepare.


 Adding MattAnder.
 -Daniel
 --
 Daniel Vetter
 Software Engineer, Intel Corporation
 +41 (0) 79 365 57 48 - http://blog.ffwll.ch

 --
 Matt Roper
 Graphics Software Engineer
 IoTG Platform Enabling  Development
 Intel Corporation
 (916) 356-2795
 ___
 Intel-gfx mailing list
 Intel-gfx@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-03 Thread Matt Roper
On Tue, Feb 03, 2015 at 10:46:49AM -0800, Rodrigo Vivi wrote:
 On Tue, Feb 3, 2015 at 8:21 AM, Matt Roper matthew.d.ro...@intel.com wrote:
  On Tue, Feb 03, 2015 at 12:57:31PM +0100, Daniel Vetter wrote:
  On Mon, Feb 02, 2015 at 03:38:16PM -0800, Rodrigo Vivi wrote:
   frontbuffer bits must be updated during commit times not on atomica 
   prepare
   one, otherwise we have a risk of false positive.
  
   Cc Daniel Vetter daniel.vet...@ffwll.ch
   Cc: Sonika Jindal sonika.jin...@intel.com
   Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
 
  atomic.fb_bits isn't used at all right now, instead the
  begin_crtc_commit function recomputes them. That looks wrong.
 
  We build up the collection of bits in atomic.fb_bits while going through
  the atomic pipeline for each plane, then do a single call to
 
  intel_frontbuffer_flip(dev, intel_crtc-atomic.fb_bits);
 
  in intel_finish_crtc_commit to flush them all out together (and if
  check/prepare fail, we never actually get to that flush).
 
  Also for async commits we need
  to do the proper 2-stage flip stuff that current page_flip code does using
  frontbuffer_flip_prepare/complete.
 
  I need to look at the frontbuffer stuff again...maybe we don't need
  atomic.fb_bits at all and should just use intel_frontbuffer_flip_prepare
  in the places we set the bits now and intel_frontbuffer_flip_complete
  where we're calling the flip mentioned above?
 
 
  Matt
 
 
  This patch here should have 0 effect (presuming I'm reading code
  correctly), so what kinf of bug exactly are you seeing?
 
 Yeah, after I sent the patch I was thinking about that: that it should
 have 0 effect,
 but the symptom that this apparently fixed here is that PSR wasn't
 starting at all without
 doing something like going to fbcon and come back to X.
 Something similar what Sonika had told on that psr-skl thread where
 she couldn't get psr working on login screen.
 After this patch I didn't' have to change back and forth to fbcon to
 make psr work.
 
 Maybe just a coincidence, but anyway I believe the way it is nowadays
 it is wrong. I believe frontbuffer bits and calls should be done at
 commit step, not under prepare.

I may be misunderstanding what you're saying, but I think this is the
way things already work?  We plan out which bits we're going to update
in the 'check' step (and record those bits in atomic.fb_bits), but then
we only actually set them with intel_frontbuffer_flip() in commit if
everything looks good.  If we wind up rejecting the update because we
fail to check/prepare, those bits in atomic.fb_bits should just get
thrown away, unless I'm missing something.


Matt

 
 
  Adding MattAnder.
  -Daniel
  --
  Daniel Vetter
  Software Engineer, Intel Corporation
  +41 (0) 79 365 57 48 - http://blog.ffwll.ch
 
  --
  Matt Roper
  Graphics Software Engineer
  IoTG Platform Enabling  Development
  Intel Corporation
  (916) 356-2795
  ___
  Intel-gfx mailing list
  Intel-gfx@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/intel-gfx
 
 
 
 -- 
 Rodrigo Vivi
 Blog: http://blog.vivi.eng.br

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling  Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-03 Thread Daniel Vetter
On Tue, Feb 03, 2015 at 11:14:10AM -0800, Matt Roper wrote:
 On Tue, Feb 03, 2015 at 10:46:49AM -0800, Rodrigo Vivi wrote:
  On Tue, Feb 3, 2015 at 8:21 AM, Matt Roper matthew.d.ro...@intel.com 
  wrote:
   On Tue, Feb 03, 2015 at 12:57:31PM +0100, Daniel Vetter wrote:
   On Mon, Feb 02, 2015 at 03:38:16PM -0800, Rodrigo Vivi wrote:
frontbuffer bits must be updated during commit times not on atomica 
prepare
one, otherwise we have a risk of false positive.
   
Cc Daniel Vetter daniel.vet...@ffwll.ch
Cc: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
  
   atomic.fb_bits isn't used at all right now, instead the
   begin_crtc_commit function recomputes them. That looks wrong.
  
   We build up the collection of bits in atomic.fb_bits while going through
   the atomic pipeline for each plane, then do a single call to
  
   intel_frontbuffer_flip(dev, intel_crtc-atomic.fb_bits);
  
   in intel_finish_crtc_commit to flush them all out together (and if
   check/prepare fail, we never actually get to that flush).
  
   Also for async commits we need
   to do the proper 2-stage flip stuff that current page_flip code does 
   using
   frontbuffer_flip_prepare/complete.
  
   I need to look at the frontbuffer stuff again...maybe we don't need
   atomic.fb_bits at all and should just use intel_frontbuffer_flip_prepare
   in the places we set the bits now and intel_frontbuffer_flip_complete
   where we're calling the flip mentioned above?
  
  
   Matt
  
  
   This patch here should have 0 effect (presuming I'm reading code
   correctly), so what kinf of bug exactly are you seeing?
  
  Yeah, after I sent the patch I was thinking about that: that it should
  have 0 effect,
  but the symptom that this apparently fixed here is that PSR wasn't
  starting at all without
  doing something like going to fbcon and come back to X.
  Something similar what Sonika had told on that psr-skl thread where
  she couldn't get psr working on login screen.
  After this patch I didn't' have to change back and forth to fbcon to
  make psr work.
  
  Maybe just a coincidence, but anyway I believe the way it is nowadays
  it is wrong. I believe frontbuffer bits and calls should be done at
  commit step, not under prepare.
 
 I may be misunderstanding what you're saying, but I think this is the
 way things already work?  We plan out which bits we're going to update
 in the 'check' step (and record those bits in atomic.fb_bits), but then
 we only actually set them with intel_frontbuffer_flip() in commit if
 everything looks good.  If we wind up rejecting the update because we
 fail to check/prepare, those bits in atomic.fb_bits should just get
 thrown away, unless I'm missing something.

What we could do is store the fb_bits in intel_plane and then move all the
code to handle them into shared code, out of the cursor/primary/sprite
specific stuff. That helps with new hw, in case we get 2 sprite planes and
so can't use the drm type any more to figure out which one it is.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-03 Thread Daniel Vetter
On Tue, Feb 03, 2015 at 08:21:33AM -0800, Matt Roper wrote:
 On Tue, Feb 03, 2015 at 12:57:31PM +0100, Daniel Vetter wrote:
  On Mon, Feb 02, 2015 at 03:38:16PM -0800, Rodrigo Vivi wrote:
   frontbuffer bits must be updated during commit times not on atomica 
   prepare
   one, otherwise we have a risk of false positive.
  
   Cc Daniel Vetter daniel.vet...@ffwll.ch
   Cc: Sonika Jindal sonika.jin...@intel.com
   Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
  
  atomic.fb_bits isn't used at all right now, instead the
  begin_crtc_commit function recomputes them. That looks wrong. 
 
 We build up the collection of bits in atomic.fb_bits while going through
 the atomic pipeline for each plane, then do a single call to
 
 intel_frontbuffer_flip(dev, intel_crtc-atomic.fb_bits);
 
 in intel_finish_crtc_commit to flush them all out together (and if
 check/prepare fail, we never actually get to that flush).

Hm right, no idea why I didn't spot this. And the code in
begin_crtc_commit is needed too.

  Also for async commits we need
  to do the proper 2-stage flip stuff that current page_flip code does using
  frontbuffer_flip_prepare/complete.
 
 I need to look at the frontbuffer stuff again...maybe we don't need
 atomic.fb_bits at all and should just use intel_frontbuffer_flip_prepare
 in the places we set the bits now and intel_frontbuffer_flip_complete
 where we're calling the flip mentioned above?

We need the same mask for both prepare and complete, so precomputing it
doesn't seem like a stupid idea.

One thing that does look a bit fishy though is the handling of
i915_gem_track_fb. Doing that in prepare_planes isn't correct since if a
later plane fails the prepare step we don't undo this.

And for simplicity it might be better to consolidate this all into the
loop in begin_crtc_commit, and maybe also push out the call to grab the
mutex - in most cases buffers will changes, so optimizing for moving
cursors doesn't seem to be a good idea.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-03 Thread Daniel Vetter
On Tue, Feb 03, 2015 at 10:46:49AM -0800, Rodrigo Vivi wrote:
 On Tue, Feb 3, 2015 at 8:21 AM, Matt Roper matthew.d.ro...@intel.com wrote:
  On Tue, Feb 03, 2015 at 12:57:31PM +0100, Daniel Vetter wrote:
  On Mon, Feb 02, 2015 at 03:38:16PM -0800, Rodrigo Vivi wrote:
   frontbuffer bits must be updated during commit times not on atomica 
   prepare
   one, otherwise we have a risk of false positive.
  
   Cc Daniel Vetter daniel.vet...@ffwll.ch
   Cc: Sonika Jindal sonika.jin...@intel.com
   Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
 
  atomic.fb_bits isn't used at all right now, instead the
  begin_crtc_commit function recomputes them. That looks wrong.
 
  We build up the collection of bits in atomic.fb_bits while going through
  the atomic pipeline for each plane, then do a single call to
 
  intel_frontbuffer_flip(dev, intel_crtc-atomic.fb_bits);
 
  in intel_finish_crtc_commit to flush them all out together (and if
  check/prepare fail, we never actually get to that flush).
 
  Also for async commits we need
  to do the proper 2-stage flip stuff that current page_flip code does using
  frontbuffer_flip_prepare/complete.
 
  I need to look at the frontbuffer stuff again...maybe we don't need
  atomic.fb_bits at all and should just use intel_frontbuffer_flip_prepare
  in the places we set the bits now and intel_frontbuffer_flip_complete
  where we're calling the flip mentioned above?
 
 
  Matt
 
 
  This patch here should have 0 effect (presuming I'm reading code
  correctly), so what kinf of bug exactly are you seeing?
 
 Yeah, after I sent the patch I was thinking about that: that it should
 have 0 effect,
 but the symptom that this apparently fixed here is that PSR wasn't
 starting at all without
 doing something like going to fbcon and come back to X.
 Something similar what Sonika had told on that psr-skl thread where
 she couldn't get psr working on login screen.
 After this patch I didn't' have to change back and forth to fbcon to
 make psr work.
 
 Maybe just a coincidence, but anyway I believe the way it is nowadays
 it is wrong. I believe frontbuffer bits and calls should be done at
 commit step, not under prepare.

Hm, I think we've had this issue for a long time, even on older
frontbuffer tracking code. I think it's time to throw lots of debug printk
at the problem and figure out why psr is disabled in this case.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-02 Thread Rodrigo Vivi
frontbuffer bits must be updated during commit times not on atomica prepare
one, otherwise we have a risk of false positive.

Cc Daniel Vetter daniel.vet...@ffwll.ch
Cc: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
 drivers/gpu/drm/i915/intel_display.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 73b5923..1e83124 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12176,9 +12176,6 @@ finish:
if (intel_crtc-active) {
if (intel_crtc-cursor_width != state-base.crtc_w)
intel_crtc-atomic.update_wm = true;
-
-   intel_crtc-atomic.fb_bits |=
-   INTEL_FRONTBUFFER_CURSOR(intel_crtc-pipe);
}
 
return ret;
@@ -12220,8 +12217,11 @@ update:
intel_crtc-cursor_width = state-base.crtc_w;
intel_crtc-cursor_height = state-base.crtc_h;
 
-   if (intel_crtc-active)
+   if (intel_crtc-active) {
intel_crtc_update_cursor(crtc, state-visible);
+   intel_crtc-atomic.fb_bits |=
+   INTEL_FRONTBUFFER_CURSOR(intel_crtc-pipe);
+   }
 }
 
 static const struct drm_plane_funcs intel_cursor_plane_funcs = {
-- 
2.1.0

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


Re: [Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

2015-02-02 Thread shuang . he
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: 
shuang...@intel.com)
Task id: 5699
-Summary-
Platform  Delta  drm-intel-nightly  Series Applied
PNV  353/353  353/353
ILK  353/353  353/353
SNB  +1 400/422  401/422
IVB  +1-1  485/487  485/487
BYT  296/296  296/296
HSW  +1-1  507/508  507/508
BDW  401/402  401/402
-Detailed-
Platform  Testdrm-intel-nightly  Series 
Applied
*SNB  igt_kms_flip_event_leak  NSPT(6, M35M22)  PASS(1, M35)
 IVB  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance  
DMESG_WARN(9, M34M21)PASS(10, M4M34)  PASS(1, M34)
 IVB  igt_gem_storedw_batches_loop_secure-dispatch  DMESG_WARN(1, 
M34)PASS(9, M34M4)  DMESG_WARN(1, M34)
*HSW  igt_gem_pwrite_pread_display-copy-performance  PASS(7, M40M20)  
DMESG_WARN(1, M40)
 HSW  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance  
DMESG_WARN(2, M40)PASS(23, M40M20)  PASS(1, M40)
Note: You need to pay more attention to line start with '*'
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx