| Message ID | 20181018233447.5187-3-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | re-organize a bit gen10 and gen11 | expand |
On Thu, 18 Oct 2018, Rodrigo Vivi <rodrigo.vivi@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@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 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; > > 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) > 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)) > 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)) {
On Fri, Oct 19, 2018 at 10:39:46AM +0300, Jani Nikula wrote: > On Thu, 18 Oct 2018, Rodrigo Vivi <rodrigo.vivi@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@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 think I've chatted about this with Chris too. This should be easy to fix since it's encoded in the pci id. Just need a new separate entry just for this one. -Daniel > > 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; > > > > 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) > > 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)) > > 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 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Oct 19, 2018 at 10:39:46AM +0300, Jani Nikula wrote: > On Thu, 18 Oct 2018, Rodrigo Vivi <rodrigo.vivi@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@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
On Fri, Oct 19, 2018 at 09:46:13AM -0700, Lucas De Marchi wrote: > On Fri, Oct 19, 2018 at 10:39:46AM +0300, Jani Nikula wrote: > > On Thu, 18 Oct 2018, Rodrigo Vivi <rodrigo.vivi@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@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. makes sense. I will change. > > > > > 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? maybe probably deserves a separated patch with proper explanation? but the only case were this case PORT_F can be reached is that this is a CNL_WITH_PORT_F or this is an ICELAKE... So the IS_CANNONLAKE or IS_GEN10 is enough here. If regular CNL was taking this path before this patch it would end with HPD_PORT_F which doesn't exist in any cnl anyway. > > 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
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; +} + 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; 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) 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)) 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)) {
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@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(-)