| Message ID | 1341341853-4092-2-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 03, 2012 at 03:57:32PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Previously we had has_pch_split to tell us whether we had a PCH or not > and we also had dev_priv->pch_type to tell us which kind of PCH it > was, but it could only be used if we were 100% sure we did have a PCH. > Now that PCH_NONE was added to dev_priv->pch_type we don't need > has_pch_split anymore: we can just check for pch_type != PCH_NONE. > > The HAS_PCH_{IBX,CPT,LPT} macros use dev_priv->pch_type, so they can > only be called after intel_detect_pch. The HAS_PCH_SPLIT macro looks > at dev_priv->info->has_pch_split, which is available earlier. > > Since the goal is to implement HAS_PCH_SPLIT using dev_priv->pch_type > instead of dev_priv->info->has_pch_split, we need to make sure that > intel_detect_pch is called before any calls to HAS_PCH_SPLIT are made. > So we moved the intel_detect_pch call to an earlier stage. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_dma.c | 5 +++-- > drivers/gpu/drm/i915/i915_drv.c | 8 -------- > drivers/gpu/drm/i915/i915_drv.h | 3 +-- > 3 files changed, 4 insertions(+), 12 deletions(-) > > This patch does not solve any real problem: it's just a "suggestion" > of something we could do after the previous patch. Some people may > argue that looking at the has_pch_split variable might make it easier > for us to find out which machines actually have a pch split without > running the machine. So I really won't complain if we don't accept > this patch: patch 01 is the important one. > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 2166519..f8bc9ea 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1547,6 +1547,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > goto out_mtrrfree; > } > > + /* This must be called before any calls to HAS_PCH_* */ > + intel_detect_pch(dev); > + Hm, what about defining PCH_NONE as -1, PCH_RESERVED as 0 and then adding a WARN_ON(dev_priv->pch_type == PCH_RESERVED)? detect_pch is called unconditionally, and that way we would catch this. Might be overkill otoh, so if you think this is not worth it, np. -Daniel > intel_irq_init(dev); > > /* Try to make sure MCHBAR is enabled before poking at it */ > @@ -1599,8 +1602,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > /* Start out suspended */ > dev_priv->mm.suspended = 1; > > - intel_detect_pch(dev); > - > if (drm_core_check_feature(dev, DRIVER_MODESET)) { > ret = i915_load_modeset_init(dev); > if (ret < 0) { > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 7d0eb82..1794833 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -225,7 +225,6 @@ static const struct intel_device_info intel_ironlake_d_info = { > .gen = 5, > .need_gfx_hws = 1, .has_hotplug = 1, > .has_bsd_ring = 1, > - .has_pch_split = 1, > }; > > static const struct intel_device_info intel_ironlake_m_info = { > @@ -233,7 +232,6 @@ static const struct intel_device_info intel_ironlake_m_info = { > .need_gfx_hws = 1, .has_hotplug = 1, > .has_fbc = 1, > .has_bsd_ring = 1, > - .has_pch_split = 1, > }; > > static const struct intel_device_info intel_sandybridge_d_info = { > @@ -242,7 +240,6 @@ static const struct intel_device_info intel_sandybridge_d_info = { > .has_bsd_ring = 1, > .has_blt_ring = 1, > .has_llc = 1, > - .has_pch_split = 1, > .has_force_wake = 1, > }; > > @@ -253,7 +250,6 @@ static const struct intel_device_info intel_sandybridge_m_info = { > .has_bsd_ring = 1, > .has_blt_ring = 1, > .has_llc = 1, > - .has_pch_split = 1, > .has_force_wake = 1, > }; > > @@ -263,7 +259,6 @@ static const struct intel_device_info intel_ivybridge_d_info = { > .has_bsd_ring = 1, > .has_blt_ring = 1, > .has_llc = 1, > - .has_pch_split = 1, > .has_force_wake = 1, > }; > > @@ -274,7 +269,6 @@ static const struct intel_device_info intel_ivybridge_m_info = { > .has_bsd_ring = 1, > .has_blt_ring = 1, > .has_llc = 1, > - .has_pch_split = 1, > .has_force_wake = 1, > }; > > @@ -302,7 +296,6 @@ static const struct intel_device_info intel_haswell_d_info = { > .has_bsd_ring = 1, > .has_blt_ring = 1, > .has_llc = 1, > - .has_pch_split = 1, > .has_force_wake = 1, > }; > > @@ -312,7 +305,6 @@ static const struct intel_device_info intel_haswell_m_info = { > .has_bsd_ring = 1, > .has_blt_ring = 1, > .has_llc = 1, > - .has_pch_split = 1, > .has_force_wake = 1, > }; > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b12e79a..89025ab 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -285,7 +285,6 @@ struct intel_device_info { > u8 is_crestline:1; > u8 is_ivybridge:1; > u8 is_valleyview:1; > - u8 has_pch_split:1; > u8 has_force_wake:1; > u8 is_haswell:1; > u8 has_fbc:1; > @@ -1113,13 +1112,13 @@ struct drm_i915_file_private { > #define HAS_PIPE_CXSR(dev) (INTEL_INFO(dev)->has_pipe_cxsr) > #define I915_HAS_FBC(dev) (INTEL_INFO(dev)->has_fbc) > > -#define HAS_PCH_SPLIT(dev) (INTEL_INFO(dev)->has_pch_split) > #define HAS_PIPE_CONTROL(dev) (INTEL_INFO(dev)->gen >= 5) > > #define INTEL_PCH_TYPE(dev) (((struct drm_i915_private *)(dev)->dev_private)->pch_type) > #define HAS_PCH_LPT(dev) (INTEL_PCH_TYPE(dev) == PCH_LPT) > #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT) > #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX) > +#define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE) > > #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake) > > -- > 1.7.10.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2012/7/3 Daniel Vetter <daniel@ffwll.ch>: > Hm, what about defining PCH_NONE as -1, PCH_RESERVED as 0 and then adding > a WARN_ON(dev_priv->pch_type == PCH_RESERVED)? detect_pch is called > unconditionally, and that way we would catch this. Might be overkill otoh, > so if you think this is not worth it, np. I actually thought about this idea before. But it would only make sense if we add these WARNs to the HAS_PCH_FOO macros. But these macros are supposed to be simple and cheap and fast... Do we really want to start adding assertions inside them? If we think the price is worth paying, then we might do as you suggested. Or maybe this could be inside some #ifdef DEBUG... On my local machines, I changed the HAS_PCH_FOO macros to print some stuff so I could check whether any of them was called before intel_detect_pch. At least on these machines (SNB, HSW and a netbook), everything was fine. > -Daniel > >> intel_irq_init(dev); >> >> /* Try to make sure MCHBAR is enabled before poking at it */ >> @@ -1599,8 +1602,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) >> /* Start out suspended */ >> dev_priv->mm.suspended = 1; >> >> - intel_detect_pch(dev); >> - >> if (drm_core_check_feature(dev, DRIVER_MODESET)) { >> ret = i915_load_modeset_init(dev); >> if (ret < 0) { >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 7d0eb82..1794833 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -225,7 +225,6 @@ static const struct intel_device_info intel_ironlake_d_info = { >> .gen = 5, >> .need_gfx_hws = 1, .has_hotplug = 1, >> .has_bsd_ring = 1, >> - .has_pch_split = 1, >> }; >> >> static const struct intel_device_info intel_ironlake_m_info = { >> @@ -233,7 +232,6 @@ static const struct intel_device_info intel_ironlake_m_info = { >> .need_gfx_hws = 1, .has_hotplug = 1, >> .has_fbc = 1, >> .has_bsd_ring = 1, >> - .has_pch_split = 1, >> }; >> >> static const struct intel_device_info intel_sandybridge_d_info = { >> @@ -242,7 +240,6 @@ static const struct intel_device_info intel_sandybridge_d_info = { >> .has_bsd_ring = 1, >> .has_blt_ring = 1, >> .has_llc = 1, >> - .has_pch_split = 1, >> .has_force_wake = 1, >> }; >> >> @@ -253,7 +250,6 @@ static const struct intel_device_info intel_sandybridge_m_info = { >> .has_bsd_ring = 1, >> .has_blt_ring = 1, >> .has_llc = 1, >> - .has_pch_split = 1, >> .has_force_wake = 1, >> }; >> >> @@ -263,7 +259,6 @@ static const struct intel_device_info intel_ivybridge_d_info = { >> .has_bsd_ring = 1, >> .has_blt_ring = 1, >> .has_llc = 1, >> - .has_pch_split = 1, >> .has_force_wake = 1, >> }; >> >> @@ -274,7 +269,6 @@ static const struct intel_device_info intel_ivybridge_m_info = { >> .has_bsd_ring = 1, >> .has_blt_ring = 1, >> .has_llc = 1, >> - .has_pch_split = 1, >> .has_force_wake = 1, >> }; >> >> @@ -302,7 +296,6 @@ static const struct intel_device_info intel_haswell_d_info = { >> .has_bsd_ring = 1, >> .has_blt_ring = 1, >> .has_llc = 1, >> - .has_pch_split = 1, >> .has_force_wake = 1, >> }; >> >> @@ -312,7 +305,6 @@ static const struct intel_device_info intel_haswell_m_info = { >> .has_bsd_ring = 1, >> .has_blt_ring = 1, >> .has_llc = 1, >> - .has_pch_split = 1, >> .has_force_wake = 1, >> }; >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index b12e79a..89025ab 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -285,7 +285,6 @@ struct intel_device_info { >> u8 is_crestline:1; >> u8 is_ivybridge:1; >> u8 is_valleyview:1; >> - u8 has_pch_split:1; >> u8 has_force_wake:1; >> u8 is_haswell:1; >> u8 has_fbc:1; >> @@ -1113,13 +1112,13 @@ struct drm_i915_file_private { >> #define HAS_PIPE_CXSR(dev) (INTEL_INFO(dev)->has_pipe_cxsr) >> #define I915_HAS_FBC(dev) (INTEL_INFO(dev)->has_fbc) >> >> -#define HAS_PCH_SPLIT(dev) (INTEL_INFO(dev)->has_pch_split) >> #define HAS_PIPE_CONTROL(dev) (INTEL_INFO(dev)->gen >= 5) >> >> #define INTEL_PCH_TYPE(dev) (((struct drm_i915_private *)(dev)->dev_private)->pch_type) >> #define HAS_PCH_LPT(dev) (INTEL_PCH_TYPE(dev) == PCH_LPT) >> #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT) >> #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX) >> +#define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE) >> >> #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake) >> >> -- >> 1.7.10.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Mail: daniel@ffwll.ch > Mobile: +41 (0)79 365 57 48
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 2166519..f8bc9ea 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1547,6 +1547,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) goto out_mtrrfree; } + /* This must be called before any calls to HAS_PCH_* */ + intel_detect_pch(dev); + intel_irq_init(dev); /* Try to make sure MCHBAR is enabled before poking at it */ @@ -1599,8 +1602,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) /* Start out suspended */ dev_priv->mm.suspended = 1; - intel_detect_pch(dev); - if (drm_core_check_feature(dev, DRIVER_MODESET)) { ret = i915_load_modeset_init(dev); if (ret < 0) { diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 7d0eb82..1794833 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -225,7 +225,6 @@ static const struct intel_device_info intel_ironlake_d_info = { .gen = 5, .need_gfx_hws = 1, .has_hotplug = 1, .has_bsd_ring = 1, - .has_pch_split = 1, }; static const struct intel_device_info intel_ironlake_m_info = { @@ -233,7 +232,6 @@ static const struct intel_device_info intel_ironlake_m_info = { .need_gfx_hws = 1, .has_hotplug = 1, .has_fbc = 1, .has_bsd_ring = 1, - .has_pch_split = 1, }; static const struct intel_device_info intel_sandybridge_d_info = { @@ -242,7 +240,6 @@ static const struct intel_device_info intel_sandybridge_d_info = { .has_bsd_ring = 1, .has_blt_ring = 1, .has_llc = 1, - .has_pch_split = 1, .has_force_wake = 1, }; @@ -253,7 +250,6 @@ static const struct intel_device_info intel_sandybridge_m_info = { .has_bsd_ring = 1, .has_blt_ring = 1, .has_llc = 1, - .has_pch_split = 1, .has_force_wake = 1, }; @@ -263,7 +259,6 @@ static const struct intel_device_info intel_ivybridge_d_info = { .has_bsd_ring = 1, .has_blt_ring = 1, .has_llc = 1, - .has_pch_split = 1, .has_force_wake = 1, }; @@ -274,7 +269,6 @@ static const struct intel_device_info intel_ivybridge_m_info = { .has_bsd_ring = 1, .has_blt_ring = 1, .has_llc = 1, - .has_pch_split = 1, .has_force_wake = 1, }; @@ -302,7 +296,6 @@ static const struct intel_device_info intel_haswell_d_info = { .has_bsd_ring = 1, .has_blt_ring = 1, .has_llc = 1, - .has_pch_split = 1, .has_force_wake = 1, }; @@ -312,7 +305,6 @@ static const struct intel_device_info intel_haswell_m_info = { .has_bsd_ring = 1, .has_blt_ring = 1, .has_llc = 1, - .has_pch_split = 1, .has_force_wake = 1, }; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b12e79a..89025ab 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -285,7 +285,6 @@ struct intel_device_info { u8 is_crestline:1; u8 is_ivybridge:1; u8 is_valleyview:1; - u8 has_pch_split:1; u8 has_force_wake:1; u8 is_haswell:1; u8 has_fbc:1; @@ -1113,13 +1112,13 @@ struct drm_i915_file_private { #define HAS_PIPE_CXSR(dev) (INTEL_INFO(dev)->has_pipe_cxsr) #define I915_HAS_FBC(dev) (INTEL_INFO(dev)->has_fbc) -#define HAS_PCH_SPLIT(dev) (INTEL_INFO(dev)->has_pch_split) #define HAS_PIPE_CONTROL(dev) (INTEL_INFO(dev)->gen >= 5) #define INTEL_PCH_TYPE(dev) (((struct drm_i915_private *)(dev)->dev_private)->pch_type) #define HAS_PCH_LPT(dev) (INTEL_PCH_TYPE(dev) == PCH_LPT) #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT) #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX) +#define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE) #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake)