* [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
@ 2016-12-12 6:57 Wang Elaine
2016-12-12 7:52 ` ✗ Fi.CI.BAT: failure for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Wang Elaine @ 2016-12-12 6:57 UTC (permalink / raw)
To: intel-gfx; +Cc: Elaine Wang
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-12 6:57 [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ Wang Elaine
@ 2016-12-12 7:52 ` Patchwork
2016-12-15 10:18 ` Wang, Elaine
2016-12-15 12:37 ` [PATCH v3] " Joonas Lahtinen
2016-12-15 12:58 ` Ville Syrjälä
2 siblings, 1 reply; 16+ messages in thread
From: Patchwork @ 2016-12-12 7:52 UTC (permalink / raw)
To: Wang Elaine; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: ✗ Fi.CI.BAT: failure for drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-12 7:52 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2016-12-15 10:18 ` Wang, Elaine
0 siblings, 0 replies; 16+ messages in thread
From: Wang, Elaine @ 2016-12-15 10:18 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-12 6:57 [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ Wang Elaine
2016-12-12 7:52 ` ✗ Fi.CI.BAT: failure for " Patchwork
@ 2016-12-15 12:37 ` Joonas Lahtinen
2016-12-15 12:58 ` Ville Syrjälä
2 siblings, 0 replies; 16+ messages in thread
From: Joonas Lahtinen @ 2016-12-15 12:37 UTC (permalink / raw)
To: Wang Elaine, intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-12 6:57 [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ Wang Elaine
2016-12-12 7:52 ` ✗ Fi.CI.BAT: failure for " Patchwork
2016-12-15 12:37 ` [PATCH v3] " Joonas Lahtinen
@ 2016-12-15 12:58 ` Ville Syrjälä
2016-12-15 14:49 ` Jani Nikula
2 siblings, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2016-12-15 12:58 UTC (permalink / raw)
To: Wang Elaine; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-15 12:58 ` Ville Syrjälä
@ 2016-12-15 14:49 ` Jani Nikula
2016-12-16 1:40 ` Wang, Elaine
0 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2016-12-15 14:49 UTC (permalink / raw)
To: Ville Syrjälä, Wang Elaine; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-15 14:49 ` Jani Nikula
@ 2016-12-16 1:40 ` Wang, Elaine
2016-12-22 7:01 ` Wang, Elaine
0 siblings, 1 reply; 16+ messages in thread
From: Wang, Elaine @ 2016-12-16 1:40 UTC (permalink / raw)
To: Jani Nikula, Ville Syrjälä; +Cc: intel-gfx
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-16 1:40 ` Wang, Elaine
@ 2016-12-22 7:01 ` Wang, Elaine
2016-12-22 8:09 ` Jani Nikula
0 siblings, 1 reply; 16+ messages in thread
From: Wang, Elaine @ 2016-12-22 7:01 UTC (permalink / raw)
To: Jani Nikula, Ville Syrjälä; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-22 7:01 ` Wang, Elaine
@ 2016-12-22 8:09 ` Jani Nikula
2016-12-22 8:34 ` Wang, Elaine
0 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2016-12-22 8:09 UTC (permalink / raw)
To: Wang, Elaine, Ville Syrjälä; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-22 8:09 ` Jani Nikula
@ 2016-12-22 8:34 ` Wang, Elaine
2016-12-22 8:52 ` Jani Nikula
0 siblings, 1 reply; 16+ messages in thread
From: Wang, Elaine @ 2016-12-22 8:34 UTC (permalink / raw)
To: Jani Nikula, Ville Syrjälä; +Cc: intel-gfx
>
> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-22 8:34 ` Wang, Elaine
@ 2016-12-22 8:52 ` Jani Nikula
2016-12-22 11:18 ` David Weinehall
0 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2016-12-22 8:52 UTC (permalink / raw)
To: Wang, Elaine, Ville Syrjälä; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-22 8:52 ` Jani Nikula
@ 2016-12-22 11:18 ` David Weinehall
2016-12-27 14:41 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: David Weinehall @ 2016-12-22 11:18 UTC (permalink / raw)
To: Jani Nikula; +Cc: Wang, Elaine, intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-22 11:18 ` David Weinehall
@ 2016-12-27 14:41 ` Daniel Vetter
2016-12-27 14:46 ` Jani Nikula
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Vetter @ 2016-12-27 14:41 UTC (permalink / raw)
To: Jani Nikula, Wang, Elaine, Ville Syrjälä, intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-27 14:41 ` Daniel Vetter
@ 2016-12-27 14:46 ` Jani Nikula
2016-12-27 15:04 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2016-12-27 14:46 UTC (permalink / raw)
To: Daniel Vetter, Wang, Elaine, Ville Syrjälä, intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-27 14:46 ` Jani Nikula
@ 2016-12-27 15:04 ` Daniel Vetter
2016-12-27 15:49 ` Jani Nikula
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Vetter @ 2016-12-27 15:04 UTC (permalink / raw)
To: Jani Nikula; +Cc: Wang, Elaine, intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ
2016-12-27 15:04 ` Daniel Vetter
@ 2016-12-27 15:49 ` Jani Nikula
0 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2016-12-27 15:49 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Wang, Elaine, intel-gfx
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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-12-27 15:49 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-12 6:57 [PATCH v3] drm/i915: Check HAS_PCH_NOP when install or reset dispaly IRQ Wang Elaine
2016-12-12 7:52 ` ✗ Fi.CI.BAT: failure for " Patchwork
2016-12-15 10:18 ` Wang, Elaine
2016-12-15 12:37 ` [PATCH v3] " Joonas Lahtinen
2016-12-15 12:58 ` Ville Syrjälä
2016-12-15 14:49 ` Jani Nikula
2016-12-16 1:40 ` Wang, Elaine
2016-12-22 7:01 ` Wang, Elaine
2016-12-22 8:09 ` Jani Nikula
2016-12-22 8:34 ` Wang, Elaine
2016-12-22 8:52 ` Jani Nikula
2016-12-22 11:18 ` David Weinehall
2016-12-27 14:41 ` Daniel Vetter
2016-12-27 14:46 ` Jani Nikula
2016-12-27 15:04 ` Daniel Vetter
2016-12-27 15:49 ` Jani Nikula
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.