| Message ID | 1360688819-31968-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 12, 2013 at 07:06:59PM +0200, Jani Nikula wrote: > Keep all the platform output selection in intel_output_setup(), and don't > scatter it around. I see this as doing the opposite. You are littering an already over complicated routine with LVDS specific information. > As a useful side effect, do not try to enable LVDS on > HSW or VLV. But you wouldn't with the old arrangement either. > Some checks are done in a slightly different order than before, and on some > platforms VGA is now initialized before LVDS. Is that significant? You have not sold me on the benefits of this change. -Chris
On Tue, 12 Feb 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Tue, Feb 12, 2013 at 07:06:59PM +0200, Jani Nikula wrote: >> Keep all the platform output selection in intel_output_setup(), and don't >> scatter it around. > > I see this as doing the opposite. You are littering an already over > complicated routine with LVDS specific information. I think it's fairly straightforward to follow what's happening there, and with all the platform dependent stuff in the same place you don't have to dig down from that complicated routine to look for *other* places where there are platform dependent ifs and checks. Especially if we need to add *more* of those deep down. >> As a useful side effect, do not try to enable LVDS on >> HSW or VLV. > > But you wouldn't with the old arrangement either. On HSW it would depend on I915_READ(PCH_LVDS) & LVDS_DETECTED, which I believe it shouldn't, and VBT. On mobile VLV it depends on VBT only. And I think we know better than the VBT. Unless I'm missing something and you have to spell it out for me... :/ >> Some checks are done in a slightly different order than before, and on some >> platforms VGA is now initialized before LVDS. > > Is that significant? I don't think so, but I thought I'd write it down as a note just in case it turns out to be significant. > You have not sold me on the benefits of this change. The main motivation was: http://mid.gmane.org/CAKMK7uGxFYUZckSbdKqqF15Lgcp9VQgKwCU1-hhTQ0VmrzvY2Q@mail.gmail.com BR, Jani.
On Tue, Feb 12, 2013 at 07:37:08PM +0200, Jani Nikula wrote: > On Tue, 12 Feb 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Tue, Feb 12, 2013 at 07:06:59PM +0200, Jani Nikula wrote: > >> Keep all the platform output selection in intel_output_setup(), and don't > >> scatter it around. > > > > I see this as doing the opposite. You are littering an already over > > complicated routine with LVDS specific information. > > I think it's fairly straightforward to follow what's happening there, > and with all the platform dependent stuff in the same place you don't > have to dig down from that complicated routine to look for *other* > places where there are platform dependent ifs and checks. Especially if > we need to add *more* of those deep down. I consider the rest of those to be a mess of repetious code that would have been simpler had the platform specific presence checks been pushed down. The only critical aspect of that function is the ordering and more importantly when to and when not to initialise certain outputs. That I feel is hidden in the forest of ifs. Just my 2 cents. -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d75c6a0..a1fb320 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8198,12 +8198,19 @@ static void intel_setup_outputs(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; struct intel_encoder *encoder; bool dpd_is_edp = false; - bool has_lvds; - has_lvds = intel_lvds_init(dev); - if (!has_lvds && !HAS_PCH_SPLIT(dev)) { - /* disable the panel fitter on everything but LVDS */ - I915_WRITE(PFIT_CONTROL, 0); + if (!HAS_PCH_SPLIT(dev)) { + bool has_lvds = false; + + /* Prior to PCH split LVDS was only attached to mobile products, + * except for the inglorious 830gm */ + if (IS_MOBILE(dev) && !IS_I830(dev)) + has_lvds = intel_lvds_init(dev); + + if (!has_lvds) { + /* disable the panel fitter on everything but LVDS */ + I915_WRITE(PFIT_CONTROL, 0); + } } if (!(HAS_DDI(dev) && (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES))) @@ -8230,6 +8237,14 @@ static void intel_setup_outputs(struct drm_device *dev) intel_ddi_init(dev, PORT_D); } else if (HAS_PCH_SPLIT(dev)) { int found; + + if (I915_READ(PCH_LVDS) & LVDS_DETECTED) { + if (dev_priv->edp.support) + DRM_DEBUG_KMS("disable LVDS for eDP support\n"); + else + intel_lvds_init(dev); + } + dpd_is_edp = intel_dpd_is_edp(dev); if (has_edp_a(dev)) diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 7e4ec63..3c76e91 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -1024,18 +1024,6 @@ static bool compute_is_dual_link_lvds(struct intel_lvds_encoder *lvds_encoder) return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP; } -static bool intel_lvds_supported(struct drm_device *dev) -{ - /* With the introduction of the PCH we gained a dedicated - * LVDS presence pin, use it. */ - if (HAS_PCH_SPLIT(dev)) - return true; - - /* Otherwise LVDS was only attached to mobile products, - * except for the inglorious 830gm */ - return IS_MOBILE(dev) && !IS_I830(dev); -} - /** * intel_lvds_init - setup LVDS connectors on this device * @dev: drm device @@ -1060,9 +1048,6 @@ bool intel_lvds_init(struct drm_device *dev) int pipe; u8 pin; - if (!intel_lvds_supported(dev)) - return false; - /* Skip init on machines we know falsely report LVDS */ if (dmi_check_system(intel_no_lvds)) return false; @@ -1073,15 +1058,6 @@ bool intel_lvds_init(struct drm_device *dev) return false; } - if (HAS_PCH_SPLIT(dev)) { - if ((I915_READ(PCH_LVDS) & LVDS_DETECTED) == 0) - return false; - if (dev_priv->edp.support) { - DRM_DEBUG_KMS("disable LVDS for eDP support\n"); - return false; - } - } - lvds_encoder = kzalloc(sizeof(struct intel_lvds_encoder), GFP_KERNEL); if (!lvds_encoder) return false;
Keep all the platform output selection in intel_output_setup(), and don't scatter it around. As a useful side effect, do not try to enable LVDS on HSW or VLV. Some checks are done in a slightly different order than before, and on some platforms VGA is now initialized before LVDS. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- I took this out of the VLV series to give it the attention it needs. --- drivers/gpu/drm/i915/intel_display.c | 25 ++++++++++++++++++++----- drivers/gpu/drm/i915/intel_lvds.c | 24 ------------------------ 2 files changed, 20 insertions(+), 29 deletions(-)