Re: [PATCH 1/3] drm/i915: Split Pineview device info into desktop and mobile

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 18/03/2019 13:56, Ville Syrjälä wrote:
> On Mon, Mar 18, 2019 at 09:52:56AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> This allows the IS_PINEVIEW_<G|M> macros to be removed and avoid
>> duplication of device ids already defined in i915_pciids.h.
>>
>> !IS_MOBILE check can be used in place of existing IS_PINEVIEW_G call
>> sites.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   arch/x86/kernel/early-quirks.c  |  3 ++-
>>   drivers/gpu/drm/i915/i915_drv.h |  2 --
>>   drivers/gpu/drm/i915/i915_pci.c | 12 ++++++++++--
>>   drivers/gpu/drm/i915/intel_pm.c |  4 ++--
>>   include/drm/i915_pciids.h       |  6 ++++--
>>   5 files changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
>> index 50d5848bf22e..f91d3ed2df62 100644
>> --- a/arch/x86/kernel/early-quirks.c
>> +++ b/arch/x86/kernel/early-quirks.c
>> @@ -525,7 +525,8 @@ static const struct pci_device_id intel_early_ids[] __initconst = {
>>   	INTEL_I945G_IDS(&gen3_early_ops),
>>   	INTEL_I945GM_IDS(&gen3_early_ops),
>>   	INTEL_VLV_IDS(&gen6_early_ops),
>> -	INTEL_PINEVIEW_IDS(&gen3_early_ops),
>> +	INTEL_PINEVIEW_G_IDS(&gen3_early_ops),
>> +	INTEL_PINEVIEW_M_IDS(&gen3_early_ops),
>>   	INTEL_I965G_IDS(&gen3_early_ops),
>>   	INTEL_G33_IDS(&gen3_early_ops),
>>   	INTEL_I965GM_IDS(&gen3_early_ops),
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index c65c2e6649df..80388984c9fc 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2296,8 +2296,6 @@ static inline unsigned int i915_sg_segment_size(void)
>>   #define IS_G45(dev_priv)	IS_PLATFORM(dev_priv, INTEL_G45)
>>   #define IS_GM45(dev_priv)	IS_PLATFORM(dev_priv, INTEL_GM45)
>>   #define IS_G4X(dev_priv)	(IS_G45(dev_priv) || IS_GM45(dev_priv))
>> -#define IS_PINEVIEW_G(dev_priv)	(INTEL_DEVID(dev_priv) == 0xa001)
>> -#define IS_PINEVIEW_M(dev_priv)	(INTEL_DEVID(dev_priv) == 0xa011)
>>   #define IS_PINEVIEW(dev_priv)	IS_PLATFORM(dev_priv, INTEL_PINEVIEW)
>>   #define IS_G33(dev_priv)	IS_PLATFORM(dev_priv, INTEL_G33)
>>   #define IS_IRONLAKE_M(dev_priv)	(INTEL_DEVID(dev_priv) == 0x0046)
>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>> index ef7410c492fd..4fa072a3280f 100644
>> --- a/drivers/gpu/drm/i915/i915_pci.c
>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>> @@ -257,7 +257,14 @@ static const struct intel_device_info intel_g33_info = {
>>   	.display.has_overlay = 1,
>>   };
>>   
>> -static const struct intel_device_info intel_pineview_info = {
>> +static const struct intel_device_info intel_pineview_g_info = {
>> +	GEN3_FEATURES,
>> +	PLATFORM(INTEL_PINEVIEW),
>> +	.display.has_hotplug = 1,
>> +	.display.has_overlay = 1,
>> +};
> 
> So far both PNVs were marked as "mobile". So the real concern is
> whether all the current IS_MOBILE() checks correct for pnv_g.
> 
> A quick scan points at these potentially being broken by this change:
>   i915_gem_detect_bit_6_swizzle()
>   i915_enable_asle_pipestat()
>   i9xx_enable_pll()/assert_panel_unlocked()
>   i9xx_get_pfit_config()
> 
> Also intel_hpll_vco() would change behaviour, but I don't think
> we actually use this one pnv. I just added the pnv case there
> when trawling the old configdb for the details.

Wow.. yep, when you said mess the first time around you really meant it. 
Or maybe not mess but just legacy, how it was and what made sense in the 
day.

I suggest to drop this patch and leave the PINEVIEW_G/M as subplatforms 
in the last patch, to be untangled later.

Regards,

Tvrtko

> 
>> +
>> +static const struct intel_device_info intel_pineview_m_info = {
>>   	GEN3_FEATURES,
>>   	PLATFORM(INTEL_PINEVIEW),
>>   	.is_mobile = 1,
>> @@ -753,7 +760,8 @@ static const struct pci_device_id pciidlist[] = {
>>   	INTEL_I965GM_IDS(&intel_i965gm_info),
>>   	INTEL_GM45_IDS(&intel_gm45_info),
>>   	INTEL_G45_IDS(&intel_g45_info),
>> -	INTEL_PINEVIEW_IDS(&intel_pineview_info),
>> +	INTEL_PINEVIEW_G_IDS(&intel_pineview_g_info),
>> +	INTEL_PINEVIEW_M_IDS(&intel_pineview_m_info),
>>   	INTEL_IRONLAKE_D_IDS(&intel_ironlake_d_info),
>>   	INTEL_IRONLAKE_M_IDS(&intel_ironlake_m_info),
>>   	INTEL_SNB_D_GT1_IDS(&intel_sandybridge_d_gt1_info),
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index d73b13ca57a0..24a34ccabc2a 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -850,7 +850,7 @@ static void pineview_update_wm(struct intel_crtc *unused_crtc)
>>   	u32 reg;
>>   	unsigned int wm;
>>   
>> -	latency = intel_get_cxsr_latency(IS_PINEVIEW_G(dev_priv),
>> +	latency = intel_get_cxsr_latency(!IS_MOBILE(dev_priv),
>>   					 dev_priv->is_ddr3,
>>   					 dev_priv->fsb_freq,
>>   					 dev_priv->mem_freq);
>> @@ -9561,7 +9561,7 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
>>   		dev_priv->display.initial_watermarks = g4x_initial_watermarks;
>>   		dev_priv->display.optimize_watermarks = g4x_optimize_watermarks;
>>   	} else if (IS_PINEVIEW(dev_priv)) {
>> -		if (!intel_get_cxsr_latency(IS_PINEVIEW_G(dev_priv),
>> +		if (!intel_get_cxsr_latency(!IS_MOBILE(dev_priv),
>>   					    dev_priv->is_ddr3,
>>   					    dev_priv->fsb_freq,
>>   					    dev_priv->mem_freq)) {
>> diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h
>> index d200000feeaa..a025ed9fb0be 100644
>> --- a/include/drm/i915_pciids.h
>> +++ b/include/drm/i915_pciids.h
>> @@ -108,8 +108,10 @@
>>   	INTEL_VGA_DEVICE(0x2e42, info), /* B43_G */ \
>>   	INTEL_VGA_DEVICE(0x2e92, info)	/* B43_G.1 */
>>   
>> -#define INTEL_PINEVIEW_IDS(info)			\
>> -	INTEL_VGA_DEVICE(0xa001, info),			\
>> +#define INTEL_PINEVIEW_G_IDS(info) \
>> +	INTEL_VGA_DEVICE(0xa001, info)
>> +
>> +#define INTEL_PINEVIEW_M_IDS(info) \
>>   	INTEL_VGA_DEVICE(0xa011, info)
>>   
>>   #define INTEL_IRONLAKE_D_IDS(info) \
>> -- 
>> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx