diff mbox

drm/i915: Avoid vblank counter for gen9+

Message ID 1455210047-1574-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi Feb. 11, 2016, 5 p.m. UTC
Framecounter register is read-only so DMC cannot restore it
after exiting DC5 and DC6.

Easiest way to go is to avoid the counter and use vblank
interruptions for this platform and for all the following
ones since DMC came to stay. At least while we can't change
this register to read-write.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

kernel test robot Feb. 12, 2016, 1:19 a.m. UTC | #1
Hi Rodrigo,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.5-rc3 next-20160211]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Rodrigo-Vivi/drm-i915-Avoid-vblank-counter-for-gen9/20160212-090608
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x011-201606 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_irq.c: In function 'intel_irq_init':
>> drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: comparison of constant '9' with boolean expression is always false [-Wbool-compare]
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
                                                          ^
>> drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]

vim +/9 +4578 drivers/gpu/drm/i915/i915_irq.c

  4562		} else if (IS_GEN2(dev_priv)) {
  4563			dev->max_vblank_count = 0;
  4564			dev->driver->get_vblank_counter = i8xx_get_vblank_counter;
  4565		} else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) {
  4566			dev->max_vblank_count = 0xffffffff; /* full 32 bit counter */
  4567			dev->driver->get_vblank_counter = g4x_get_vblank_counter;
  4568		} else {
  4569			dev->driver->get_vblank_counter = i915_get_vblank_counter;
  4570			dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
  4571		}
  4572	
  4573		/*
  4574		 * Opt out of the vblank disable timer on everything except gen2.
  4575		 * Gen2 doesn't have a hardware frame counter and so depends on
  4576		 * vblank interrupts to produce sane vblank seuquence numbers.
  4577		 */
> 4578		if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
  4579			dev->vblank_disable_immediate = true;
  4580	
  4581		dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
  4582		dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
  4583	
  4584		if (IS_CHERRYVIEW(dev_priv)) {
  4585			dev->driver->irq_handler = cherryview_irq_handler;
  4586			dev->driver->irq_preinstall = cherryview_irq_preinstall;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot Feb. 12, 2016, 1:32 a.m. UTC | #2
Hi Rodrigo,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.5-rc3 next-20160211]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Rodrigo-Vivi/drm-i915-Avoid-vblank-counter-for-gen9/20160212-090608
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x000-201606 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/linux/sysrq.h:18,
                    from drivers/gpu/drm/i915/i915_irq.c:31:
   drivers/gpu/drm/i915/i915_irq.c: In function 'intel_irq_init':
   drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: comparison of constant '9' with boolean expression is always false [-Wbool-compare]
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
                                                          ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                               ^
>> drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of macro 'if'
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
     ^
   drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
                                                          ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                               ^
>> drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of macro 'if'
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
     ^
   drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: comparison of constant '9' with boolean expression is always false [-Wbool-compare]
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
                                                          ^
   include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                                           ^
>> drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of macro 'if'
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
     ^
   drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
                                                          ^
   include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                                           ^
>> drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of macro 'if'
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
     ^
   drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: comparison of constant '9' with boolean expression is always false [-Wbool-compare]
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
                                                          ^
   include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^
>> drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of macro 'if'
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
     ^
   drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: logical not is only applied to the left hand side of comparison [-Wlogical-not-parentheses]
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
                                                          ^
   include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^
>> drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of macro 'if'
     if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
     ^

vim +/if +4578 drivers/gpu/drm/i915/i915_irq.c

  4562		} else if (IS_GEN2(dev_priv)) {
  4563			dev->max_vblank_count = 0;
  4564			dev->driver->get_vblank_counter = i8xx_get_vblank_counter;
  4565		} else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) {
  4566			dev->max_vblank_count = 0xffffffff; /* full 32 bit counter */
  4567			dev->driver->get_vblank_counter = g4x_get_vblank_counter;
  4568		} else {
  4569			dev->driver->get_vblank_counter = i915_get_vblank_counter;
  4570			dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
  4571		}
  4572	
  4573		/*
  4574		 * Opt out of the vblank disable timer on everything except gen2.
  4575		 * Gen2 doesn't have a hardware frame counter and so depends on
  4576		 * vblank interrupts to produce sane vblank seuquence numbers.
  4577		 */
> 4578		if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
  4579			dev->vblank_disable_immediate = true;
  4580	
  4581		dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
  4582		dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
  4583	
  4584		if (IS_CHERRYVIEW(dev_priv)) {
  4585			dev->driver->irq_handler = cherryview_irq_handler;
  4586			dev->driver->irq_preinstall = cherryview_irq_preinstall;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
David Weinehall Feb. 12, 2016, 9:34 a.m. UTC | #3
On Thu, Feb 11, 2016 at 09:00:47AM -0800, Rodrigo Vivi wrote:
> Framecounter register is read-only so DMC cannot restore it
> after exiting DC5 and DC6.
> 
> Easiest way to go is to avoid the counter and use vblank
> interruptions for this platform and for all the following
> ones since DMC came to stay. At least while we can't change
> this register to read-write.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 25a8937..c294a4b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4556,7 +4556,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  
>  	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>  
> -	if (IS_GEN2(dev_priv)) {
> +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> +		dev->max_vblank_count = 0;
> +		dev->driver->get_vblank_counter = g4x_get_vblank_counter;
> +	} else if (IS_GEN2(dev_priv)) {
>  		dev->max_vblank_count = 0;
>  		dev->driver->get_vblank_counter = i8xx_get_vblank_counter;
>  	} else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) {
> @@ -4572,7 +4575,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  	 * Gen2 doesn't have a hardware frame counter and so depends on
>  	 * vblank interrupts to produce sane vblank seuquence numbers.
>  	 */
> -	if (!IS_GEN2(dev_priv))
> +	if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

I think this should be:

if (INTEL_INFO(dev_priv)->gen < 9)

If gen < 9, then IS_GEN2 is always true, also ! has higher precedence
than >=, so you're essentially comparing whether the logical negation of
INTEL_INFO(dev_priv)->gen is >= 9.


Kind regards, David
Rodrigo Vivi Feb. 12, 2016, 3:51 p.m. UTC | #4
doh!! sent the wrong version... going to sent the right one now...

On Fri, 2016-02-12 at 09:32 +0800, kbuild test robot wrote:
> Hi Rodrigo,

> 

> [auto build test WARNING on drm-intel/for-linux-next]

> [also build test WARNING on v4.5-rc3 next-20160211]

> [if your patch is applied to the wrong git tree, please drop us a 

> note to help improving the system]

> 

> url:    https://github.com/0day-ci/linux/commits/Rodrigo-Vivi/drm-i91

> 5-Avoid-vblank-counter-for-gen9/20160212-090608

> base:   git://anongit.freedesktop.org/drm-intel for-linux-next

> config: x86_64-randconfig-x000-201606 (attached as .config)

> reproduce:

>         # save the attached .config to linux build tree

>         make ARCH=x86_64 

> 

> All warnings (new ones prefixed by >>):

> 

>    In file included from include/uapi/linux/stddef.h:1:0,

>                     from include/linux/stddef.h:4,

>                     from include/uapi/linux/posix_types.h:4,

>                     from include/uapi/linux/types.h:13,

>                     from include/linux/types.h:5,

>                     from include/linux/sysrq.h:18,

>                     from drivers/gpu/drm/i915/i915_irq.c:31:

>    drivers/gpu/drm/i915/i915_irq.c: In function 'intel_irq_init':

>    drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: comparison of 

> constant '9' with boolean expression is always false [-Wbool-compare]

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>                                                           ^

>    include/linux/compiler.h:147:28: note: in definition of macro 

> '__trace_if'

>      if (__builtin_constant_p((cond)) ? !!(cond) :   \

>                                ^

> > > drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of 

> > > macro 'if'

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>      ^

>    drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: logical not is 

> only applied to the left hand side of comparison [-Wlogical-not

> -parentheses]

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>                                                           ^

>    include/linux/compiler.h:147:28: note: in definition of macro 

> '__trace_if'

>      if (__builtin_constant_p((cond)) ? !!(cond) :   \

>                                ^

> > > drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of 

> > > macro 'if'

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>      ^

>    drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: comparison of 

> constant '9' with boolean expression is always false [-Wbool-compare]

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>                                                           ^

>    include/linux/compiler.h:147:40: note: in definition of macro 

> '__trace_if'

>      if (__builtin_constant_p((cond)) ? !!(cond) :   \

>                                            ^

> > > drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of 

> > > macro 'if'

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>      ^

>    drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: logical not is 

> only applied to the left hand side of comparison [-Wlogical-not

> -parentheses]

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>                                                           ^

>    include/linux/compiler.h:147:40: note: in definition of macro 

> '__trace_if'

>      if (__builtin_constant_p((cond)) ? !!(cond) :   \

>                                            ^

> > > drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of 

> > > macro 'if'

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>      ^

>    drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: comparison of 

> constant '9' with boolean expression is always false [-Wbool-compare]

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>                                                           ^

>    include/linux/compiler.h:158:16: note: in definition of macro 

> '__trace_if'

>       ______r = !!(cond);     \

>                    ^

> > > drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of 

> > > macro 'if'

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>      ^

>    drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: logical not is 

> only applied to the left hand side of comparison [-Wlogical-not

> -parentheses]

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>                                                           ^

>    include/linux/compiler.h:158:16: note: in definition of macro 

> '__trace_if'

>       ______r = !!(cond);     \

>                    ^

> > > drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of 

> > > macro 'if'

>      if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

>      ^

> 

> vim +/if +4578 drivers/gpu/drm/i915/i915_irq.c

> 

>   4562		} else if (IS_GEN2(dev_priv)) {

>   4563			dev->max_vblank_count = 0;

>   4564			dev->driver->get_vblank_counter = 

> i8xx_get_vblank_counter;

>   4565		} else if (IS_G4X(dev_priv) || 

> INTEL_INFO(dev_priv)->gen >= 5) {

>   4566			dev->max_vblank_count = 0xffffffff; /* 

> full 32 bit counter */

>   4567			dev->driver->get_vblank_counter = 

> g4x_get_vblank_counter;

>   4568		} else {

>   4569			dev->driver->get_vblank_counter = 

> i915_get_vblank_counter;

>   4570			dev->max_vblank_count = 0xffffff; /* 

> only 24 bits of frame count */

>   4571		}

>   4572	

>   4573		/*

>   4574		 * Opt out of the vblank disable timer on 

> everything except gen2.

>   4575		 * Gen2 doesn't have a hardware frame counter 

> and so depends on

>   4576		 * vblank interrupts to produce sane vblank 

> seuquence numbers.

>   4577		 */

> > 4578		if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)

> > ->gen >= 9)

>   4579			dev->vblank_disable_immediate = true;

>   4580	

>   4581		dev->driver->get_vblank_timestamp = 

> i915_get_vblank_timestamp;

>   4582		dev->driver->get_scanout_position = 

> i915_get_crtc_scanoutpos;

>   4583	

>   4584		if (IS_CHERRYVIEW(dev_priv)) {

>   4585			dev->driver->irq_handler = 

> cherryview_irq_handler;

>   4586			dev->driver->irq_preinstall = 

> cherryview_irq_preinstall;

> 

> ---

> 0-DAY kernel test infrastructure                Open Source 

> Technology Center

> https://lists.01.org/pipermail/kbuild-all                   Intel Cor

> poration
Rodrigo Vivi Feb. 12, 2016, 3:57 p.m. UTC | #5
On Fri, 2016-02-12 at 11:34 +0200, David Weinehall wrote:
> On Thu, Feb 11, 2016 at 09:00:47AM -0800, Rodrigo Vivi wrote:

> > Framecounter register is read-only so DMC cannot restore it

> > after exiting DC5 and DC6.

> > 

> > Easiest way to go is to avoid the counter and use vblank

> > interruptions for this platform and for all the following

> > ones since DMC came to stay. At least while we can't change

> > this register to read-write.

> > 

> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > ---

> >  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--

> >  1 file changed, 5 insertions(+), 2 deletions(-)

> > 

> > diff --git a/drivers/gpu/drm/i915/i915_irq.c 

> > b/drivers/gpu/drm/i915/i915_irq.c

> > index 25a8937..c294a4b 100644

> > --- a/drivers/gpu/drm/i915/i915_irq.c

> > +++ b/drivers/gpu/drm/i915/i915_irq.c

> > @@ -4556,7 +4556,10 @@ void intel_irq_init(struct drm_i915_private 

> > *dev_priv)

> >  

> >  	pm_qos_add_request(&dev_priv->pm_qos, 

> > PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);

> >  

> > -	if (IS_GEN2(dev_priv)) {

> > +	if (INTEL_INFO(dev_priv)->gen >= 9) {

> > +		dev->max_vblank_count = 0;

> > +		dev->driver->get_vblank_counter = 

> > g4x_get_vblank_counter;

> > +	} else if (IS_GEN2(dev_priv)) {

> >  		dev->max_vblank_count = 0;

> >  		dev->driver->get_vblank_counter = 

> > i8xx_get_vblank_counter;

> >  	} else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen 

> > >= 5) {

> > @@ -4572,7 +4575,7 @@ void intel_irq_init(struct drm_i915_private 

> > *dev_priv)

> >  	 * Gen2 doesn't have a hardware frame counter and so 

> > depends on

> >  	 * vblank interrupts to produce sane vblank seuquence 

> > numbers.

> >  	 */

> > -	if (!IS_GEN2(dev_priv))

> > +	if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)

> 

> I think this should be:

> 

> if (INTEL_INFO(dev_priv)->gen < 9)


Yeap, < 9 is better...

> 

> If gen < 9, then IS_GEN2 is always true, also ! has higher precedence

> than >=, so you're essentially comparing whether the logical negation 

> of

> INTEL_INFO(dev_priv)->gen is >= 9.


but it is also !gen2

So I believe the right is

if (INTEL_INFO(dev_priv)->gen < 9 && !IS_GEN2(dev_priv))

agree?

> 

> 

> Kind regards, David
Ville Syrjälä Feb. 12, 2016, 4:04 p.m. UTC | #6
On Thu, Feb 11, 2016 at 09:00:47AM -0800, Rodrigo Vivi wrote:
> Framecounter register is read-only so DMC cannot restore it
> after exiting DC5 and DC6.
> 
> Easiest way to go is to avoid the counter and use vblank
> interruptions for this platform and for all the following
> ones since DMC came to stay. At least while we can't change
> this register to read-write.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 25a8937..c294a4b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4556,7 +4556,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  
>  	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>  
> -	if (IS_GEN2(dev_priv)) {
> +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> +		dev->max_vblank_count = 0;
> +		dev->driver->get_vblank_counter = g4x_get_vblank_counter;

= i8xx_get_vblank_counter

> +	} else if (IS_GEN2(dev_priv)) {
>  		dev->max_vblank_count = 0;
>  		dev->driver->get_vblank_counter = i8xx_get_vblank_counter;
>  	} else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) {
> @@ -4572,7 +4575,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  	 * Gen2 doesn't have a hardware frame counter and so depends on
>  	 * vblank interrupts to produce sane vblank seuquence numbers.
>  	 */
> -	if (!IS_GEN2(dev_priv))
> +	if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
>  		dev->vblank_disable_immediate = true;

You can just drop this part.

>  
>  	dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Rodrigo Vivi Feb. 12, 2016, 4:21 p.m. UTC | #7
Oh, actually please just ignore this patch completely.
More work here need to be done. This apparently helped on the issue
that I was facing here but doesnt' solve completely.

On Fri, 2016-02-12 at 07:58 -0800, Rodrigo Vivi wrote:
> On Fri, 2016-02-12 at 11:34 +0200, David Weinehall wrote:

> > On Thu, Feb 11, 2016 at 09:00:47AM -0800, Rodrigo Vivi wrote:

> > > Framecounter register is read-only so DMC cannot restore it

> > > after exiting DC5 and DC6.

> > > 

> > > Easiest way to go is to avoid the counter and use vblank

> > > interruptions for this platform and for all the following

> > > ones since DMC came to stay. At least while we can't change

> > > this register to read-write.

> > > 

> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > > ---

> > >  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--

> > >  1 file changed, 5 insertions(+), 2 deletions(-)

> > > 

> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c 

> > > b/drivers/gpu/drm/i915/i915_irq.c

> > > index 25a8937..c294a4b 100644

> > > --- a/drivers/gpu/drm/i915/i915_irq.c

> > > +++ b/drivers/gpu/drm/i915/i915_irq.c

> > > @@ -4556,7 +4556,10 @@ void intel_irq_init(struct 

> > > drm_i915_private 

> > > *dev_priv)

> > >  

> > >  	pm_qos_add_request(&dev_priv->pm_qos, 

> > > PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);

> > >  

> > > -	if (IS_GEN2(dev_priv)) {

> > > +	if (INTEL_INFO(dev_priv)->gen >= 9) {

> > > +		dev->max_vblank_count = 0;

> > > +		dev->driver->get_vblank_counter = 

> > > g4x_get_vblank_counter;

> > > +	} else if (IS_GEN2(dev_priv)) {

> > >  		dev->max_vblank_count = 0;

> > >  		dev->driver->get_vblank_counter = 

> > > i8xx_get_vblank_counter;

> > >  	} else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen 

> > > > = 5) {

> > > @@ -4572,7 +4575,7 @@ void intel_irq_init(struct drm_i915_private 

> > > *dev_priv)

> > >  	 * Gen2 doesn't have a hardware frame counter and so 

> > > depends on

> > >  	 * vblank interrupts to produce sane vblank seuquence 

> > > numbers.

> > >  	 */

> > > -	if (!IS_GEN2(dev_priv))

> > > +	if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 

> > > 9)

> > 

> > I think this should be:

> > 

> > if (INTEL_INFO(dev_priv)->gen < 9)

> 

> Yeap, < 9 is better...

> 

> > 

> > If gen < 9, then IS_GEN2 is always true, also ! has higher 

> > precedence

> > than >=, so you're essentially comparing whether the logical 

> > negation 

> > of

> > INTEL_INFO(dev_priv)->gen is >= 9.

> 

> but it is also !gen2

> 

> So I believe the right is

> 

> if (INTEL_INFO(dev_priv)->gen < 9 && !IS_GEN2(dev_priv))

> 

> agree?

> 

> > 

> > 

> > Kind regards, David
Daniel Vetter Feb. 16, 2016, 3:50 p.m. UTC | #8
On Thu, Feb 11, 2016 at 09:00:47AM -0800, Rodrigo Vivi wrote:
> Framecounter register is read-only so DMC cannot restore it
> after exiting DC5 and DC6.
> 
> Easiest way to go is to avoid the counter and use vblank
> interruptions for this platform and for all the following
> ones since DMC came to stay. At least while we can't change
> this register to read-write.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Now my comments also in public:
- Do we still get reasonable dc5 residency with this - it means we'll keep
  vblank irq running forever.

- I'm a bit unclear on what exactly this fixes - have you tested that
  long-lasting vblank waits are still accurate? Just want to make sure we
  don't just paper over the issue and desktops can still get stuck waiting
  for a vblank.

Just a bit suprised that the only problem is the framecounter, and not
that vblanks stop happening too.

We need to also know these details for the proper fix, which will involve
grabbing power well references (might need a new one for vblank
interrupts) to make sure.

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 25a8937..c294a4b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4556,7 +4556,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  
>  	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
>  
> -	if (IS_GEN2(dev_priv)) {
> +	if (INTEL_INFO(dev_priv)->gen >= 9) {
> +		dev->max_vblank_count = 0;
> +		dev->driver->get_vblank_counter = g4x_get_vblank_counter;
> +	} else if (IS_GEN2(dev_priv)) {
>  		dev->max_vblank_count = 0;
>  		dev->driver->get_vblank_counter = i8xx_get_vblank_counter;
>  	} else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) {
> @@ -4572,7 +4575,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  	 * Gen2 doesn't have a hardware frame counter and so depends on
>  	 * vblank interrupts to produce sane vblank seuquence numbers.
>  	 */
> -	if (!IS_GEN2(dev_priv))
> +	if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
>  		dev->vblank_disable_immediate = true;
>  
>  	dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 25a8937..c294a4b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4556,7 +4556,10 @@  void intel_irq_init(struct drm_i915_private *dev_priv)
 
 	pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE);
 
-	if (IS_GEN2(dev_priv)) {
+	if (INTEL_INFO(dev_priv)->gen >= 9) {
+		dev->max_vblank_count = 0;
+		dev->driver->get_vblank_counter = g4x_get_vblank_counter;
+	} else if (IS_GEN2(dev_priv)) {
 		dev->max_vblank_count = 0;
 		dev->driver->get_vblank_counter = i8xx_get_vblank_counter;
 	} else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) {
@@ -4572,7 +4575,7 @@  void intel_irq_init(struct drm_i915_private *dev_priv)
 	 * Gen2 doesn't have a hardware frame counter and so depends on
 	 * vblank interrupts to produce sane vblank seuquence numbers.
 	 */
-	if (!IS_GEN2(dev_priv))
+	if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9)
 		dev->vblank_disable_immediate = true;
 
 	dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;