Re: [Intel-gfx] [PATCH] Revert "drm/i915: re-order if/else ladder for hpd_irq_setup"
On Mon, Nov 30, 2020 at 09:46:18PM -0800, Lucas De Marchi wrote: > On Mon, Nov 30, 2020 at 07:46:39PM +0200, Ville Syrjälä wrote: > >On Mon, Nov 30, 2020 at 09:31:04AM -0800, Lucas De Marchi wrote: > >> On Mon, Nov 30, 2020 at 04:19:54PM +0200, Ville Syrjälä wrote: > >> >On Fri, Nov 27, 2020 at 08:52:29PM -0800, Lucas De Marchi wrote: > >> >> On Fri, Nov 27, 2020 at 02:57:48PM +, Chris Wilson wrote: > >> >> >We now use ilk_hpd_irq_setup for all GMCH platforms that do not have > >> >> >hotplug. These are early gen3 and gen2 devices that now explode on boot > >> >> >as they try to access non-existent registers. > >> >> > >> >> humn... true, my bad. But I don't think a revert is the right fix. It > >> >> would be much better if we would not be setting up the hpd setup > >> >> function at all for platforms that do not have hotplug. I think a > >> >> separate early check for I915_HAS_HOTPLUG() would be deserved. > >> > > >> >I think it generally leads to much less convoluted logic when we keep > >> >gmch vs. rest separate. So I'm confused as to what we're even trying > >> >to achieve here? > >> > >> 1) Stop trying to setup hotplug in a platform that doesn't have hotplug > >> was the main focus. Later it would be better to move some of these > >> hotplug to display/ as they are clearly display related and account for > >> a great portion of i915_irq.c. > >> > >> I left the I915_HAS_HOTPLUG() in the middle by > >> mistake, it should had been an earlier call. > >> > >> 2) semi-related is the move of GMCH to the middle and I guess this is > >> what you're complaining here. I find it's cumbersome to have it > >> separate as we go and extend these checks for newer platforms. Almost > >> everywhere we settled on having last platform first in the if/else > >> ladders - this makes it much more clear on how/where to add a new > >> platform. > > > >You never touch the gmch path for new platforms. What could be more > >clear than that? > > the second level branch mixing the code path for new and old platform > instead of following the convention we settled on. The convention I care about most is "clear code" which in many cases is best achieved with gmch vs. not split. > But I'm ok with > moving it back as a HAS_* check in the middle of GEN_* check is proving > controversial. > > Lucas De Marchi > > > > >-- > >Ville Syrjälä > >Intel -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "drm/i915: re-order if/else ladder for hpd_irq_setup"
On Mon, Nov 30, 2020 at 07:46:39PM +0200, Ville Syrjälä wrote: On Mon, Nov 30, 2020 at 09:31:04AM -0800, Lucas De Marchi wrote: On Mon, Nov 30, 2020 at 04:19:54PM +0200, Ville Syrjälä wrote: >On Fri, Nov 27, 2020 at 08:52:29PM -0800, Lucas De Marchi wrote: >> On Fri, Nov 27, 2020 at 02:57:48PM +, Chris Wilson wrote: >> >We now use ilk_hpd_irq_setup for all GMCH platforms that do not have >> >hotplug. These are early gen3 and gen2 devices that now explode on boot >> >as they try to access non-existent registers. >> >> humn... true, my bad. But I don't think a revert is the right fix. It >> would be much better if we would not be setting up the hpd setup >> function at all for platforms that do not have hotplug. I think a >> separate early check for I915_HAS_HOTPLUG() would be deserved. > >I think it generally leads to much less convoluted logic when we keep >gmch vs. rest separate. So I'm confused as to what we're even trying >to achieve here? 1) Stop trying to setup hotplug in a platform that doesn't have hotplug was the main focus. Later it would be better to move some of these hotplug to display/ as they are clearly display related and account for a great portion of i915_irq.c. I left the I915_HAS_HOTPLUG() in the middle by mistake, it should had been an earlier call. 2) semi-related is the move of GMCH to the middle and I guess this is what you're complaining here. I find it's cumbersome to have it separate as we go and extend these checks for newer platforms. Almost everywhere we settled on having last platform first in the if/else ladders - this makes it much more clear on how/where to add a new platform. You never touch the gmch path for new platforms. What could be more clear than that? the second level branch mixing the code path for new and old platform instead of following the convention we settled on. But I'm ok with moving it back as a HAS_* check in the middle of GEN_* check is proving controversial. Lucas De Marchi -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "drm/i915: re-order if/else ladder for hpd_irq_setup"
On Mon, Nov 30, 2020 at 09:31:04AM -0800, Lucas De Marchi wrote: > On Mon, Nov 30, 2020 at 04:19:54PM +0200, Ville Syrjälä wrote: > >On Fri, Nov 27, 2020 at 08:52:29PM -0800, Lucas De Marchi wrote: > >> On Fri, Nov 27, 2020 at 02:57:48PM +, Chris Wilson wrote: > >> >We now use ilk_hpd_irq_setup for all GMCH platforms that do not have > >> >hotplug. These are early gen3 and gen2 devices that now explode on boot > >> >as they try to access non-existent registers. > >> > >> humn... true, my bad. But I don't think a revert is the right fix. It > >> would be much better if we would not be setting up the hpd setup > >> function at all for platforms that do not have hotplug. I think a > >> separate early check for I915_HAS_HOTPLUG() would be deserved. > > > >I think it generally leads to much less convoluted logic when we keep > >gmch vs. rest separate. So I'm confused as to what we're even trying > >to achieve here? > > 1) Stop trying to setup hotplug in a platform that doesn't have hotplug > was the main focus. Later it would be better to move some of these > hotplug to display/ as they are clearly display related and account for > a great portion of i915_irq.c. > > I left the I915_HAS_HOTPLUG() in the middle by > mistake, it should had been an earlier call. > > 2) semi-related is the move of GMCH to the middle and I guess this is > what you're complaining here. I find it's cumbersome to have it > separate as we go and extend these checks for newer platforms. Almost > everywhere we settled on having last platform first in the if/else > ladders - this makes it much more clear on how/where to add a new > platform. You never touch the gmch path for new platforms. What could be more clear than that? -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "drm/i915: re-order if/else ladder for hpd_irq_setup"
On Mon, Nov 30, 2020 at 04:19:54PM +0200, Ville Syrjälä wrote:
On Fri, Nov 27, 2020 at 08:52:29PM -0800, Lucas De Marchi wrote:
On Fri, Nov 27, 2020 at 02:57:48PM +, Chris Wilson wrote:
>We now use ilk_hpd_irq_setup for all GMCH platforms that do not have
>hotplug. These are early gen3 and gen2 devices that now explode on boot
>as they try to access non-existent registers.
humn... true, my bad. But I don't think a revert is the right fix. It
would be much better if we would not be setting up the hpd setup
function at all for platforms that do not have hotplug. I think a
separate early check for I915_HAS_HOTPLUG() would be deserved.
I think it generally leads to much less convoluted logic when we keep
gmch vs. rest separate. So I'm confused as to what we're even trying
to achieve here?
1) Stop trying to setup hotplug in a platform that doesn't have hotplug
was the main focus. Later it would be better to move some of these
hotplug to display/ as they are clearly display related and account for
a great portion of i915_irq.c.
I left the I915_HAS_HOTPLUG() in the middle by
mistake, it should had been an earlier call.
2) semi-related is the move of GMCH to the middle and I guess this is
what you're complaining here. I find it's cumbersome to have it
separate as we go and extend these checks for newer platforms. Almost
everywhere we settled on having last platform first in the if/else
ladders - this makes it much more clear on how/where to add a new
platform.
Lucas De Marchi
Lucas De Marchi
>
>Fixes: 794d61a19090 ("drm/i915: re-order if/else ladder for hpd_irq_setup")
>Signed-off-by: Chris Wilson
>Cc: Lucas De Marchi
>Cc: José Roberto de Souza
>Cc: Jani Nikula
>---
> drivers/gpu/drm/i915/i915_irq.c | 27 +++
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>index dc6febc63f1c..c80eeac53952 100644
>--- a/drivers/gpu/drm/i915/i915_irq.c
>+++ b/drivers/gpu/drm/i915/i915_irq.c
>@@ -4242,18 +4242,21 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
> */
>dev_priv->hotplug.hpd_short_storm_enabled = !HAS_DP_MST(dev_priv);
>
>- if (HAS_PCH_DG1(dev_priv))
>- dev_priv->display.hpd_irq_setup = dg1_hpd_irq_setup;
>- else if (INTEL_GEN(dev_priv) >= 11)
>- dev_priv->display.hpd_irq_setup = gen11_hpd_irq_setup;
>- else if (IS_GEN9_LP(dev_priv))
>- dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
>- else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
>- dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
>- else if (HAS_GMCH(dev_priv) && I915_HAS_HOTPLUG(dev_priv))
>- dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
>- else
>- dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
>+ if (HAS_GMCH(dev_priv)) {
>+ if (I915_HAS_HOTPLUG(dev_priv))
>+ dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
>+ } else {
>+ if (HAS_PCH_DG1(dev_priv))
>+ dev_priv->display.hpd_irq_setup = dg1_hpd_irq_setup;
>+ else if (INTEL_GEN(dev_priv) >= 11)
>+ dev_priv->display.hpd_irq_setup = gen11_hpd_irq_setup;
>+ else if (IS_GEN9_LP(dev_priv))
>+ dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
>+ else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
>+ dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
>+ else
>+ dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
>+ }
> }
>
> /**
>--
>2.20.1
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "drm/i915: re-order if/else ladder for hpd_irq_setup"
On Fri, Nov 27, 2020 at 08:52:29PM -0800, Lucas De Marchi wrote:
> On Fri, Nov 27, 2020 at 02:57:48PM +, Chris Wilson wrote:
> >We now use ilk_hpd_irq_setup for all GMCH platforms that do not have
> >hotplug. These are early gen3 and gen2 devices that now explode on boot
> >as they try to access non-existent registers.
>
> humn... true, my bad. But I don't think a revert is the right fix. It
> would be much better if we would not be setting up the hpd setup
> function at all for platforms that do not have hotplug. I think a
> separate early check for I915_HAS_HOTPLUG() would be deserved.
I think it generally leads to much less convoluted logic when we keep
gmch vs. rest separate. So I'm confused as to what we're even trying
to achieve here?
>
> Lucas De Marchi
>
> >
> >Fixes: 794d61a19090 ("drm/i915: re-order if/else ladder for hpd_irq_setup")
> >Signed-off-by: Chris Wilson
> >Cc: Lucas De Marchi
> >Cc: José Roberto de Souza
> >Cc: Jani Nikula
>
>
> >---
> > drivers/gpu/drm/i915/i915_irq.c | 27 +++
> > 1 file changed, 15 insertions(+), 12 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_irq.c
> >b/drivers/gpu/drm/i915/i915_irq.c
> >index dc6febc63f1c..c80eeac53952 100644
> >--- a/drivers/gpu/drm/i915/i915_irq.c
> >+++ b/drivers/gpu/drm/i915/i915_irq.c
> >@@ -4242,18 +4242,21 @@ void intel_irq_init(struct drm_i915_private
> >*dev_priv)
> > */
> > dev_priv->hotplug.hpd_short_storm_enabled = !HAS_DP_MST(dev_priv);
> >
> >-if (HAS_PCH_DG1(dev_priv))
> >-dev_priv->display.hpd_irq_setup = dg1_hpd_irq_setup;
> >-else if (INTEL_GEN(dev_priv) >= 11)
> >-dev_priv->display.hpd_irq_setup = gen11_hpd_irq_setup;
> >-else if (IS_GEN9_LP(dev_priv))
> >-dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
> >-else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
> >-dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
> >-else if (HAS_GMCH(dev_priv) && I915_HAS_HOTPLUG(dev_priv))
> >-dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
> >-else
> >-dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
> >+if (HAS_GMCH(dev_priv)) {
> >+if (I915_HAS_HOTPLUG(dev_priv))
> >+dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
> >+} else {
> >+if (HAS_PCH_DG1(dev_priv))
> >+dev_priv->display.hpd_irq_setup = dg1_hpd_irq_setup;
> >+else if (INTEL_GEN(dev_priv) >= 11)
> >+dev_priv->display.hpd_irq_setup = gen11_hpd_irq_setup;
> >+else if (IS_GEN9_LP(dev_priv))
> >+dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
> >+else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
> >+dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
> >+else
> >+dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
> >+}
> > }
> >
> > /**
> >--
> >2.20.1
> >
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "drm/i915: re-order if/else ladder for hpd_irq_setup"
On Fri, Nov 27, 2020 at 02:57:48PM +, Chris Wilson wrote:
We now use ilk_hpd_irq_setup for all GMCH platforms that do not have
hotplug. These are early gen3 and gen2 devices that now explode on boot
as they try to access non-existent registers.
humn... true, my bad. But I don't think a revert is the right fix. It
would be much better if we would not be setting up the hpd setup
function at all for platforms that do not have hotplug. I think a
separate early check for I915_HAS_HOTPLUG() would be deserved.
Lucas De Marchi
Fixes: 794d61a19090 ("drm/i915: re-order if/else ladder for hpd_irq_setup")
Signed-off-by: Chris Wilson
Cc: Lucas De Marchi
Cc: José Roberto de Souza
Cc: Jani Nikula
---
drivers/gpu/drm/i915/i915_irq.c | 27 +++
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index dc6febc63f1c..c80eeac53952 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4242,18 +4242,21 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
*/
dev_priv->hotplug.hpd_short_storm_enabled = !HAS_DP_MST(dev_priv);
- if (HAS_PCH_DG1(dev_priv))
- dev_priv->display.hpd_irq_setup = dg1_hpd_irq_setup;
- else if (INTEL_GEN(dev_priv) >= 11)
- dev_priv->display.hpd_irq_setup = gen11_hpd_irq_setup;
- else if (IS_GEN9_LP(dev_priv))
- dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
- else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
- dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
- else if (HAS_GMCH(dev_priv) && I915_HAS_HOTPLUG(dev_priv))
- dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
- else
- dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
+ if (HAS_GMCH(dev_priv)) {
+ if (I915_HAS_HOTPLUG(dev_priv))
+ dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
+ } else {
+ if (HAS_PCH_DG1(dev_priv))
+ dev_priv->display.hpd_irq_setup = dg1_hpd_irq_setup;
+ else if (INTEL_GEN(dev_priv) >= 11)
+ dev_priv->display.hpd_irq_setup = gen11_hpd_irq_setup;
+ else if (IS_GEN9_LP(dev_priv))
+ dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
+ else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
+ dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
+ else
+ dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
+ }
}
/**
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "drm/i915: re-order if/else ladder for hpd_irq_setup"
On Fri, 27 Nov 2020, Chris Wilson wrote:
> We now use ilk_hpd_irq_setup for all GMCH platforms that do not have
> hotplug. These are early gen3 and gen2 devices that now explode on boot
> as they try to access non-existent registers.
>
> Fixes: 794d61a19090 ("drm/i915: re-order if/else ladder for hpd_irq_setup")
> Signed-off-by: Chris Wilson
> Cc: Lucas De Marchi
> Cc: José Roberto de Souza
> Cc: Jani Nikula
Reviewed-by: Jani Nikula
> ---
> drivers/gpu/drm/i915/i915_irq.c | 27 +++
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index dc6febc63f1c..c80eeac53952 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4242,18 +4242,21 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>*/
> dev_priv->hotplug.hpd_short_storm_enabled = !HAS_DP_MST(dev_priv);
>
> - if (HAS_PCH_DG1(dev_priv))
> - dev_priv->display.hpd_irq_setup = dg1_hpd_irq_setup;
> - else if (INTEL_GEN(dev_priv) >= 11)
> - dev_priv->display.hpd_irq_setup = gen11_hpd_irq_setup;
> - else if (IS_GEN9_LP(dev_priv))
> - dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
> - else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
> - dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
> - else if (HAS_GMCH(dev_priv) && I915_HAS_HOTPLUG(dev_priv))
> - dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
> - else
> - dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
> + if (HAS_GMCH(dev_priv)) {
> + if (I915_HAS_HOTPLUG(dev_priv))
> + dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
> + } else {
> + if (HAS_PCH_DG1(dev_priv))
> + dev_priv->display.hpd_irq_setup = dg1_hpd_irq_setup;
> + else if (INTEL_GEN(dev_priv) >= 11)
> + dev_priv->display.hpd_irq_setup = gen11_hpd_irq_setup;
> + else if (IS_GEN9_LP(dev_priv))
> + dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
> + else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
> + dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
> + else
> + dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup;
> + }
> }
>
> /**
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
