Re: [PATCH 2/3] drm/i915: get rid of dev_priv->info->has_pch_split

From: Paulo Zanoni <przanoni@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/3] drm/i915: get rid of dev_priv->info->has_pch_split
Date: Tue, 3 Jul 2012 17:46:39 -0300	[thread overview]
Message-ID: <CA+gsUGSV8s9oFdrvYZJqNL8Tn8tJGA_=NNKKcpGb=gdKTQjrXA@mail.gmail.com> (raw)
In-Reply-To: <20120703191528.GD5103@phenom.ffwll.local>

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

-- 
Paulo Zanoni