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
