[PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Wang Elaine]

From: Elaine Wang <elaine.wang@intel.com>

Some platforms don't have display. To avoid accessing the
non-existent registers, check HAS_PCH_NOP before invoking
display IRQ install or reset function.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Elaine Wang <elaine.wang@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0b119b9..369a038 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct drm_device *dev)
 						   POWER_DOMAIN_PIPE(pipe)))
 			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
 
-	GEN5_IRQ_RESET(GEN8_DE_PORT_);
-	GEN5_IRQ_RESET(GEN8_DE_MISC_);
+	if (!HAS_PCH_NOP(dev_priv)) {
+		GEN5_IRQ_RESET(GEN8_DE_PORT_);
+		GEN5_IRQ_RESET(GEN8_DE_MISC_);
+	}
 	GEN5_IRQ_RESET(GEN8_PCU_);
 
 	if (HAS_PCH_SPLIT(dev_priv))
@@ -3414,7 +3416,9 @@ static int gen8_irq_postinstall(struct drm_device *dev)
 		ibx_irq_pre_postinstall(dev);
 
 	gen8_gt_irq_postinstall(dev_priv);
-	gen8_de_irq_postinstall(dev_priv);
+
+	if (!HAS_PCH_NOP(dev_priv))
+		gen8_de_irq_postinstall(dev_priv);
 
 	if (HAS_PCH_SPLIT(dev_priv))
 		ibx_irq_postinstall(dev);
-- 
1.9.1

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

✗ Fi.CI.BAT: failure for drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Patchwork]

== Series Details ==

Series: drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
URL   : https://patchwork.freedesktop.org/series/16678/
State : failure

== Summary ==

Series 16678v1 drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
https://patchwork.freedesktop.org/api/1.0/series/16678/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload-inject:
                pass       -> INCOMPLETE (fi-kbl-7500u)

fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:7    pass:6    dwarn:0   dfail:0   fail:0   skip:0  
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

f6a248e2507f98d7eb1f4fec8cfcbf685a33d289 drm-tip: 2016y-12m-10d-21h-47m-23s UTC integration manifest
538d2ec drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

== Logs ==

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

Re: ✗ Fi.CI.BAT: failure for drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Wang, Elaine]

Reply to correct thread.

The failed case is KBL hang. This patch shouldn't impact platforms that have non-zero num_pipes.
So the KBL hang isn't a regression caused by this patch. According to below kernel log before hang
(https://intel-gfx-ci.01.org/CI/Patchwork_3266/fi-kbl-7500u/dmesg-during.log),
It should be the same issue as Bug 98670 - [BAT] WARN_ON(dev_priv->gt.awake) during 
drv_module_reload_basic (https://bugs.freedesktop.org/show_bug.cgi?id=98670), 

[   25.000193] ------------[ cut here ]------------
[   25.000227] WARNING: CPU: 3 PID: 6396 at drivers/gpu/drm/i915/i915_gem.c:4241 i915_gem_suspend+0x181/0x190 [i915]
[   25.000228] WARN_ON(dev_priv->gt.awake)
[   25.000229] Modules linked in:
[   25.000231]  i915(-) snd_hda_codec_hdmi snd_hda_codec_realtek x86_pkg_temp_thermal intel_powerclamp snd_hda_codec_generic coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_codec snd_hwdep snd_hda_core snd_pcm mei_me mei e1000e ptp pps_core i2c_hid [last unloaded: snd_hda_intel]
[   25.000249] CPU: 3 PID: 6396 Comm: drv_module_relo Tainted: G     U          4.9.0-rc8-CI-Patchwork_3266+ #1
[   25.000250] Hardware name: GIGABYTE GB-BKi7A-7500/MFLP7AP-00, BIOS F1 07/27/2016
[   25.000252]  ffffc90000547d18 ffffffff81435bf5 ffffc90000547d68 0000000000000000
[   25.000256]  ffffc90000547d58 ffffffff8107e4d6 0000109100000000 ffff880257a30000
[   25.000259]  0000000000000000 ffff880257a30068 ffffffffa01370c0 0000000000000000
[   25.000263] Call Trace:
[   25.000268]  [<ffffffff81435bf5>] dump_stack+0x67/0x92
[   25.000271]  [<ffffffff8107e4d6>] __warn+0xc6/0xe0
[   25.000274]  [<ffffffff8107e53a>] warn_slowpath_fmt+0x4a/0x50
[   25.000298]  [<ffffffffa0049a21>] i915_gem_suspend+0x181/0x190 [i915]
[   25.000314]  [<ffffffffa0006f4e>] i915_driver_unload+0x1e/0x190 [i915]
[   25.000331]  [<ffffffffa0010b24>] i915_pci_remove+0x14/0x20 [i915]
[   25.000334]  [<ffffffff81485954>] pci_device_remove+0x34/0xb0
[   25.000339]  [<ffffffff81584d8c>] __device_release_driver+0x9c/0x150
[   25.000340]  [<ffffffff81585906>] driver_detach+0xb6/0xc0
[   25.000343]  [<ffffffff81584823>] bus_remove_driver+0x53/0xd0
[   25.000345]  [<ffffffff815863c7>] driver_unregister+0x27/0x50
[   25.000347]  [<ffffffff814842f5>] pci_unregister_driver+0x25/0x70
[   25.000375]  [<ffffffffa00f55e4>] i915_exit+0x1a/0x71 [i915]
[   25.000378]  [<ffffffff8111a863>] SyS_delete_module+0x193/0x1e0
[   25.000380]  [<ffffffff81823d6e>] entry_SYSCALL_64_fastpath+0x1c/0xb1

Thanks,
Ealine
> -----Original Message-----
> From: Patchwork [mailto:patchwork@emeril.freedesktop.org]
> Sent: Monday, December 12, 2016 3:53 PM
> To: Wang, Elaine <elaine.wang@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: ✗ Fi.CI.BAT: failure for drm/i915: Check HAS_PCH_NOP when install
> or reset dispaly IRQ
> 
> == Series Details ==
> 
> Series: drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
> URL   : https://patchwork.freedesktop.org/series/16678/
> State : failure
> 
> == Summary ==
> 
> Series 16678v1 drm/i915: Check HAS_PCH_NOP when install or reset dispaly
> IRQ
> https://patchwork.freedesktop.org/api/1.0/series/16678/revisions/1/mbox/
> 
> Test drv_module_reload:
>         Subgroup basic-reload-inject:
>                 pass       -> INCOMPLETE (fi-kbl-7500u)
> 
> fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14
> fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39
> fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27
> fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31
> fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19
> fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19
> fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52
> fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21
> fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21
> fi-kbl-7500u     total:7    pass:6    dwarn:0   dfail:0   fail:0   skip:0
> fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13
> fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20
> fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20
> fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13
> fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32
> 
> f6a248e2507f98d7eb1f4fec8cfcbf685a33d289 drm-tip: 2016y-12m-10d-21h-
> 47m-23s UTC integration manifest 538d2ec drm/i915: Check HAS_PCH_NOP
> when install or reset dispaly IRQ
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3266/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Joonas Lahtinen]

On ma, 2016-12-12 at 14:57 +0800, Wang Elaine wrote:
> From: Elaine Wang <elaine.wang@intel.com>
> 
> Some platforms don't have display. To avoid accessing the
> non-existent registers, check HAS_PCH_NOP before invoking
> display IRQ install or reset function.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Elaine Wang <elaine.wang@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Unless there are objections from Ville or Chris, I'll merge the patch.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Ville Syrjälä]

On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
> From: Elaine Wang <elaine.wang@intel.com>
> 
> Some platforms don't have display. To avoid accessing the
> non-existent registers, check HAS_PCH_NOP before invoking
> display IRQ install or reset function.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 0b119b9..369a038 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct drm_device *dev)
>  						   POWER_DOMAIN_PIPE(pipe)))
>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>  
> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> +	if (!HAS_PCH_NOP(dev_priv)) {
> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
> +	}

Hmm. These are north side registers so looking at PCH_NOP feels
questionable.

>  	GEN5_IRQ_RESET(GEN8_PCU_);
>  
>  	if (HAS_PCH_SPLIT(dev_priv))
> @@ -3414,7 +3416,9 @@ static int gen8_irq_postinstall(struct drm_device *dev)
>  		ibx_irq_pre_postinstall(dev);
>  
>  	gen8_gt_irq_postinstall(dev_priv);
> -	gen8_de_irq_postinstall(dev_priv);
> +
> +	if (!HAS_PCH_NOP(dev_priv))
> +		gen8_de_irq_postinstall(dev_priv);
>  
>  	if (HAS_PCH_SPLIT(dev_priv))
>  		ibx_irq_postinstall(dev);
> -- 
> 1.9.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Jani Nikula]

On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
>> From: Elaine Wang <elaine.wang@intel.com>
>> 
>> Some platforms don't have display. To avoid accessing the
>> non-existent registers, check HAS_PCH_NOP before invoking
>> display IRQ install or reset function.
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 0b119b9..369a038 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct drm_device *dev)
>>  						   POWER_DOMAIN_PIPE(pipe)))
>>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>>  
>> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
>> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> +	if (!HAS_PCH_NOP(dev_priv)) {
>> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
>> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> +	}
>
> Hmm. These are north side registers so looking at PCH_NOP feels
> questionable.

Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.

Jani.


>
>>  	GEN5_IRQ_RESET(GEN8_PCU_);
>>  
>>  	if (HAS_PCH_SPLIT(dev_priv))
>> @@ -3414,7 +3416,9 @@ static int gen8_irq_postinstall(struct drm_device *dev)
>>  		ibx_irq_pre_postinstall(dev);
>>  
>>  	gen8_gt_irq_postinstall(dev_priv);
>> -	gen8_de_irq_postinstall(dev_priv);
>> +
>> +	if (!HAS_PCH_NOP(dev_priv))
>> +		gen8_de_irq_postinstall(dev_priv);
>>  
>>  	if (HAS_PCH_SPLIT(dev_priv))
>>  		ibx_irq_postinstall(dev);
>> -- 
>> 1.9.1

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

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Wang, Elaine]

> On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
> >> From: Elaine Wang <elaine.wang@intel.com>
> >>
> >> Some platforms don't have display. To avoid accessing the
> >> non-existent registers, check HAS_PCH_NOP before invoking display IRQ
> >> install or reset function.
> >>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> >> b/drivers/gpu/drm/i915/i915_irq.c index 0b119b9..369a038 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct drm_device
> *dev)
> >>
> POWER_DOMAIN_PIPE(pipe)))
> >>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
> >>
> >> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> +	if (!HAS_PCH_NOP(dev_priv)) {
> >> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> +	}
> >
> > Hmm. These are north side registers so looking at PCH_NOP feels
> > questionable.
> 
> Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.
> 
> Jani.

I thought HAS_PCH_NOP had same meaning as num_pipes == 0 because I saw following
code in i915_drv.c. Is there any exception?

https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_drv.c?h=drm-intel-nightly#n145
static void intel_detect_pch(struct drm_i915_private *dev_priv)
{
	struct pci_dev *pch = NULL;

	/* In all current cases, num_pipes is equivalent to the PCH_NOP setting
	 * (which really amounts to a PCH but no South Display).
	 */
	if (INTEL_INFO(dev_priv)->num_pipes == 0) {
		dev_priv->pch_type = PCH_NOP;
		return;
	}

Thanks,
Elaine
> 
> 
> >
> >>  	GEN5_IRQ_RESET(GEN8_PCU_);
> >>
> >>  	if (HAS_PCH_SPLIT(dev_priv))
> >> @@ -3414,7 +3416,9 @@ static int gen8_irq_postinstall(struct drm_device
> *dev)
> >>  		ibx_irq_pre_postinstall(dev);
> >>
> >>  	gen8_gt_irq_postinstall(dev_priv);
> >> -	gen8_de_irq_postinstall(dev_priv);
> >> +
> >> +	if (!HAS_PCH_NOP(dev_priv))
> >> +		gen8_de_irq_postinstall(dev_priv);
> >>
> >>  	if (HAS_PCH_SPLIT(dev_priv))
> >>  		ibx_irq_postinstall(dev);
> >> --
> >> 1.9.1
> 
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Wang, Elaine]

Hi Jani, Ville,

Any comment about the "PCH_NOP" vs "num_pipes == 0"?

Thanks,
Elaine
> On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
> >> From: Elaine Wang <elaine.wang@intel.com>
> >>
> >> Some platforms don't have display. To avoid accessing the 
> >> non-existent registers, check HAS_PCH_NOP before invoking display 
> >> IRQ install or reset function.
> >>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
> >> b/drivers/gpu/drm/i915/i915_irq.c index 0b119b9..369a038 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct drm_device
> *dev)
> >>
> POWER_DOMAIN_PIPE(pipe)))
> >>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
> >>
> >> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> +	if (!HAS_PCH_NOP(dev_priv)) {
> >> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> +	}
> >
> > Hmm. These are north side registers so looking at PCH_NOP feels 
> > questionable.
> 
> Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.
> 
> Jani.

I thought HAS_PCH_NOP had same meaning as num_pipes == 0 because I saw following code in i915_drv.c. Is there any exception?

https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_drv.c?h=drm-intel-nightly#n145
static void intel_detect_pch(struct drm_i915_private *dev_priv) {
	struct pci_dev *pch = NULL;

	/* In all current cases, num_pipes is equivalent to the PCH_NOP setting
	 * (which really amounts to a PCH but no South Display).
	 */
	if (INTEL_INFO(dev_priv)->num_pipes == 0) {
		dev_priv->pch_type = PCH_NOP;
		return;
	}

Thanks,
Elaine
> 
> 
> >
> >>  	GEN5_IRQ_RESET(GEN8_PCU_);
> >>
> >>  	if (HAS_PCH_SPLIT(dev_priv))
> >> @@ -3414,7 +3416,9 @@ static int gen8_irq_postinstall(struct 
> >> drm_device
> *dev)
> >>  		ibx_irq_pre_postinstall(dev);
> >>
> >>  	gen8_gt_irq_postinstall(dev_priv);
> >> -	gen8_de_irq_postinstall(dev_priv);
> >> +
> >> +	if (!HAS_PCH_NOP(dev_priv))
> >> +		gen8_de_irq_postinstall(dev_priv);
> >>
> >>  	if (HAS_PCH_SPLIT(dev_priv))
> >>  		ibx_irq_postinstall(dev);
> >> --
> >> 1.9.1
> 
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Jani Nikula]

On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
> Hi Jani, Ville,
>
> Any comment about the "PCH_NOP" vs "num_pipes == 0"?
>
> Thanks,
> Elaine
>> On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
>> >> From: Elaine Wang <elaine.wang@intel.com>
>> >>
>> >> Some platforms don't have display. To avoid accessing the 
>> >> non-existent registers, check HAS_PCH_NOP before invoking display 
>> >> IRQ install or reset function.
>> >>
>> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> >> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
>> >>  1 file changed, 7 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> >> b/drivers/gpu/drm/i915/i915_irq.c index 0b119b9..369a038 100644
>> >> --- a/drivers/gpu/drm/i915/i915_irq.c
>> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> >> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct drm_device
>> *dev)
>> >>
>> POWER_DOMAIN_PIPE(pipe)))
>> >>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>> >>
>> >> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
>> >> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> >> +	if (!HAS_PCH_NOP(dev_priv)) {
>> >> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
>> >> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> >> +	}
>> >
>> > Hmm. These are north side registers so looking at PCH_NOP feels 
>> > questionable.
>> 
>> Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.
>> 
>> Jani.
>
> I thought HAS_PCH_NOP had same meaning as num_pipes == 0 because I saw
> following code in i915_drv.c. Is there any exception?
>
> https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_drv.c?h=drm-intel-nightly#n145
> static void intel_detect_pch(struct drm_i915_private *dev_priv) {
> 	struct pci_dev *pch = NULL;
>
> 	/* In all current cases, num_pipes is equivalent to the PCH_NOP setting
> 	 * (which really amounts to a PCH but no South Display).
> 	 */

The key is in this comment; "In all current cases", where "current" is
3½ years ago. IIRC this was written for some Xeons which did have a PCH
but no display. PCH_NOP is a kind of hack for those. Nowadays you don't
always have a PCH on gen 5+ (VLV, CHV, BXT, ...). You might have a PCH
but only need the North Display for some outputs. And I guess you might
still have a PCH but no display at all.

I'm just saying, we should not overload this hack to, say, cover
platforms that don't even have a PCH, or platforms that have a PCH but a
functioning North Display.

BR,
Jani.


> 	if (INTEL_INFO(dev_priv)->num_pipes == 0) {
> 		dev_priv->pch_type = PCH_NOP;
> 		return;
> 	}
>
> Thanks,
> Elaine
>> 
>> 
>> >
>> >>  	GEN5_IRQ_RESET(GEN8_PCU_);
>> >>
>> >>  	if (HAS_PCH_SPLIT(dev_priv))
>> >> @@ -3414,7 +3416,9 @@ static int gen8_irq_postinstall(struct 
>> >> drm_device
>> *dev)
>> >>  		ibx_irq_pre_postinstall(dev);
>> >>
>> >>  	gen8_gt_irq_postinstall(dev_priv);
>> >> -	gen8_de_irq_postinstall(dev_priv);
>> >> +
>> >> +	if (!HAS_PCH_NOP(dev_priv))
>> >> +		gen8_de_irq_postinstall(dev_priv);
>> >>
>> >>  	if (HAS_PCH_SPLIT(dev_priv))
>> >>  		ibx_irq_postinstall(dev);
>> >> --
>> >> 1.9.1
>> 
>> --
>> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Wang, Elaine]

> 
> On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
> > Hi Jani, Ville,
> >
> > Any comment about the "PCH_NOP" vs "num_pipes == 0"?
> >
> > Thanks,
> > Elaine
> >> On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
> >> >> From: Elaine Wang <elaine.wang@intel.com>
> >> >>
> >> >> Some platforms don't have display. To avoid accessing the
> >> >> non-existent registers, check HAS_PCH_NOP before invoking display
> >> >> IRQ install or reset function.
> >> >>
> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> >> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
> >> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> >> >> b/drivers/gpu/drm/i915/i915_irq.c index 0b119b9..369a038 100644
> >> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> >> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct
> >> >> drm_device
> >> *dev)
> >> >>
> >> POWER_DOMAIN_PIPE(pipe)))
> >> >>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
> >> >>
> >> >> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> >> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> >> +	if (!HAS_PCH_NOP(dev_priv)) {
> >> >> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> >> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> >> +	}
> >> >
> >> > Hmm. These are north side registers so looking at PCH_NOP feels
> >> > questionable.
> >>
> >> Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.
> >>
> >> Jani.
> >
> > I thought HAS_PCH_NOP had same meaning as num_pipes == 0 because I
> saw
> > following code in i915_drv.c. Is there any exception?
> >
> > https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_
> > drv.c?h=drm-intel-nightly#n145 static void intel_detect_pch(struct
> > drm_i915_private *dev_priv) {
> > 	struct pci_dev *pch = NULL;
> >
> > 	/* In all current cases, num_pipes is equivalent to the PCH_NOP
> setting
> > 	 * (which really amounts to a PCH but no South Display).
> > 	 */
> 
> The key is in this comment; "In all current cases", where "current" is 3½ years
> ago. IIRC this was written for some Xeons which did have a PCH but no
> display. PCH_NOP is a kind of hack for those. Nowadays you don't always
> have a PCH on gen 5+ (VLV, CHV, BXT, ...). You might have a PCH but only
> need the North Display for some outputs. And I guess you might still have a
> PCH but no display at all.
> 
> I'm just saying, we should not overload this hack to, say, cover platforms that
> don't even have a PCH, or platforms that have a PCH but a functioning North
> Display.
> 
> BR,
> Jani.
> 
I understand your point now. Thank you for explaining this. I'll update the patch and
Use num_pipes for checking whether display engine exists.
Thanks,
Elaine
> 
> > 	if (INTEL_INFO(dev_priv)->num_pipes == 0) {
> > 		dev_priv->pch_type = PCH_NOP;
> > 		return;
> > 	}
> >
> > Thanks,
> > Elaine
> >>
> >>
> >> >
> >> >>  	GEN5_IRQ_RESET(GEN8_PCU_);
> >> >>
> >> >>  	if (HAS_PCH_SPLIT(dev_priv))
> >> >> @@ -3414,7 +3416,9 @@ static int gen8_irq_postinstall(struct
> >> >> drm_device
> >> *dev)
> >> >>  		ibx_irq_pre_postinstall(dev);
> >> >>
> >> >>  	gen8_gt_irq_postinstall(dev_priv);
> >> >> -	gen8_de_irq_postinstall(dev_priv);
> >> >> +
> >> >> +	if (!HAS_PCH_NOP(dev_priv))
> >> >> +		gen8_de_irq_postinstall(dev_priv);
> >> >>
> >> >>  	if (HAS_PCH_SPLIT(dev_priv))
> >> >>  		ibx_irq_postinstall(dev);
> >> >> --
> >> >> 1.9.1
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Jani Nikula]

On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
>> 
>> On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
>> > Hi Jani, Ville,
>> >
>> > Any comment about the "PCH_NOP" vs "num_pipes == 0"?
>> >
>> > Thanks,
>> > Elaine
>> >> On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> >> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
>> >> >> From: Elaine Wang <elaine.wang@intel.com>
>> >> >>
>> >> >> Some platforms don't have display. To avoid accessing the
>> >> >> non-existent registers, check HAS_PCH_NOP before invoking display
>> >> >> IRQ install or reset function.
>> >> >>
>> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> >> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> >> >> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
>> >> >> ---
>> >> >>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
>> >> >>  1 file changed, 7 insertions(+), 3 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> >> >> b/drivers/gpu/drm/i915/i915_irq.c index 0b119b9..369a038 100644
>> >> >> --- a/drivers/gpu/drm/i915/i915_irq.c
>> >> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> >> >> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct
>> >> >> drm_device
>> >> *dev)
>> >> >>
>> >> POWER_DOMAIN_PIPE(pipe)))
>> >> >>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>> >> >>
>> >> >> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
>> >> >> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> >> >> +	if (!HAS_PCH_NOP(dev_priv)) {
>> >> >> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
>> >> >> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> >> >> +	}
>> >> >
>> >> > Hmm. These are north side registers so looking at PCH_NOP feels
>> >> > questionable.
>> >>
>> >> Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.
>> >>
>> >> Jani.
>> >
>> > I thought HAS_PCH_NOP had same meaning as num_pipes == 0 because I
>> saw
>> > following code in i915_drv.c. Is there any exception?
>> >
>> > https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_
>> > drv.c?h=drm-intel-nightly#n145 static void intel_detect_pch(struct
>> > drm_i915_private *dev_priv) {
>> > 	struct pci_dev *pch = NULL;
>> >
>> > 	/* In all current cases, num_pipes is equivalent to the PCH_NOP
>> setting
>> > 	 * (which really amounts to a PCH but no South Display).
>> > 	 */
>> 
>> The key is in this comment; "In all current cases", where "current" is 3½ years
>> ago. IIRC this was written for some Xeons which did have a PCH but no
>> display. PCH_NOP is a kind of hack for those. Nowadays you don't always
>> have a PCH on gen 5+ (VLV, CHV, BXT, ...). You might have a PCH but only
>> need the North Display for some outputs. And I guess you might still have a
>> PCH but no display at all.
>> 
>> I'm just saying, we should not overload this hack to, say, cover platforms that
>> don't even have a PCH, or platforms that have a PCH but a functioning North
>> Display.
>> 
>> BR,
>> Jani.
>> 
> I understand your point now. Thank you for explaining this. I'll update the patch and
> Use num_pipes for checking whether display engine exists.

Ville, how about adding something like:

#define HAS_DISPLAY(dev_priv) (INTEL_INFO(dev_priv)->num_pipes == 0)

and possibly

#define HAS_SOUTH_DISPLAY(dev_priv) HAS_PCH_NOP(dev_priv)

So we could clarify the code, and abstract the rules, so it'll perhaps
be easier to change them later?

Though I fear we may end up needing to add a finer granularity depending
on which parts we do need to and must not touch, and that might not
always map to low granularity north/south display. *shrug*

BR,
Jani.


> Thanks,
> Elaine
>> 
>> > 	if (INTEL_INFO(dev_priv)->num_pipes == 0) {
>> > 		dev_priv->pch_type = PCH_NOP;
>> > 		return;
>> > 	}
>> >
>> > Thanks,
>> > Elaine
>> >>
>> >>
>> >> >
>> >> >>  	GEN5_IRQ_RESET(GEN8_PCU_);
>> >> >>
>> >> >>  	if (HAS_PCH_SPLIT(dev_priv))
>> >> >> @@ -3414,7 +3416,9 @@ static int gen8_irq_postinstall(struct
>> >> >> drm_device
>> >> *dev)
>> >> >>  		ibx_irq_pre_postinstall(dev);
>> >> >>
>> >> >>  	gen8_gt_irq_postinstall(dev_priv);
>> >> >> -	gen8_de_irq_postinstall(dev_priv);
>> >> >> +
>> >> >> +	if (!HAS_PCH_NOP(dev_priv))
>> >> >> +		gen8_de_irq_postinstall(dev_priv);
>> >> >>
>> >> >>  	if (HAS_PCH_SPLIT(dev_priv))
>> >> >>  		ibx_irq_postinstall(dev);
>> >> >> --
>> >> >> 1.9.1
>> >>
>> >> --
>> >> Jani Nikula, Intel Open Source Technology Center
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> --
>> Jani Nikula, Intel Open Source Technology Center

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

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[David Weinehall]

On Thu, Dec 22, 2016 at 10:52:29AM +0200, Jani Nikula wrote:
> On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
> >> 
> >> On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
> >> > Hi Jani, Ville,
> >> >
> >> > Any comment about the "PCH_NOP" vs "num_pipes == 0"?
> >> >
> >> > Thanks,
> >> > Elaine
> >> >> On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> >> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
> >> >> >> From: Elaine Wang <elaine.wang@intel.com>
> >> >> >>
> >> >> >> Some platforms don't have display. To avoid accessing the
> >> >> >> non-existent registers, check HAS_PCH_NOP before invoking display
> >> >> >> IRQ install or reset function.
> >> >> >>
> >> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> >> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> >> >> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
> >> >> >> ---
> >> >> >>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
> >> >> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >> >> >>
> >> >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> >> >> >> b/drivers/gpu/drm/i915/i915_irq.c index 0b119b9..369a038 100644
> >> >> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> >> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> >> >> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct
> >> >> >> drm_device
> >> >> *dev)
> >> >> >>
> >> >> POWER_DOMAIN_PIPE(pipe)))
> >> >> >>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
> >> >> >>
> >> >> >> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> >> >> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> >> >> +	if (!HAS_PCH_NOP(dev_priv)) {
> >> >> >> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> >> >> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> >> >> +	}
> >> >> >
> >> >> > Hmm. These are north side registers so looking at PCH_NOP feels
> >> >> > questionable.
> >> >>
> >> >> Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.
> >> >>
> >> >> Jani.
> >> >
> >> > I thought HAS_PCH_NOP had same meaning as num_pipes == 0 because I
> >> saw
> >> > following code in i915_drv.c. Is there any exception?
> >> >
> >> > https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_
> >> > drv.c?h=drm-intel-nightly#n145 static void intel_detect_pch(struct
> >> > drm_i915_private *dev_priv) {
> >> > 	struct pci_dev *pch = NULL;
> >> >
> >> > 	/* In all current cases, num_pipes is equivalent to the PCH_NOP
> >> setting
> >> > 	 * (which really amounts to a PCH but no South Display).
> >> > 	 */
> >> 
> >> The key is in this comment; "In all current cases", where "current" is 3½ years
> >> ago. IIRC this was written for some Xeons which did have a PCH but no
> >> display. PCH_NOP is a kind of hack for those. Nowadays you don't always
> >> have a PCH on gen 5+ (VLV, CHV, BXT, ...). You might have a PCH but only
> >> need the North Display for some outputs. And I guess you might still have a
> >> PCH but no display at all.
> >> 
> >> I'm just saying, we should not overload this hack to, say, cover platforms that
> >> don't even have a PCH, or platforms that have a PCH but a functioning North
> >> Display.
> >> 
> >> BR,
> >> Jani.
> >> 
> > I understand your point now. Thank you for explaining this. I'll update the patch and
> > Use num_pipes for checking whether display engine exists.
> 
> Ville, how about adding something like:
> 
> #define HAS_DISPLAY(dev_priv) (INTEL_INFO(dev_priv)->num_pipes == 0)
> 
> and possibly
> 
> #define HAS_SOUTH_DISPLAY(dev_priv) HAS_PCH_NOP(dev_priv)
> 
> So we could clarify the code, and abstract the rules, so it'll perhaps
> be easier to change them later?
> 
> Though I fear we may end up needing to add a finer granularity depending
> on which parts we do need to and must not touch, and that might not
> always map to low granularity north/south display. *shrug*

Even if we might end up making things more granular later on,
I'm a big fan of self-documenting code. HAS_DISPLAY() clearly
explains what we're testing for, as does HAS_SOUTH_DISPLAY().


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Daniel Vetter]

On Thu, Dec 22, 2016 at 01:18:16PM +0200, David Weinehall wrote:
> On Thu, Dec 22, 2016 at 10:52:29AM +0200, Jani Nikula wrote:
> > On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
> > >> 
> > >> On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
> > >> > Hi Jani, Ville,
> > >> >
> > >> > Any comment about the "PCH_NOP" vs "num_pipes == 0"?
> > >> >
> > >> > Thanks,
> > >> > Elaine
> > >> >> On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > >> >> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
> > >> >> >> From: Elaine Wang <elaine.wang@intel.com>
> > >> >> >>
> > >> >> >> Some platforms don't have display. To avoid accessing the
> > >> >> >> non-existent registers, check HAS_PCH_NOP before invoking display
> > >> >> >> IRQ install or reset function.
> > >> >> >>
> > >> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > >> >> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > >> >> >> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
> > >> >> >> ---
> > >> >> >>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
> > >> >> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> > >> >> >>
> > >> >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > >> >> >> b/drivers/gpu/drm/i915/i915_irq.c index 0b119b9..369a038 100644
> > >> >> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> > >> >> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> > >> >> >> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct
> > >> >> >> drm_device
> > >> >> *dev)
> > >> >> >>
> > >> >> POWER_DOMAIN_PIPE(pipe)))
> > >> >> >>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
> > >> >> >>
> > >> >> >> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
> > >> >> >> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> > >> >> >> +	if (!HAS_PCH_NOP(dev_priv)) {
> > >> >> >> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
> > >> >> >> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
> > >> >> >> +	}
> > >> >> >
> > >> >> > Hmm. These are north side registers so looking at PCH_NOP feels
> > >> >> > questionable.
> > >> >>
> > >> >> Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.
> > >> >>
> > >> >> Jani.
> > >> >
> > >> > I thought HAS_PCH_NOP had same meaning as num_pipes == 0 because I
> > >> saw
> > >> > following code in i915_drv.c. Is there any exception?
> > >> >
> > >> > https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_
> > >> > drv.c?h=drm-intel-nightly#n145 static void intel_detect_pch(struct
> > >> > drm_i915_private *dev_priv) {
> > >> > 	struct pci_dev *pch = NULL;
> > >> >
> > >> > 	/* In all current cases, num_pipes is equivalent to the PCH_NOP
> > >> setting
> > >> > 	 * (which really amounts to a PCH but no South Display).
> > >> > 	 */
> > >> 
> > >> The key is in this comment; "In all current cases", where "current" is 3½ years
> > >> ago. IIRC this was written for some Xeons which did have a PCH but no
> > >> display. PCH_NOP is a kind of hack for those. Nowadays you don't always
> > >> have a PCH on gen 5+ (VLV, CHV, BXT, ...). You might have a PCH but only
> > >> need the North Display for some outputs. And I guess you might still have a
> > >> PCH but no display at all.
> > >> 
> > >> I'm just saying, we should not overload this hack to, say, cover platforms that
> > >> don't even have a PCH, or platforms that have a PCH but a functioning North
> > >> Display.
> > >> 
> > >> BR,
> > >> Jani.
> > >> 
> > > I understand your point now. Thank you for explaining this. I'll update the patch and
> > > Use num_pipes for checking whether display engine exists.
> > 
> > Ville, how about adding something like:
> > 
> > #define HAS_DISPLAY(dev_priv) (INTEL_INFO(dev_priv)->num_pipes == 0)
> > 
> > and possibly
> > 
> > #define HAS_SOUTH_DISPLAY(dev_priv) HAS_PCH_NOP(dev_priv)
> > 
> > So we could clarify the code, and abstract the rules, so it'll perhaps
> > be easier to change them later?
> > 
> > Though I fear we may end up needing to add a finer granularity depending
> > on which parts we do need to and must not touch, and that might not
> > always map to low granularity north/south display. *shrug*
> 
> Even if we might end up making things more granular later on,
> I'm a big fan of self-documenting code. HAS_DISPLAY() clearly
> explains what we're testing for, as does HAS_SOUTH_DISPLAY().

+1 on HAS_DISPLAY.

Re PCH_NOP vs. PCH_NONE:
- PCH_NONE is for platforms where there's really no PCH anywhere.
- PCH_NOP is for platforms that in general have it, but don't touch it
  it's kinda disabled. This is somewhat relevant to make sure all the
  HAS_PCH_SPLIT checks (of which not all are exclusively in display-only
  code) still work correctly for those platforms.

Given that I'm not entirely sure what you're aiming for with
HAS_SOUTH_DISPLAY ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Jani Nikula]

On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Dec 22, 2016 at 01:18:16PM +0200, David Weinehall wrote:
>> On Thu, Dec 22, 2016 at 10:52:29AM +0200, Jani Nikula wrote:
>> > On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
>> > >> 
>> > >> On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
>> > >> > Hi Jani, Ville,
>> > >> >
>> > >> > Any comment about the "PCH_NOP" vs "num_pipes == 0"?
>> > >> >
>> > >> > Thanks,
>> > >> > Elaine
>> > >> >> On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > >> >> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
>> > >> >> >> From: Elaine Wang <elaine.wang@intel.com>
>> > >> >> >>
>> > >> >> >> Some platforms don't have display. To avoid accessing the
>> > >> >> >> non-existent registers, check HAS_PCH_NOP before invoking display
>> > >> >> >> IRQ install or reset function.
>> > >> >> >>
>> > >> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> > >> >> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> > >> >> >> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
>> > >> >> >> ---
>> > >> >> >>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
>> > >> >> >>  1 file changed, 7 insertions(+), 3 deletions(-)
>> > >> >> >>
>> > >> >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> > >> >> >> b/drivers/gpu/drm/i915/i915_irq.c index 0b119b9..369a038 100644
>> > >> >> >> --- a/drivers/gpu/drm/i915/i915_irq.c
>> > >> >> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > >> >> >> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct
>> > >> >> >> drm_device
>> > >> >> *dev)
>> > >> >> >>
>> > >> >> POWER_DOMAIN_PIPE(pipe)))
>> > >> >> >>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>> > >> >> >>
>> > >> >> >> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
>> > >> >> >> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> > >> >> >> +	if (!HAS_PCH_NOP(dev_priv)) {
>> > >> >> >> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
>> > >> >> >> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> > >> >> >> +	}
>> > >> >> >
>> > >> >> > Hmm. These are north side registers so looking at PCH_NOP feels
>> > >> >> > questionable.
>> > >> >>
>> > >> >> Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.
>> > >> >>
>> > >> >> Jani.
>> > >> >
>> > >> > I thought HAS_PCH_NOP had same meaning as num_pipes == 0 because I
>> > >> saw
>> > >> > following code in i915_drv.c. Is there any exception?
>> > >> >
>> > >> > https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_
>> > >> > drv.c?h=drm-intel-nightly#n145 static void intel_detect_pch(struct
>> > >> > drm_i915_private *dev_priv) {
>> > >> > 	struct pci_dev *pch = NULL;
>> > >> >
>> > >> > 	/* In all current cases, num_pipes is equivalent to the PCH_NOP
>> > >> setting
>> > >> > 	 * (which really amounts to a PCH but no South Display).
>> > >> > 	 */
>> > >> 
>> > >> The key is in this comment; "In all current cases", where "current" is 3½ years
>> > >> ago. IIRC this was written for some Xeons which did have a PCH but no
>> > >> display. PCH_NOP is a kind of hack for those. Nowadays you don't always
>> > >> have a PCH on gen 5+ (VLV, CHV, BXT, ...). You might have a PCH but only
>> > >> need the North Display for some outputs. And I guess you might still have a
>> > >> PCH but no display at all.
>> > >> 
>> > >> I'm just saying, we should not overload this hack to, say, cover platforms that
>> > >> don't even have a PCH, or platforms that have a PCH but a functioning North
>> > >> Display.
>> > >> 
>> > >> BR,
>> > >> Jani.
>> > >> 
>> > > I understand your point now. Thank you for explaining this. I'll update the patch and
>> > > Use num_pipes for checking whether display engine exists.
>> > 
>> > Ville, how about adding something like:
>> > 
>> > #define HAS_DISPLAY(dev_priv) (INTEL_INFO(dev_priv)->num_pipes == 0)
>> > 
>> > and possibly
>> > 
>> > #define HAS_SOUTH_DISPLAY(dev_priv) HAS_PCH_NOP(dev_priv)
>> > 
>> > So we could clarify the code, and abstract the rules, so it'll perhaps
>> > be easier to change them later?
>> > 
>> > Though I fear we may end up needing to add a finer granularity depending
>> > on which parts we do need to and must not touch, and that might not
>> > always map to low granularity north/south display. *shrug*
>> 
>> Even if we might end up making things more granular later on,
>> I'm a big fan of self-documenting code. HAS_DISPLAY() clearly
>> explains what we're testing for, as does HAS_SOUTH_DISPLAY().
>
> +1 on HAS_DISPLAY.
>
> Re PCH_NOP vs. PCH_NONE:
> - PCH_NONE is for platforms where there's really no PCH anywhere.
> - PCH_NOP is for platforms that in general have it, but don't touch it
>   it's kinda disabled. This is somewhat relevant to make sure all the
>   HAS_PCH_SPLIT checks (of which not all are exclusively in display-only
>   code) still work correctly for those platforms.
>
> Given that I'm not entirely sure what you're aiming for with
> HAS_SOUTH_DISPLAY ...

Potentially a more sensible sounding check than HAS_PCH_NOP.

BR,
Jani.

> -Daniel

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

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Daniel Vetter]

On Tue, Dec 27, 2016 at 04:46:40PM +0200, Jani Nikula wrote:
> On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Dec 22, 2016 at 01:18:16PM +0200, David Weinehall wrote:
> >> On Thu, Dec 22, 2016 at 10:52:29AM +0200, Jani Nikula wrote:
> >> > On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
> >> > >> 
> >> > >> On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
> >> > >> > Hi Jani, Ville,
> >> > >> >
> >> > >> > Any comment about the "PCH_NOP" vs "num_pipes == 0"?
> >> > >> >
> >> > >> > Thanks,
> >> > >> > Elaine
> >> > >> >> On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> > >> >> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
> >> > >> >> >> From: Elaine Wang <elaine.wang@intel.com>
> >> > >> >> >>
> >> > >> >> >> Some platforms don't have display. To avoid accessing the
> >> > >> >> >> non-existent registers, check HAS_PCH_NOP before invoking display
> >> > >> >> >> IRQ install or reset function.
> >> > >> >> >>
> >> > >> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> > >> >> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> > >> >> >> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
> >> > >> >> >> ---
> >> > >> >> >>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
> >> > >> >> >>  1 file changed, 7 insertions(+), 3 deletions(-)
> >> > >> >> >>
> >> > >> >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> >> > >> >> >> b/drivers/gpu/drm/i915/i915_irq.c index 0b119b9..369a038 100644
> >> > >> >> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> > >> >> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> > >> >> >> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct
> >> > >> >> >> drm_device
> >> > >> >> *dev)
> >> > >> >> >>
> >> > >> >> POWER_DOMAIN_PIPE(pipe)))
> >> > >> >> >>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
> >> > >> >> >>
> >> > >> >> >> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> > >> >> >> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> > >> >> >> +	if (!HAS_PCH_NOP(dev_priv)) {
> >> > >> >> >> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
> >> > >> >> >> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
> >> > >> >> >> +	}
> >> > >> >> >
> >> > >> >> > Hmm. These are north side registers so looking at PCH_NOP feels
> >> > >> >> > questionable.
> >> > >> >>
> >> > >> >> Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.
> >> > >> >>
> >> > >> >> Jani.
> >> > >> >
> >> > >> > I thought HAS_PCH_NOP had same meaning as num_pipes == 0 because I
> >> > >> saw
> >> > >> > following code in i915_drv.c. Is there any exception?
> >> > >> >
> >> > >> > https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_
> >> > >> > drv.c?h=drm-intel-nightly#n145 static void intel_detect_pch(struct
> >> > >> > drm_i915_private *dev_priv) {
> >> > >> > 	struct pci_dev *pch = NULL;
> >> > >> >
> >> > >> > 	/* In all current cases, num_pipes is equivalent to the PCH_NOP
> >> > >> setting
> >> > >> > 	 * (which really amounts to a PCH but no South Display).
> >> > >> > 	 */
> >> > >> 
> >> > >> The key is in this comment; "In all current cases", where "current" is 3½ years
> >> > >> ago. IIRC this was written for some Xeons which did have a PCH but no
> >> > >> display. PCH_NOP is a kind of hack for those. Nowadays you don't always
> >> > >> have a PCH on gen 5+ (VLV, CHV, BXT, ...). You might have a PCH but only
> >> > >> need the North Display for some outputs. And I guess you might still have a
> >> > >> PCH but no display at all.
> >> > >> 
> >> > >> I'm just saying, we should not overload this hack to, say, cover platforms that
> >> > >> don't even have a PCH, or platforms that have a PCH but a functioning North
> >> > >> Display.
> >> > >> 
> >> > >> BR,
> >> > >> Jani.
> >> > >> 
> >> > > I understand your point now. Thank you for explaining this. I'll update the patch and
> >> > > Use num_pipes for checking whether display engine exists.
> >> > 
> >> > Ville, how about adding something like:
> >> > 
> >> > #define HAS_DISPLAY(dev_priv) (INTEL_INFO(dev_priv)->num_pipes == 0)
> >> > 
> >> > and possibly
> >> > 
> >> > #define HAS_SOUTH_DISPLAY(dev_priv) HAS_PCH_NOP(dev_priv)
> >> > 
> >> > So we could clarify the code, and abstract the rules, so it'll perhaps
> >> > be easier to change them later?
> >> > 
> >> > Though I fear we may end up needing to add a finer granularity depending
> >> > on which parts we do need to and must not touch, and that might not
> >> > always map to low granularity north/south display. *shrug*
> >> 
> >> Even if we might end up making things more granular later on,
> >> I'm a big fan of self-documenting code. HAS_DISPLAY() clearly
> >> explains what we're testing for, as does HAS_SOUTH_DISPLAY().
> >
> > +1 on HAS_DISPLAY.
> >
> > Re PCH_NOP vs. PCH_NONE:
> > - PCH_NONE is for platforms where there's really no PCH anywhere.
> > - PCH_NOP is for platforms that in general have it, but don't touch it
> >   it's kinda disabled. This is somewhat relevant to make sure all the
> >   HAS_PCH_SPLIT checks (of which not all are exclusively in display-only
> >   code) still work correctly for those platforms.
> >
> > Given that I'm not entirely sure what you're aiming for with
> > HAS_SOUTH_DISPLAY ...
> 
> Potentially a more sensible sounding check than HAS_PCH_NOP.

Hm, isn't that covered with HAS_PCH_SPLIT already?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ

[Jani Nikula]

On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Dec 27, 2016 at 04:46:40PM +0200, Jani Nikula wrote:
>> On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Thu, Dec 22, 2016 at 01:18:16PM +0200, David Weinehall wrote:
>> >> On Thu, Dec 22, 2016 at 10:52:29AM +0200, Jani Nikula wrote:
>> >> > On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
>> >> > >> 
>> >> > >> On Thu, 22 Dec 2016, "Wang, Elaine" <elaine.wang@intel.com> wrote:
>> >> > >> > Hi Jani, Ville,
>> >> > >> >
>> >> > >> > Any comment about the "PCH_NOP" vs "num_pipes == 0"?
>> >> > >> >
>> >> > >> > Thanks,
>> >> > >> > Elaine
>> >> > >> >> On Thu, 15 Dec 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> >> > >> >> > On Mon, Dec 12, 2016 at 02:57:44PM +0800, Wang Elaine wrote:
>> >> > >> >> >> From: Elaine Wang <elaine.wang@intel.com>
>> >> > >> >> >>
>> >> > >> >> >> Some platforms don't have display. To avoid accessing the
>> >> > >> >> >> non-existent registers, check HAS_PCH_NOP before invoking display
>> >> > >> >> >> IRQ install or reset function.
>> >> > >> >> >>
>> >> > >> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> >> > >> >> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> >> > >> >> >> Signed-off-by: Elaine Wang <elaine.wang@intel.com>
>> >> > >> >> >> ---
>> >> > >> >> >>  drivers/gpu/drm/i915/i915_irq.c | 10 +++++++---
>> >> > >> >> >>  1 file changed, 7 insertions(+), 3 deletions(-)
>> >> > >> >> >>
>> >> > >> >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> >> > >> >> >> b/drivers/gpu/drm/i915/i915_irq.c index 0b119b9..369a038 100644
>> >> > >> >> >> --- a/drivers/gpu/drm/i915/i915_irq.c
>> >> > >> >> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> >> > >> >> >> @@ -2990,8 +2990,10 @@ static void gen8_irq_reset(struct
>> >> > >> >> >> drm_device
>> >> > >> >> *dev)
>> >> > >> >> >>
>> >> > >> >> POWER_DOMAIN_PIPE(pipe)))
>> >> > >> >> >>  			GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>> >> > >> >> >>
>> >> > >> >> >> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
>> >> > >> >> >> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> >> > >> >> >> +	if (!HAS_PCH_NOP(dev_priv)) {
>> >> > >> >> >> +		GEN5_IRQ_RESET(GEN8_DE_PORT_);
>> >> > >> >> >> +		GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> >> > >> >> >> +	}
>> >> > >> >> >
>> >> > >> >> > Hmm. These are north side registers so looking at PCH_NOP feels
>> >> > >> >> > questionable.
>> >> > >> >>
>> >> > >> >> Indeed, num_pipes == 0 isn't exactly the same thing as HAS_PCH_NOP.
>> >> > >> >>
>> >> > >> >> Jani.
>> >> > >> >
>> >> > >> > I thought HAS_PCH_NOP had same meaning as num_pipes == 0 because I
>> >> > >> saw
>> >> > >> > following code in i915_drv.c. Is there any exception?
>> >> > >> >
>> >> > >> > https://cgit.freedesktop.org/drm-intel/tree/drivers/gpu/drm/i915/i915_
>> >> > >> > drv.c?h=drm-intel-nightly#n145 static void intel_detect_pch(struct
>> >> > >> > drm_i915_private *dev_priv) {
>> >> > >> > 	struct pci_dev *pch = NULL;
>> >> > >> >
>> >> > >> > 	/* In all current cases, num_pipes is equivalent to the PCH_NOP
>> >> > >> setting
>> >> > >> > 	 * (which really amounts to a PCH but no South Display).
>> >> > >> > 	 */
>> >> > >> 
>> >> > >> The key is in this comment; "In all current cases", where "current" is 3½ years
>> >> > >> ago. IIRC this was written for some Xeons which did have a PCH but no
>> >> > >> display. PCH_NOP is a kind of hack for those. Nowadays you don't always
>> >> > >> have a PCH on gen 5+ (VLV, CHV, BXT, ...). You might have a PCH but only
>> >> > >> need the North Display for some outputs. And I guess you might still have a
>> >> > >> PCH but no display at all.
>> >> > >> 
>> >> > >> I'm just saying, we should not overload this hack to, say, cover platforms that
>> >> > >> don't even have a PCH, or platforms that have a PCH but a functioning North
>> >> > >> Display.
>> >> > >> 
>> >> > >> BR,
>> >> > >> Jani.
>> >> > >> 
>> >> > > I understand your point now. Thank you for explaining this. I'll update the patch and
>> >> > > Use num_pipes for checking whether display engine exists.
>> >> > 
>> >> > Ville, how about adding something like:
>> >> > 
>> >> > #define HAS_DISPLAY(dev_priv) (INTEL_INFO(dev_priv)->num_pipes == 0)
>> >> > 
>> >> > and possibly
>> >> > 
>> >> > #define HAS_SOUTH_DISPLAY(dev_priv) HAS_PCH_NOP(dev_priv)
>> >> > 
>> >> > So we could clarify the code, and abstract the rules, so it'll perhaps
>> >> > be easier to change them later?
>> >> > 
>> >> > Though I fear we may end up needing to add a finer granularity depending
>> >> > on which parts we do need to and must not touch, and that might not
>> >> > always map to low granularity north/south display. *shrug*
>> >> 
>> >> Even if we might end up making things more granular later on,
>> >> I'm a big fan of self-documenting code. HAS_DISPLAY() clearly
>> >> explains what we're testing for, as does HAS_SOUTH_DISPLAY().
>> >
>> > +1 on HAS_DISPLAY.
>> >
>> > Re PCH_NOP vs. PCH_NONE:
>> > - PCH_NONE is for platforms where there's really no PCH anywhere.
>> > - PCH_NOP is for platforms that in general have it, but don't touch it
>> >   it's kinda disabled. This is somewhat relevant to make sure all the
>> >   HAS_PCH_SPLIT checks (of which not all are exclusively in display-only
>> >   code) still work correctly for those platforms.
>> >
>> > Given that I'm not entirely sure what you're aiming for with
>> > HAS_SOUTH_DISPLAY ...
>> 
>> Potentially a more sensible sounding check than HAS_PCH_NOP.
>
> Hm, isn't that covered with HAS_PCH_SPLIT already?

It is, but these are not the same thing, obviously:

#define HAS_PCH_NOP(dev_priv) (INTEL_PCH_TYPE(dev_priv) == PCH_NOP)
#define HAS_PCH_SPLIT(dev_priv) (INTEL_PCH_TYPE(dev_priv) != PCH_NONE)

It's just that checking for HAS_PCH_NOP does not adequately describe
what is being checked, which is clear from the discussion around
PCH_NOP.

BR,
Jani.



> -Daniel

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