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.
> - 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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
