| Message ID | 1400861368-11691-1-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <shobhit.kumar@intel.com> > --- > 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. > struct { > + u16 port; We store the port but never use it, is it needed? > 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. 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 > + 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. > + > 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? > } else if (SUPPORTS_DIGITAL_OUTPUTS(dev)) { > bool found = false; > > -- > 1.8.3.2 >
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 <shobhit.kumar@intel.com> Will the mipi patch I've just merged without this one here break machines? If so we need to get this one here into shape asap, for otherwise I need to kick out the mipi patch again. Which we really don't want. -Daniel > --- > 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; > struct { > + u16 port; > 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 > + 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); > + > 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); > } else if (SUPPORTS_DIGITAL_OUTPUTS(dev)) { > bool found = false; > > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 5/27/2014 5:10 PM, Daniel Vetter 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 <shobhit.kumar@intel.com> > > Will the mipi patch I've just merged without this one here break machines? > If so we need to get this one here into shape asap, for otherwise I need > to kick out the mipi patch again. Which we really don't want. Yes it will break. I am working on this patch right now Regards Shobhit > -Daniel > >> --- >> 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; >> struct { >> + u16 port; >> 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 >> + 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); >> + >> 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); >> } else if (SUPPORTS_DIGITAL_OUTPUTS(dev)) { >> bool found = false; >> >> -- >> 1.8.3.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
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 <shobhit.kumar@intel.com> >> --- >> 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 >>
On Tue, May 27, 2014 at 05:40:23PM +0530, Kumar, Shobhit wrote: > >> 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. Sure, no harm there. > >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. Mind sending me your version?
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; struct { + u16 port; 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 + 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); + 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); } else if (SUPPORTS_DIGITAL_OUTPUTS(dev)) { bool found = false;
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 <shobhit.kumar@intel.com> --- 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(-)