[Intel-gfx] [RFC 2/8] drm/i915: Use the ddi_ports info to kill ugly CNL_WITH_PORT_F.
Lucas De Marchi
lucas.demarchi at intel.com
Fri Oct 19 16:46:13 UTC 2018
On Fri, Oct 19, 2018 at 10:39:46AM +0300, Jani Nikula wrote:
> On Thu, 18 Oct 2018, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
> > Now that we have the number of ddi ports information available
> > let's use it instead of that ugly platform macro.
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
> > drivers/gpu/drm/i915/i915_drv.h | 2 --
> > drivers/gpu/drm/i915/i915_irq.c | 5 ++---
> > drivers/gpu/drm/i915/intel_dp.c | 2 +-
> > drivers/gpu/drm/i915/intel_hotplug.c | 2 +-
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
> > 6 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index baac35f698f9..83ab325c6675 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -239,6 +239,14 @@ intel_virt_detect_pch(const struct drm_i915_private *dev_priv)
> > return id;
> > }
> >
> > +static void intel_adjust_skus_differences(struct drm_i915_private *dev_priv)
> > +{
> > + /* Some Cannonlake SKUs have Port F */
> > + if (IS_CANNONLAKE(dev_priv) &&
> > + (INTEL_DEVID(dev_priv) & 0x0004) == 0x0004)
> > + mkwrite_device_info(dev_priv)->ddi_ports = 6;
> > +}
>
> I'd like reduce the amount of device info stuff we adjust at probe, not
> increase.
>
> Long term, I'd like to make dev_priv->info a const pointer to the const
> data in i915_pci.c, and make it truly immutable. We could have an
> additional runtime device info to complement that, but this approach
> calls for a more concrete split in const and non-const data, not "mostly
> const but sometimes mutable".
I agree with this. I'd also like to have a const dev_info. What I think we need is to split
in what is const what can be changed in runtime/initialization.
However, I'm not fond on using the number of ddi ports to take decisions in the code not
related to the number of ports, but rather with other things of that sku. See below.
>
> I think I've chatted about this with Chris too.
>
> BR,
> Jani.
>
> > +
> > static void intel_detect_pch(struct drm_i915_private *dev_priv)
> > {
> > struct pci_dev *pch = NULL;
> > @@ -904,6 +912,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv)
> > if (ret < 0)
> > goto err_workqueues;
> >
> > + intel_adjust_skus_differences(dev_priv);
> > +
> > /* This must be called before any calls to HAS_PCH_* */
> > intel_detect_pch(dev_priv);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 7ad232849268..99e42df79ed8 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2485,8 +2485,6 @@ intel_info(const struct drm_i915_private *dev_priv)
> > (dev_priv)->info.gt == 2)
> > #define IS_CFL_GT3(dev_priv) (IS_COFFEELAKE(dev_priv) && \
> > (dev_priv)->info.gt == 3)
> > -#define IS_CNL_WITH_PORT_F(dev_priv) (IS_CANNONLAKE(dev_priv) && \
> > - (INTEL_DEVID(dev_priv) & 0x0004) == 0x0004)
> >
> > #define IS_ALPHA_SUPPORT(intel_info) ((intel_info)->is_alpha_support)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 5d1f53723388..63d676de3840 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2782,8 +2782,7 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> > if (INTEL_GEN(dev_priv) >= 11)
> > tmp_mask |= ICL_AUX_CHANNEL_E;
> >
> > - if (IS_CNL_WITH_PORT_F(dev_priv) ||
> > - INTEL_GEN(dev_priv) >= 11)
> > + if (INTEL_INFO(dev_priv)->ddi_ports >= 6)
> > tmp_mask |= CNL_AUX_CHANNEL_F;
ok, this is directly related to the number of ports
> >
> > if (iir & tmp_mask) {
> > @@ -4220,7 +4219,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> > if (INTEL_GEN(dev_priv) >= 11)
> > de_port_masked |= ICL_AUX_CHANNEL_E;
> >
> > - if (IS_CNL_WITH_PORT_F(dev_priv) || INTEL_GEN(dev_priv) >= 11)
> > + if (INTEL_INFO(dev_priv)->ddi_ports >= 6)
> > de_port_masked |= CNL_AUX_CHANNEL_F;
> >
> > de_pipe_enables = de_pipe_masked | GEN8_PIPE_VBLANK |
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 3384a9bbdafd..0ea0414ccef4 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -402,7 +402,7 @@ static int cnl_max_source_rate(struct intel_dp *intel_dp)
> > return 540000;
> >
> > /* For this SKU 8.1G is supported in all ports */
> > - if (IS_CNL_WITH_PORT_F(dev_priv))
> > + if (INTEL_INFO(dev_priv)->ddi_ports == 6)
this is not. And anyone reading this will have to chase back what it means to have 6 ddi ports here.
So we may need to create a more useful macro (that can in fact check by the number of ports).
Places in which the number of ports is not related to what's being checked will be much more
difficult to check to support newer platforms. Explicit variables in dev_info won't.
> > return 810000;
> >
> > /* For other SKUs, max rate on ports A and D is 5.4G */
> > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> > index 648a13c6043c..05d1faf77048 100644
> > --- a/drivers/gpu/drm/i915/intel_hotplug.c
> > +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> > @@ -101,7 +101,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
> > case PORT_E:
> > return HPD_PORT_E;
> > case PORT_F:
> > - if (IS_CNL_WITH_PORT_F(dev_priv))
> > + if (IS_GEN10(dev_priv))
this seems wrong, no? missing a && ddi_ports == 6?
Lucas De Marchi
> > return HPD_PORT_E;
> > return HPD_PORT_F;
> > default:
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 31a49bdcf193..80e14be11279 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -3099,7 +3099,7 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
> > * timeouts, lets remove them from the list
> > * for the SKUs without port F.
> > */
> > - if (!IS_CNL_WITH_PORT_F(dev_priv))
> > + if (INTEL_INFO(dev_priv)->ddi_ports == 5)
> > power_domains->power_well_count -= 2;
> >
> > } else if (IS_BROXTON(dev_priv)) {
>
> --
> Jani Nikula, Intel Open Source Graphics Center
More information about the Intel-gfx
mailing list