Re: [PATCH 1/2] drm/i915: split off Pineview desktop and mobile device info

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 08 Dec 2016, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Thu, Dec 08, 2016 at 04:58:21PM +0100, Daniel Vetter wrote:
>> On Thu, Dec 08, 2016 at 11:11:50AM +0200, Jani Nikula wrote:
>> > On Thu, 08 Dec 2016, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
>> > > On Thu, 08 Dec 2016, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
>> > >> This lets us use IS_MOBILE() for distinguishing the desktop and mobile
>> > >> parts instead of duplicating PCI IDs in Pineview specific checks.
>> > >
>> > > This probably needs more thorough review of the !IS_MOBILE() paths in
>> > > parts not touched by this patch.
>> > 
>> > And Daniel says,
>> > 
>> >  11:06          danvet   j4ni, pnv is_mobile, even for the desktop version
>> >  11:06          danvet   iirc
>> >  11:07          danvet   because is_mobile has nothing to do with whether the 
>> >                          chip is for laptops or not
>> > 
>> > so better just forget about these two patches I guess.
>> 
>> Yeah, IS_MOBILE is seriously confusing in our code. On gen2/3 it was
>> essentially matching HAS_LVDS (but we never made that one),
>
> We do use it in quite a few places to differentiate between the mobile
> and desktop variants of the chipset. There are some weird differences
> between the two branches of the family, eg. some MCHBAR registers live
> at different offsets, and swizzling stuff is different IIRC.
>
>> later on the
>> splits started to happened on different axis (atom vs big core and stuff
>> like that), but somehow IS_MOBILE stuck around to this day and is
>> sometimes abused to restrict w/a to specific chips. If someone wants to
>> clean this up:
>> 
>> - Remove IS_MOBILE on gen5+, use specific platforms/pci ids where we need
>>   it.
>
> IIRC I did look into that once, and the only place where it was
> used for ilk-ivb was eDP setup, as in "do we have port A or not?".
> For hsw+ we don't use it at all, and IIRC don't even set it.

There's IS_IRONLAKE_M() which looks at the PCI ID, but essentially it's
IS_IRONLAKE && IS_MOBILE.

I really dislike direct PCI ID checks in the IS_* macros, because they
duplicate PCI IDs from i915_pciids.h, which typically already has the
IDs grouped at the granularity we'd like (mobile vs. not in this
case). I wish we could do all the IS_*_{ULX,ULT,GT?} using the PCI ID
macros from one source.

BR,
Jani.


>
>> - Rename IS_MOBILE to HAS_LVDS on gen2/3/4 (not entirely sure about gen4,
>>   would need to double-check that it makes sense).
>
> 830 doesn't have the LVDS port, so would need a bit of special care. And
> like I said. there are actual differences well outside the LVDS vs. not
> thing.
>
> If we can solve the ilk-ivb port A case, then I think we can kill
> .is_mobile. For the older platforms we have separate IS_FOO and IS_FOO_M
> type of things so those can be used instead. But for ilk-ivb we don't
> have those, I think. Or maybe we do, not sure.
>
>> - Nuke is_mobile from the chip info, it's only misleading people. Mobile
>>   is the future everywhere anyway ;-)
>> 
>> Cheers, Daniel
>> 
>> > 
>> > 
>> > BR,
>> > Jani.
>> > 
>> > 
>> > >
>> > > For example this one in i915_gem_detect_bit_6_swizzle() which was pretty
>> > > weird already before the Pineview device info patches:
>> > >
>> > > 	} else if (IS_MOBILE(dev_priv) ||
>> > > 		   (IS_GEN3(dev_priv) &&
>> > > 		    !IS_G33(dev_priv) && !IS_PINEVIEW(dev_priv))) {
>> > >
>> > > BR,
>> > > Jani.
>> > >
>> > >
>> > >>
>> > >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> > >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> > >> ---
>> > >>  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 6a08e25a48d8..34af418d88cc 100644
>> > >> --- a/arch/x86/kernel/early-quirks.c
>> > >> +++ b/arch/x86/kernel/early-quirks.c
>> > >> @@ -508,7 +508,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_D_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 1480e733312a..ee1726b28b82 100644
>> > >> --- a/drivers/gpu/drm/i915/i915_drv.h
>> > >> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > >> @@ -2616,8 +2616,6 @@ intel_info(const struct drm_i915_private *dev_priv)
>> > >>  #define IS_G45(dev_priv)	((dev_priv)->info.platform == INTEL_G45)
>> > >>  #define IS_GM45(dev_priv)	((dev_priv)->info.platform == 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)	((dev_priv)->info.platform == INTEL_PINEVIEW)
>> > >>  #define IS_G33(dev_priv)	((dev_priv)->info.platform == 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 93f50ef2a309..451113f79499 100644
>> > >> --- a/drivers/gpu/drm/i915/i915_pci.c
>> > >> +++ b/drivers/gpu/drm/i915/i915_pci.c
>> > >> @@ -139,7 +139,14 @@ static const struct intel_device_info intel_g33_info = {
>> > >>  	.has_overlay = 1,
>> > >>  };
>> > >>  
>> > >> -static const struct intel_device_info intel_pineview_info = {
>> > >> +static const struct intel_device_info intel_pineview_d_info = {
>> > >> +	GEN3_FEATURES,
>> > >> +	.platform = INTEL_PINEVIEW,
>> > >> +	.has_hotplug = 1,
>> > >> +	.has_overlay = 1,
>> > >> +};
>> > >> +
>> > >> +static const struct intel_device_info intel_pineview_m_info = {
>> > >>  	GEN3_FEATURES,
>> > >>  	.platform = INTEL_PINEVIEW, .is_mobile = 1,
>> > >>  	.has_hotplug = 1,
>> > >> @@ -444,7 +451,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_D_IDS(&intel_pineview_d_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_IDS(&intel_sandybridge_d_info),
>> > >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> > >> index 9171431558a3..afe07947e51c 100644
>> > >> --- a/drivers/gpu/drm/i915/intel_pm.c
>> > >> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > >> @@ -659,7 +659,7 @@ static void pineview_update_wm(struct intel_crtc *unused_crtc)
>> > >>  	u32 reg;
>> > >>  	unsigned long 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);
>> > >> @@ -7730,7 +7730,7 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
>> > >>  		vlv_setup_wm_latency(dev_priv);
>> > >>  		dev_priv->display.update_wm = vlv_update_wm;
>> > >>  	} 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 540be9ff0346..9c226eb1788f 100644
>> > >> --- a/include/drm/i915_pciids.h
>> > >> +++ b/include/drm/i915_pciids.h
>> > >> @@ -100,8 +100,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_D_IDS(info) \
>> > >> +	INTEL_VGA_DEVICE(0xa001, info)
>> > >> +
>> > >> +#define INTEL_PINEVIEW_M_IDS(info) \
>> > >>  	INTEL_VGA_DEVICE(0xa011, info)
>> > >>  
>> > >>  #define INTEL_IRONLAKE_D_IDS(info) \
>> > 
>> > -- 
>> > Jani Nikula, Intel Open Source Technology Center
>> 
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux