Re: [Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
On Wed, Mar 13, 2019 at 10:30:52AM -0700, Lucas De Marchi wrote:
> On Fri, Mar 08, 2019 at 01:43:00PM -0800, Rodrigo Vivi wrote:
> > In order to make it easier to bring up new platforms
> > without having to take care about all corner cases
> > that was previously taken care for previous platforms
> > we already use comparative INTEL_GEN statements.
> >
> > Let's start doing the same with PCH.
> >
> > The only caveats are:
> > - less-than comparisons need to be avoided or done with
> > attention and check > PCH_NONE as well.
> > - It is not necessarily a chronological order, but a matter
> > of south display compatibility/inheritance.
> >
> > v2: Rebased on top of Jani's clean-up which removed the
> >need for less-than comparison
> >
> > Cc: Jani Nikula
> > Cc: Ville Syrjälä
> > Cc: Lucas De Marchi
> > Signed-off-by: Rodrigo Vivi
>
>
> Reviewed-by: Lucas De Marchi
Thanks, series pushed.
>
> thanks
> Lucas De Marchi
>
>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h| 6 ++
> > drivers/gpu/drm/i915/i915_irq.c| 7 ++-
> > drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
> > drivers/gpu/drm/i915/intel_dp.c| 3 +--
> > drivers/gpu/drm/i915/intel_panel.c | 5 ++---
> > 5 files changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 8a57cdde5385..9a93accbb2e1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -523,6 +523,12 @@ struct i915_psr {
> > u16 su_x_granularity;
> > };
> >
> > +/*
> > + * Sorted by south display engine compatibility.
> > + * If the new PCH comes with a south display engine that is not
> > + * inherited from the latest item, please do not add it to the
> > + * end. Instead, add it right after its "parent" PCH.
> > + */
> > enum intel_pch {
> > PCH_NOP = -1, /* PCH without south display */
> > PCH_NONE = 0, /* No PCH present */
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 1f4e984ce42f..c823d2e76852 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2831,9 +2831,7 @@ gen8_de_irq_handler(struct drm_i915_private
> > *dev_priv, u32 master_ctl)
> >
> > if (HAS_PCH_ICP(dev_priv))
> > icp_irq_handler(dev_priv, iir);
> > - else if (HAS_PCH_SPT(dev_priv) ||
> > -HAS_PCH_KBP(dev_priv) ||
> > -HAS_PCH_CNP(dev_priv))
> > + else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
> > spt_irq_handler(dev_priv, iir);
> > else
> > cpt_irq_handler(dev_priv, iir);
> > @@ -4621,8 +4619,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
> > dev->driver->disable_vblank = gen8_disable_vblank;
> > if (IS_GEN9_LP(dev_priv))
> > dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
> > - else if (HAS_PCH_SPT(dev_priv) || HAS_PCH_KBP(dev_priv) ||
> > -HAS_PCH_CNP(dev_priv))
> > + 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;
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 7e5132772477..9d236e4ed26a 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -2723,7 +2723,7 @@ static int g4x_hrawclk(struct drm_i915_private
> > *dev_priv)
> > */
> > void intel_update_rawclk(struct drm_i915_private *dev_priv)
> > {
> > - if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv))
> > + if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP)
> > dev_priv->rawclk_freq = cnp_rawclk(dev_priv);
> > else if (HAS_PCH_SPLIT(dev_priv))
> > dev_priv->rawclk_freq = pch_rawclk(dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index f40b3342d82a..47857f96c3b1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -951,8 +951,7 @@ static void intel_pps_get_registers(struct intel_dp
> > *intel_dp,
> > regs->pp_off = PP_OFF_DELAYS(pps_idx);
> >
> > /* Cycle delay moved from PP_DIVISOR to PP_CONTROL */
> > - if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
> > - HAS_PCH_ICP(dev_priv))
> > + if (IS_GEN9_LP(dev_priv) || INTEL_PCH_TYPE(dev_priv) >= PCH_CNP)
> > regs->pp_div = INVALID_MMIO_REG;
> > else
> > regs->pp_div = PP_DIVISOR(pps_idx);
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c
> > b/drivers/gpu/drm/i915/intel_panel.c
> > index beca98d2b035..edd5540639b0 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
On Fri, Mar 08, 2019 at 01:43:00PM -0800, Rodrigo Vivi wrote:
In order to make it easier to bring up new platforms
without having to take care about all corner cases
that was previously taken care for previous platforms
we already use comparative INTEL_GEN statements.
Let's start doing the same with PCH.
The only caveats are:
- less-than comparisons need to be avoided or done with
attention and check > PCH_NONE as well.
- It is not necessarily a chronological order, but a matter
of south display compatibility/inheritance.
v2: Rebased on top of Jani's clean-up which removed the
need for less-than comparison
Cc: Jani Nikula
Cc: Ville Syrjälä
Cc: Lucas De Marchi
Signed-off-by: Rodrigo Vivi
Reviewed-by: Lucas De Marchi
thanks
Lucas De Marchi
---
drivers/gpu/drm/i915/i915_drv.h| 6 ++
drivers/gpu/drm/i915/i915_irq.c| 7 ++-
drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
drivers/gpu/drm/i915/intel_dp.c| 3 +--
drivers/gpu/drm/i915/intel_panel.c | 5 ++---
5 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8a57cdde5385..9a93accbb2e1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -523,6 +523,12 @@ struct i915_psr {
u16 su_x_granularity;
};
+/*
+ * Sorted by south display engine compatibility.
+ * If the new PCH comes with a south display engine that is not
+ * inherited from the latest item, please do not add it to the
+ * end. Instead, add it right after its "parent" PCH.
+ */
enum intel_pch {
PCH_NOP = -1, /* PCH without south display */
PCH_NONE = 0, /* No PCH present */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1f4e984ce42f..c823d2e76852 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2831,9 +2831,7 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv,
u32 master_ctl)
if (HAS_PCH_ICP(dev_priv))
icp_irq_handler(dev_priv, iir);
- else if (HAS_PCH_SPT(dev_priv) ||
-HAS_PCH_KBP(dev_priv) ||
-HAS_PCH_CNP(dev_priv))
+ else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
spt_irq_handler(dev_priv, iir);
else
cpt_irq_handler(dev_priv, iir);
@@ -4621,8 +4619,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
dev->driver->disable_vblank = gen8_disable_vblank;
if (IS_GEN9_LP(dev_priv))
dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
- else if (HAS_PCH_SPT(dev_priv) || HAS_PCH_KBP(dev_priv) ||
-HAS_PCH_CNP(dev_priv))
+ 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;
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
b/drivers/gpu/drm/i915/intel_cdclk.c
index 7e5132772477..9d236e4ed26a 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -2723,7 +2723,7 @@ static int g4x_hrawclk(struct drm_i915_private *dev_priv)
*/
void intel_update_rawclk(struct drm_i915_private *dev_priv)
{
- if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv))
+ if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP)
dev_priv->rawclk_freq = cnp_rawclk(dev_priv);
else if (HAS_PCH_SPLIT(dev_priv))
dev_priv->rawclk_freq = pch_rawclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f40b3342d82a..47857f96c3b1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -951,8 +951,7 @@ static void intel_pps_get_registers(struct intel_dp
*intel_dp,
regs->pp_off = PP_OFF_DELAYS(pps_idx);
/* Cycle delay moved from PP_DIVISOR to PP_CONTROL */
- if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
- HAS_PCH_ICP(dev_priv))
+ if (IS_GEN9_LP(dev_priv) || INTEL_PCH_TYPE(dev_priv) >= PCH_CNP)
regs->pp_div = INVALID_MMIO_REG;
else
regs->pp_div = PP_DIVISOR(pps_idx);
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index beca98d2b035..edd5540639b0 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1894,15 +1894,14 @@ intel_panel_init_backlight_funcs(struct intel_panel
*panel)
panel->backlight.set = bxt_set_backlight;
panel->backlight.get = bxt_get_backlight;
panel->backlight.hz_to_pwm = bxt_hz_to_pwm;
- } else if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv)) {
+ } else if (INTEL_PCH_TYPE(dev_priv) >=
[Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
In order to make it easier to bring up new platforms
without having to take care about all corner cases
that was previously taken care for previous platforms
we already use comparative INTEL_GEN statements.
Let's start doing the same with PCH.
The only caveats are:
- less-than comparisons need to be avoided or done with
attention and check > PCH_NONE as well.
- It is not necessarily a chronological order, but a matter
of south display compatibility/inheritance.
v2: Rebased on top of Jani's clean-up which removed the
need for less-than comparison
Cc: Jani Nikula
Cc: Ville Syrjälä
Cc: Lucas De Marchi
Signed-off-by: Rodrigo Vivi
---
drivers/gpu/drm/i915/i915_drv.h| 6 ++
drivers/gpu/drm/i915/i915_irq.c| 7 ++-
drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
drivers/gpu/drm/i915/intel_dp.c| 3 +--
drivers/gpu/drm/i915/intel_panel.c | 5 ++---
5 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8a57cdde5385..9a93accbb2e1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -523,6 +523,12 @@ struct i915_psr {
u16 su_x_granularity;
};
+/*
+ * Sorted by south display engine compatibility.
+ * If the new PCH comes with a south display engine that is not
+ * inherited from the latest item, please do not add it to the
+ * end. Instead, add it right after its "parent" PCH.
+ */
enum intel_pch {
PCH_NOP = -1, /* PCH without south display */
PCH_NONE = 0, /* No PCH present */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1f4e984ce42f..c823d2e76852 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2831,9 +2831,7 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv,
u32 master_ctl)
if (HAS_PCH_ICP(dev_priv))
icp_irq_handler(dev_priv, iir);
- else if (HAS_PCH_SPT(dev_priv) ||
-HAS_PCH_KBP(dev_priv) ||
-HAS_PCH_CNP(dev_priv))
+ else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
spt_irq_handler(dev_priv, iir);
else
cpt_irq_handler(dev_priv, iir);
@@ -4621,8 +4619,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
dev->driver->disable_vblank = gen8_disable_vblank;
if (IS_GEN9_LP(dev_priv))
dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
- else if (HAS_PCH_SPT(dev_priv) || HAS_PCH_KBP(dev_priv) ||
-HAS_PCH_CNP(dev_priv))
+ 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;
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
b/drivers/gpu/drm/i915/intel_cdclk.c
index 7e5132772477..9d236e4ed26a 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -2723,7 +2723,7 @@ static int g4x_hrawclk(struct drm_i915_private *dev_priv)
*/
void intel_update_rawclk(struct drm_i915_private *dev_priv)
{
- if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv))
+ if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP)
dev_priv->rawclk_freq = cnp_rawclk(dev_priv);
else if (HAS_PCH_SPLIT(dev_priv))
dev_priv->rawclk_freq = pch_rawclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f40b3342d82a..47857f96c3b1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -951,8 +951,7 @@ static void intel_pps_get_registers(struct intel_dp
*intel_dp,
regs->pp_off = PP_OFF_DELAYS(pps_idx);
/* Cycle delay moved from PP_DIVISOR to PP_CONTROL */
- if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
- HAS_PCH_ICP(dev_priv))
+ if (IS_GEN9_LP(dev_priv) || INTEL_PCH_TYPE(dev_priv) >= PCH_CNP)
regs->pp_div = INVALID_MMIO_REG;
else
regs->pp_div = PP_DIVISOR(pps_idx);
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index beca98d2b035..edd5540639b0 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1894,15 +1894,14 @@ intel_panel_init_backlight_funcs(struct intel_panel
*panel)
panel->backlight.set = bxt_set_backlight;
panel->backlight.get = bxt_get_backlight;
panel->backlight.hz_to_pwm = bxt_hz_to_pwm;
- } else if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv)) {
+ } else if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP) {
panel->backlight.setup = cnp_setup_backlight;
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
On Tue, Mar 05, 2019 at 09:46:29PM +0200, Jani Nikula wrote: > On Tue, 05 Mar 2019, Lucas De Marchi wrote: > > On Tue, Mar 05, 2019 at 10:38:46AM -0800, Rodrigo Vivi wrote: > >>On Tue, Mar 05, 2019 at 09:42:22AM -0800, Lucas De Marchi wrote: > >>> On Tue, Mar 05, 2019 at 07:16:47PM +0200, Jani Nikula wrote: > >>> > On Tue, 05 Mar 2019, Lucas De Marchi wrote: > >>> > > On Tue, Mar 05, 2019 at 04:10:04PM +0200, Jani Nikula wrote: > >>> > > > On Mon, 04 Mar 2019, Rodrigo Vivi wrote: > >>> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > >>> > > > > b/drivers/gpu/drm/i915/intel_dp.c > >>> > > > > index e1a051c0fbfe..acd2336bb214 100644 > >>> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > >>> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > >>> > > > > @@ -949,8 +949,8 @@ static void intel_pps_get_registers(struct > >>> > > > > intel_dp *intel_dp, > >>> > > > > regs->pp_stat = PP_STATUS(pps_idx); > >>> > > > > regs->pp_on = PP_ON_DELAYS(pps_idx); > >>> > > > > regs->pp_off = PP_OFF_DELAYS(pps_idx); > >>> > > > > - if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) && > >>> > > > > - !HAS_PCH_ICP(dev_priv)) > >>> > > > > + if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE && > >>> > > > > + INTEL_PCH_TYPE(dev_priv) < PCH_CNP) > >>> > > > > >>> > > > This is not right, starts to require PCH. > >>> > > > >>> > > But in those cases INTEL_PCH_TYPE() will return either NONE or NOP. > >>> > > >>> > Exactly. Non-PCH platforms before CNP should match, but won't. > >>> > >>> yeah, right. I misread the !IS_GEN9_LP(). > >> > >>ouch... indeed. > >>probably this explains failure on ci for bsw and byt > >> > >>options: > >> > >>1. if (!IS_GEN9_LP(dev_priv) && !(INTEL_PCH_TYPE(dev_priv) >= 10)) > > > > 10? I think you meant PCH_CNP > > > >> > >>2. if (INTEL_GEN(dev_priv) <= 8 || IS_GEN9_BC(dev_priv)) > >> > >>3. other ideas? > > > > "all PCHs before CNP, excluding GEN9_LP": > > > > if (INTEL_PCH_TYPE(dev_priv) < PCH_CNP && !IS_GEN9_LP(dev_priv)) > > See the series I mentioned upthread [1], it reverses the condition > making this easier to write anyway. Thanks a lot! I will make a v2 on top of yours and resend after it gets pushed. > > BR, > Jani. > > > [1] https://patchwork.freedesktop.org/series/57579/ > > > > > > > > Lucas De Marchi > > > >> > >>> > >>> Lucas De Marchi > >>> > >>> > > >>> > BR, > >>> > Jani. > >>> > > >>> > > >>> > -- > >>> > Jani Nikula, Intel Open Source Graphics Center > > -- > Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
On Tue, 05 Mar 2019, Lucas De Marchi wrote: > On Tue, Mar 05, 2019 at 10:38:46AM -0800, Rodrigo Vivi wrote: >>On Tue, Mar 05, 2019 at 09:42:22AM -0800, Lucas De Marchi wrote: >>> On Tue, Mar 05, 2019 at 07:16:47PM +0200, Jani Nikula wrote: >>> > On Tue, 05 Mar 2019, Lucas De Marchi wrote: >>> > > On Tue, Mar 05, 2019 at 04:10:04PM +0200, Jani Nikula wrote: >>> > > > On Mon, 04 Mar 2019, Rodrigo Vivi wrote: >>> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c >>> > > > > b/drivers/gpu/drm/i915/intel_dp.c >>> > > > > index e1a051c0fbfe..acd2336bb214 100644 >>> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c >>> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c >>> > > > > @@ -949,8 +949,8 @@ static void intel_pps_get_registers(struct >>> > > > > intel_dp *intel_dp, >>> > > > > regs->pp_stat = PP_STATUS(pps_idx); >>> > > > > regs->pp_on = PP_ON_DELAYS(pps_idx); >>> > > > > regs->pp_off = PP_OFF_DELAYS(pps_idx); >>> > > > > - if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) && >>> > > > > - !HAS_PCH_ICP(dev_priv)) >>> > > > > + if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE && >>> > > > > + INTEL_PCH_TYPE(dev_priv) < PCH_CNP) >>> > > > >>> > > > This is not right, starts to require PCH. >>> > > >>> > > But in those cases INTEL_PCH_TYPE() will return either NONE or NOP. >>> > >>> > Exactly. Non-PCH platforms before CNP should match, but won't. >>> >>> yeah, right. I misread the !IS_GEN9_LP(). >> >>ouch... indeed. >>probably this explains failure on ci for bsw and byt >> >>options: >> >>1. if (!IS_GEN9_LP(dev_priv) && !(INTEL_PCH_TYPE(dev_priv) >= 10)) > > 10? I think you meant PCH_CNP > >> >>2. if (INTEL_GEN(dev_priv) <= 8 || IS_GEN9_BC(dev_priv)) >> >>3. other ideas? > > "all PCHs before CNP, excluding GEN9_LP": > > if (INTEL_PCH_TYPE(dev_priv) < PCH_CNP && !IS_GEN9_LP(dev_priv)) See the series I mentioned upthread [1], it reverses the condition making this easier to write anyway. BR, Jani. [1] https://patchwork.freedesktop.org/series/57579/ > > > Lucas De Marchi > >> >>> >>> Lucas De Marchi >>> >>> > >>> > BR, >>> > Jani. >>> > >>> > >>> > -- >>> > Jani Nikula, Intel Open Source Graphics Center -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
On Tue, Mar 05, 2019 at 10:38:46AM -0800, Rodrigo Vivi wrote: On Tue, Mar 05, 2019 at 09:42:22AM -0800, Lucas De Marchi wrote: On Tue, Mar 05, 2019 at 07:16:47PM +0200, Jani Nikula wrote: > On Tue, 05 Mar 2019, Lucas De Marchi wrote: > > On Tue, Mar 05, 2019 at 04:10:04PM +0200, Jani Nikula wrote: > > > On Mon, 04 Mar 2019, Rodrigo Vivi wrote: > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > > index e1a051c0fbfe..acd2336bb214 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -949,8 +949,8 @@ static void intel_pps_get_registers(struct intel_dp *intel_dp, > > > > regs->pp_stat = PP_STATUS(pps_idx); > > > > regs->pp_on = PP_ON_DELAYS(pps_idx); > > > > regs->pp_off = PP_OFF_DELAYS(pps_idx); > > > > - if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) && > > > > - !HAS_PCH_ICP(dev_priv)) > > > > + if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE && > > > > + INTEL_PCH_TYPE(dev_priv) < PCH_CNP) > > > > > > This is not right, starts to require PCH. > > > > But in those cases INTEL_PCH_TYPE() will return either NONE or NOP. > > Exactly. Non-PCH platforms before CNP should match, but won't. yeah, right. I misread the !IS_GEN9_LP(). ouch... indeed. probably this explains failure on ci for bsw and byt options: 1. if (!IS_GEN9_LP(dev_priv) && !(INTEL_PCH_TYPE(dev_priv) >= 10)) 10? I think you meant PCH_CNP 2. if (INTEL_GEN(dev_priv) <= 8 || IS_GEN9_BC(dev_priv)) 3. other ideas? "all PCHs before CNP, excluding GEN9_LP": if (INTEL_PCH_TYPE(dev_priv) < PCH_CNP && !IS_GEN9_LP(dev_priv)) Lucas De Marchi Lucas De Marchi > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
On Tue, Mar 05, 2019 at 09:42:22AM -0800, Lucas De Marchi wrote: > On Tue, Mar 05, 2019 at 07:16:47PM +0200, Jani Nikula wrote: > > On Tue, 05 Mar 2019, Lucas De Marchi wrote: > > > On Tue, Mar 05, 2019 at 04:10:04PM +0200, Jani Nikula wrote: > > > > On Mon, 04 Mar 2019, Rodrigo Vivi wrote: > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > > index e1a051c0fbfe..acd2336bb214 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > > @@ -949,8 +949,8 @@ static void intel_pps_get_registers(struct > > > > > intel_dp *intel_dp, > > > > > regs->pp_stat = PP_STATUS(pps_idx); > > > > > regs->pp_on = PP_ON_DELAYS(pps_idx); > > > > > regs->pp_off = PP_OFF_DELAYS(pps_idx); > > > > > - if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) && > > > > > - !HAS_PCH_ICP(dev_priv)) > > > > > + if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE && > > > > > + INTEL_PCH_TYPE(dev_priv) < PCH_CNP) > > > > > > > > This is not right, starts to require PCH. > > > > > > But in those cases INTEL_PCH_TYPE() will return either NONE or NOP. > > > > Exactly. Non-PCH platforms before CNP should match, but won't. > > yeah, right. I misread the !IS_GEN9_LP(). ouch... indeed. probably this explains failure on ci for bsw and byt options: 1. if (!IS_GEN9_LP(dev_priv) && !(INTEL_PCH_TYPE(dev_priv) >= 10)) 2. if (INTEL_GEN(dev_priv) <= 8 || IS_GEN9_BC(dev_priv)) 3. other ideas? > > Lucas De Marchi > > > > > BR, > > Jani. > > > > > > -- > > Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
On Tue, Mar 05, 2019 at 07:16:47PM +0200, Jani Nikula wrote: On Tue, 05 Mar 2019, Lucas De Marchi wrote: On Tue, Mar 05, 2019 at 04:10:04PM +0200, Jani Nikula wrote: On Mon, 04 Mar 2019, Rodrigo Vivi wrote: diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e1a051c0fbfe..acd2336bb214 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -949,8 +949,8 @@ static void intel_pps_get_registers(struct intel_dp *intel_dp, regs->pp_stat = PP_STATUS(pps_idx); regs->pp_on = PP_ON_DELAYS(pps_idx); regs->pp_off = PP_OFF_DELAYS(pps_idx); - if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) && - !HAS_PCH_ICP(dev_priv)) + if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE && + INTEL_PCH_TYPE(dev_priv) < PCH_CNP) This is not right, starts to require PCH. But in those cases INTEL_PCH_TYPE() will return either NONE or NOP. Exactly. Non-PCH platforms before CNP should match, but won't. yeah, right. I misread the !IS_GEN9_LP(). Lucas De Marchi BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
On Tue, 05 Mar 2019, Lucas De Marchi wrote: > On Tue, Mar 05, 2019 at 04:10:04PM +0200, Jani Nikula wrote: >>On Mon, 04 Mar 2019, Rodrigo Vivi wrote: >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>> b/drivers/gpu/drm/i915/intel_dp.c >>> index e1a051c0fbfe..acd2336bb214 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -949,8 +949,8 @@ static void intel_pps_get_registers(struct intel_dp >>> *intel_dp, >>> regs->pp_stat = PP_STATUS(pps_idx); >>> regs->pp_on = PP_ON_DELAYS(pps_idx); >>> regs->pp_off = PP_OFF_DELAYS(pps_idx); >>> - if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) && >>> - !HAS_PCH_ICP(dev_priv)) >>> + if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE && >>> + INTEL_PCH_TYPE(dev_priv) < PCH_CNP) >> >>This is not right, starts to require PCH. > > But in those cases INTEL_PCH_TYPE() will return either NONE or NOP. Exactly. Non-PCH platforms before CNP should match, but won't. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
On Tue, Mar 05, 2019 at 04:10:04PM +0200, Jani Nikula wrote: On Mon, 04 Mar 2019, Rodrigo Vivi wrote: diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e1a051c0fbfe..acd2336bb214 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -949,8 +949,8 @@ static void intel_pps_get_registers(struct intel_dp *intel_dp, regs->pp_stat = PP_STATUS(pps_idx); regs->pp_on = PP_ON_DELAYS(pps_idx); regs->pp_off = PP_OFF_DELAYS(pps_idx); - if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) && - !HAS_PCH_ICP(dev_priv)) + if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE && + INTEL_PCH_TYPE(dev_priv) < PCH_CNP) This is not right, starts to require PCH. But in those cases INTEL_PCH_TYPE() will return either NONE or NOP. Lucas De Marchi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
On Mon, 04 Mar 2019, Rodrigo Vivi wrote:
> In order to make it easier to bring up new platforms
> without having to take care about all corner cases
> that was previously taken care for previous platforms
> we already use comparative INTEL_GEN statements.
>
> Let's start doing the same with PCH.
>
> The only caveats are:
> - for less-than comparisons we need to be careful
>and check PCH_NONE < pch < PCH_CNP.
> - It is not necessarily a chronological order, but a matter
>of south display compatibility/inheritance.
This scares me a bit, but I understand the reasons. Maybe we need an
IS_PCH_RANGE() macro to complement IS_GEN_RANGE(). But that can come
later as we see how this evolves.
Some notes below.
>
> Cc: Ville Syrjälä
> Cc: Lucas De Marchi
> Signed-off-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/i915_drv.h| 6 ++
> drivers/gpu/drm/i915/i915_irq.c| 7 ++-
> drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
> drivers/gpu/drm/i915/intel_dp.c| 21 +
> drivers/gpu/drm/i915/intel_panel.c | 5 ++---
> 5 files changed, 20 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e6be327ba86d..e327736c76a0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -523,6 +523,12 @@ struct i915_psr {
> u16 su_x_granularity;
> };
>
> +/*
> + * Sorted by south display engine compatibility.
> + * If the new PCH comes with a south display engine that is not
> + * inherited from the latest item, please do not add it to the
> + * end. Instead, add it right after its "parent" PCH.
> + */
> enum intel_pch {
> PCH_NOP = -1, /* PCH without south display */
> PCH_NONE = 0, /* No PCH present */
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a42eb6394b69..923135d6b781 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2833,9 +2833,7 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv,
> u32 master_ctl)
>
> if (HAS_PCH_ICP(dev_priv))
PCH_TYPE >= ICP?
> icp_irq_handler(dev_priv, iir);
> - else if (HAS_PCH_SPT(dev_priv) ||
> - HAS_PCH_KBP(dev_priv) ||
> - HAS_PCH_CNP(dev_priv))
> + else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
> spt_irq_handler(dev_priv, iir);
> else
> cpt_irq_handler(dev_priv, iir);
> @@ -4620,8 +4618,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
> dev->driver->disable_vblank = gen8_disable_vblank;
> if (IS_GEN9_LP(dev_priv))
> dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
> - else if (HAS_PCH_SPT(dev_priv) || HAS_PCH_KBP(dev_priv) ||
> - HAS_PCH_CNP(dev_priv))
> + 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;
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> b/drivers/gpu/drm/i915/intel_cdclk.c
> index 7e5132772477..9d236e4ed26a 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -2723,7 +2723,7 @@ static int g4x_hrawclk(struct drm_i915_private
> *dev_priv)
> */
> void intel_update_rawclk(struct drm_i915_private *dev_priv)
> {
> - if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv))
> + if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP)
> dev_priv->rawclk_freq = cnp_rawclk(dev_priv);
> else if (HAS_PCH_SPLIT(dev_priv))
> dev_priv->rawclk_freq = pch_rawclk(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e1a051c0fbfe..acd2336bb214 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -949,8 +949,8 @@ static void intel_pps_get_registers(struct intel_dp
> *intel_dp,
> regs->pp_stat = PP_STATUS(pps_idx);
> regs->pp_on = PP_ON_DELAYS(pps_idx);
> regs->pp_off = PP_OFF_DELAYS(pps_idx);
> - if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) &&
> - !HAS_PCH_ICP(dev_priv))
> + if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE &&
> + INTEL_PCH_TYPE(dev_priv) < PCH_CNP)
This is not right, starts to require PCH.
> regs->pp_div = PP_DIVISOR(pps_idx);
> }
>
> @@ -6431,8 +6431,8 @@ intel_pps_readout_hw_state(struct intel_dp *intel_dp,
> struct edp_power_seq *seq)
>
> pp_on = I915_READ(regs.pp_on);
> pp_off = I915_READ(regs.pp_off);
> - if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) &&
> - !HAS_PCH_ICP(dev_priv)) {
> + if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE &&
> +
[Intel-gfx] [PATCH 3/3] drm/i915: Start using comparative INTEL_PCH_TYPE
In order to make it easier to bring up new platforms
without having to take care about all corner cases
that was previously taken care for previous platforms
we already use comparative INTEL_GEN statements.
Let's start doing the same with PCH.
The only caveats are:
- for less-than comparisons we need to be careful
and check PCH_NONE < pch < PCH_CNP.
- It is not necessarily a chronological order, but a matter
of south display compatibility/inheritance.
Cc: Ville Syrjälä
Cc: Lucas De Marchi
Signed-off-by: Rodrigo Vivi
---
drivers/gpu/drm/i915/i915_drv.h| 6 ++
drivers/gpu/drm/i915/i915_irq.c| 7 ++-
drivers/gpu/drm/i915/intel_cdclk.c | 2 +-
drivers/gpu/drm/i915/intel_dp.c| 21 +
drivers/gpu/drm/i915/intel_panel.c | 5 ++---
5 files changed, 20 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e6be327ba86d..e327736c76a0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -523,6 +523,12 @@ struct i915_psr {
u16 su_x_granularity;
};
+/*
+ * Sorted by south display engine compatibility.
+ * If the new PCH comes with a south display engine that is not
+ * inherited from the latest item, please do not add it to the
+ * end. Instead, add it right after its "parent" PCH.
+ */
enum intel_pch {
PCH_NOP = -1, /* PCH without south display */
PCH_NONE = 0, /* No PCH present */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a42eb6394b69..923135d6b781 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2833,9 +2833,7 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv,
u32 master_ctl)
if (HAS_PCH_ICP(dev_priv))
icp_irq_handler(dev_priv, iir);
- else if (HAS_PCH_SPT(dev_priv) ||
-HAS_PCH_KBP(dev_priv) ||
-HAS_PCH_CNP(dev_priv))
+ else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT)
spt_irq_handler(dev_priv, iir);
else
cpt_irq_handler(dev_priv, iir);
@@ -4620,8 +4618,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
dev->driver->disable_vblank = gen8_disable_vblank;
if (IS_GEN9_LP(dev_priv))
dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
- else if (HAS_PCH_SPT(dev_priv) || HAS_PCH_KBP(dev_priv) ||
-HAS_PCH_CNP(dev_priv))
+ 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;
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
b/drivers/gpu/drm/i915/intel_cdclk.c
index 7e5132772477..9d236e4ed26a 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -2723,7 +2723,7 @@ static int g4x_hrawclk(struct drm_i915_private *dev_priv)
*/
void intel_update_rawclk(struct drm_i915_private *dev_priv)
{
- if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv))
+ if (INTEL_PCH_TYPE(dev_priv) >= PCH_CNP)
dev_priv->rawclk_freq = cnp_rawclk(dev_priv);
else if (HAS_PCH_SPLIT(dev_priv))
dev_priv->rawclk_freq = pch_rawclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e1a051c0fbfe..acd2336bb214 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -949,8 +949,8 @@ static void intel_pps_get_registers(struct intel_dp
*intel_dp,
regs->pp_stat = PP_STATUS(pps_idx);
regs->pp_on = PP_ON_DELAYS(pps_idx);
regs->pp_off = PP_OFF_DELAYS(pps_idx);
- if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) &&
- !HAS_PCH_ICP(dev_priv))
+ if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE &&
+ INTEL_PCH_TYPE(dev_priv) < PCH_CNP)
regs->pp_div = PP_DIVISOR(pps_idx);
}
@@ -6431,8 +6431,8 @@ intel_pps_readout_hw_state(struct intel_dp *intel_dp,
struct edp_power_seq *seq)
pp_on = I915_READ(regs.pp_on);
pp_off = I915_READ(regs.pp_off);
- if (!IS_GEN9_LP(dev_priv) && !HAS_PCH_CNP(dev_priv) &&
- !HAS_PCH_ICP(dev_priv)) {
+ if (INTEL_PCH_TYPE(dev_priv) > PCH_NONE &&
+ INTEL_PCH_TYPE(dev_priv) < PCH_CNP) {
I915_WRITE(regs.pp_ctrl, pp_ctl);
pp_div = I915_READ(regs.pp_div);
}
@@ -6450,8 +6450,7 @@ intel_pps_readout_hw_state(struct intel_dp *intel_dp,
struct edp_power_seq *seq)
seq->t10 = (pp_off & PANEL_POWER_DOWN_DELAY_MASK) >>
PANEL_POWER_DOWN_DELAY_SHIFT;
- if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
-
