From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Kumar, Shobhit" Subject: Re: [PATCH] drm/i915: Detect if MIPI panel based on VBT and initialize only if present Date: Tue, 27 May 2014 17:40:23 +0530 Message-ID: <538480AF.2010702@intel.com> References: <1400861368-11691-1-git-send-email-shobhit.kumar@intel.com> <20140527113255.GB18389@strange.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id AC1BB6E410 for ; Tue, 27 May 2014 05:10:39 -0700 (PDT) In-Reply-To: <20140527113255.GB18389@strange.amr.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Damien Lespiau Cc: Jani Nikula , Daniel Vetter , intel-gfx List-Id: intel-gfx@lists.freedesktop.org On 5/27/2014 5:02 PM, Damien Lespiau wrote: > On Fri, May 23, 2014 at 09:39:28PM +0530, Shobhit Kumar wrote: >> It seems by default the VBT has MIPI configuration block as well. The >> Generic driver will assume always MIPI if MIPI configuration block is found. >> This is causing probelm when actually there is eDP. Fix this by looking >> into general definition block which will have device configurations. From here >> we can figure out what is the LFP type and initialize MIPI only if MIPI >> is found. >> >> Signed-off-by: Shobhit Kumar >> --- >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >> drivers/gpu/drm/i915/intel_bios.c | 20 +++++++++++++++++++- >> drivers/gpu/drm/i915/intel_display.c | 4 +++- >> 3 files changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 8e78703..5a5225b 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1229,7 +1229,9 @@ struct intel_vbt_data { >> } backlight; >> >> /* MIPI DSI */ >> + int is_mipi; > > There's a "feature" bitfield near the start of the structure, I think > you should add that field there and maybe rename it to has_mipi. will do. > >> struct { >> + u16 port; > > We store the port but never use it, is it needed? Just wanted to store as we might want to use later while enabling. Shouldn't harm ? In fact we have a need to enable PORT C internally. > >> u16 panel_id; >> struct mipi_config *config; >> struct mipi_pps_data *pps; >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c >> index 6b65096..b825c80 100644 >> --- a/drivers/gpu/drm/i915/intel_bios.c >> +++ b/drivers/gpu/drm/i915/intel_bios.c >> @@ -1059,6 +1059,20 @@ parse_device_mapping(struct drm_i915_private *dev_priv, >> /* skip the device block if device type is invalid */ >> continue; >> } >> + >> +#define MIPI_PORT_A 0x15 >> +#define MIPI_PORT_B 0x16 >> +#define MIPI_PORT_C 0x17 >> +#define MIPI_PORT_D 0x18 > > These should go with the other valid defines for dvo_port in > intel_bios.h and be renamed to match, eg. DEVICE_PORT_MIPI_A. Will do > > Also this is not what I have in my copy of the VBT documentation, I have > (in decimal) MIPI PORT_A = 11, .., PORT_D = 14 What I have say 21..24. Again this is the problem we need to fix. > >> + if (p_child->common.dvo_port >= MIPI_PORT_A && p_child->common.dvo_port <= MIPI_PORT_D) { >> + /* check the device type and confirm its MIPI */ >> + if (p_child->common.device_type & DEVICE_TYPE_MIPI_OUTPUT) { >> + DRM_DEBUG_KMS("Found MIPI as LFP\n"); >> + dev_priv->vbt.is_mipi = 1; >> + dev_priv->vbt.dsi.port = p_child->common.dvo_port; >> + } >> + } >> + >> child_dev_ptr = dev_priv->vbt.child_dev + count; >> count++; >> memcpy((void *)child_dev_ptr, (void *)p_child, >> @@ -1230,7 +1244,11 @@ intel_parse_bios(struct drm_device *dev) >> parse_device_mapping(dev_priv, bdb); >> parse_driver_features(dev_priv, bdb); >> parse_edp(dev_priv, bdb); >> - parse_mipi(dev_priv, bdb); >> + >> + /* parse MIPI blocks only if LFP type is MIPI */ >> + if (dev_priv->vbt.is_mipi) >> + parse_mipi(dev_priv, bdb); > > Let's put that check inside parse_mipi so we don't break the symetry of > those parse_* calls. Will do. > >> + >> parse_ddi_ports(dev_priv, bdb); >> >> if (bios) >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 3da73ef..a55fa41 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -11151,7 +11151,9 @@ static void intel_setup_outputs(struct drm_device *dev) >> } >> } >> >> - intel_dsi_init(dev); >> + /* There is no detection method for MIPI so rely on VBT */ >> + if (dev_priv->vbt.is_mipi) >> + intel_dsi_init(dev); > > Can we have that check inside intel_dsi_init() so we don't have to do it > for every platform with DSI? Will do regards Shobhit > >> } else if (SUPPORTS_DIGITAL_OUTPUTS(dev)) { >> bool found = false; >> >> -- >> 1.8.3.2 >>