| Message ID | 1401199439-22703-1-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sorry to be such a bore but: On Tue, May 27, 2014 at 07:33:59PM +0530, Shobhit Kumar wrote: > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -660,6 +660,10 @@ bool intel_dsi_init(struct drm_device *dev) > > DRM_DEBUG_KMS("\n"); > > + /* There is no detection method for MIPI so rely on VBT */ > + if (!dev_priv->vbt.has_mipi) > + return false; > + Huum, if we can intel_dsi_init() on VLV, but we don't have a MIPI panel, shouldn't return true here? ie. "intel_dsi_init() was successful, we just don't have a MIPI panel.
On Tue, May 27, 2014 at 03:17:15PM +0100, Damien Lespiau wrote: > Sorry to be such a bore but: > > On Tue, May 27, 2014 at 07:33:59PM +0530, Shobhit Kumar wrote: > > --- a/drivers/gpu/drm/i915/intel_dsi.c > > +++ b/drivers/gpu/drm/i915/intel_dsi.c > > @@ -660,6 +660,10 @@ bool intel_dsi_init(struct drm_device *dev) > > > > DRM_DEBUG_KMS("\n"); > > > > + /* There is no detection method for MIPI so rely on VBT */ > > + if (!dev_priv->vbt.has_mipi) > > + return false; > > + > > Huum, if we can intel_dsi_init() on VLV, but we don't have a MIPI panel, > shouldn't return true here? ie. "intel_dsi_init() was successful, we > just don't have a MIPI panel. Why does it even have a return value? Either it added the connector and encoder or it didn't.
On 5/27/2014 7:47 PM, Damien Lespiau wrote: > Sorry to be such a bore but: > > On Tue, May 27, 2014 at 07:33:59PM +0530, Shobhit Kumar wrote: >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> @@ -660,6 +660,10 @@ bool intel_dsi_init(struct drm_device *dev) >> >> DRM_DEBUG_KMS("\n"); >> >> + /* There is no detection method for MIPI so rely on VBT */ >> + if (!dev_priv->vbt.has_mipi) >> + return false; >> + > > Huum, if we can intel_dsi_init() on VLV, but we don't have a MIPI panel, > shouldn't return true here? ie. "intel_dsi_init() was successful, we > just don't have a MIPI panel. This check just determines that the design has probably eDP and not MIPI panel attached. Assuming of course that on any design will have either eDP or MIPI as LFP. So I was checking in terms of the OEM design and not platform capability to have DSI. In fact even in intel_dp_init when in edp_init_connector fails to read DPCD we return false. In fact why we really need to have a return value when we don't even check it and for example intel_dp_init is void intel_dp_init Regards Shobhit
On Tue, May 27, 2014 at 05:31:42PM +0300, Ville Syrjälä wrote: > On Tue, May 27, 2014 at 03:17:15PM +0100, Damien Lespiau wrote: > > Sorry to be such a bore but: > > > > On Tue, May 27, 2014 at 07:33:59PM +0530, Shobhit Kumar wrote: > > > --- a/drivers/gpu/drm/i915/intel_dsi.c > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c > > > @@ -660,6 +660,10 @@ bool intel_dsi_init(struct drm_device *dev) > > > > > > DRM_DEBUG_KMS("\n"); > > > > > > + /* There is no detection method for MIPI so rely on VBT */ > > > + if (!dev_priv->vbt.has_mipi) > > > + return false; > > > + > > > > Huum, if we can intel_dsi_init() on VLV, but we don't have a MIPI panel, > > shouldn't return true here? ie. "intel_dsi_init() was successful, we > > just don't have a MIPI panel. > > Why does it even have a return value? Either it added the > connector and encoder or it didn't. That's even better. I guess we can have a patch on top once this one has landed.
On Tue, May 27, 2014 at 03:35:44PM +0100, Damien Lespiau wrote: > On Tue, May 27, 2014 at 05:31:42PM +0300, Ville Syrjälä wrote: > > On Tue, May 27, 2014 at 03:17:15PM +0100, Damien Lespiau wrote: > > > Sorry to be such a bore but: > > > > > > On Tue, May 27, 2014 at 07:33:59PM +0530, Shobhit Kumar wrote: > > > > --- a/drivers/gpu/drm/i915/intel_dsi.c > > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c > > > > @@ -660,6 +660,10 @@ bool intel_dsi_init(struct drm_device *dev) > > > > > > > > DRM_DEBUG_KMS("\n"); > > > > > > > > + /* There is no detection method for MIPI so rely on VBT */ > > > > + if (!dev_priv->vbt.has_mipi) > > > > + return false; > > > > + > > > > > > Huum, if we can intel_dsi_init() on VLV, but we don't have a MIPI panel, > > > shouldn't return true here? ie. "intel_dsi_init() was successful, we > > > just don't have a MIPI panel. > > > > Why does it even have a return value? Either it added the > > connector and encoder or it didn't. > > That's even better. I guess we can have a patch on top once this one has > landed. dp_init has this since edp init can fail and that decides what we will do with it occasionally. But that's about it wrt reasons for a return value for a encoder init function. Please remove. -Daniel
On Tue, May 27, 2014 at 07:33:59PM +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. > > v2: Addressed review comments by Damien > - Moved PORT definitions to intel_bios.h and renamed as DVO_PORT_MIPIA > - renamed is_mipi to has_mipi and moved definition as suggested > - Check has_mipi inside parse_mipi and intel_dsi_init insted of outside > > v3: Make has_mipi as a bitfield as suggested > > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_bios.c | 14 ++++++++++++++ > drivers/gpu/drm/i915/intel_bios.h | 4 ++++ > drivers/gpu/drm/i915/intel_dsi.c | 4 ++++ > 4 files changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 9ef6712..ef7ab0a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1208,6 +1208,7 @@ struct intel_vbt_data { > unsigned int lvds_use_ssc:1; > unsigned int display_clock_mode:1; > unsigned int fdi_rx_polarity_inverted:1; > + unsigned int has_mipi:1; > int lvds_ssc_freq; > unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */ > > @@ -1231,6 +1232,7 @@ struct intel_vbt_data { > > /* MIPI DSI */ > 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..6f6b6c6 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -744,6 +744,10 @@ parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb) > int i, panel_id, seq_size; > u16 block_size; > > + /* parse MIPI blocks only if LFP type is MIPI */ > + if (!dev_priv->vbt.has_mipi) > + return; > + > /* Initialize this to undefined indicating no generic MIPI support */ > dev_priv->vbt.dsi.panel_id = MIPI_DSI_UNDEFINED_PANEL_ID; > > @@ -1059,6 +1063,16 @@ parse_device_mapping(struct drm_i915_private *dev_priv, > /* skip the device block if device type is invalid */ > continue; > } > + > + if (p_child->common.dvo_port >= DVO_PORT_MIPIA && p_child->common.dvo_port <= DVO_PORT_MIPID) { checkpatch thinks you should break this line. I agree. And folding in the nested if will make everything neatly fit below 80 chars, so I've done that. Queued for -next, thanks for the patch. -Daniel > + /* 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.has_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, > diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h > index 6009deb..b986677 100644 > --- a/drivers/gpu/drm/i915/intel_bios.h > +++ b/drivers/gpu/drm/i915/intel_bios.h > @@ -743,6 +743,10 @@ int intel_parse_bios(struct drm_device *dev); > #define DVO_PORT_DPC 8 > #define DVO_PORT_DPD 9 > #define DVO_PORT_DPA 10 > +#define DVO_PORT_MIPIA 21 > +#define DVO_PORT_MIPIB 22 > +#define DVO_PORT_MIPIC 23 > +#define DVO_PORT_MIPID 24 > > /* Block 52 contains MIPI Panel info > * 6 such enteries will there. Index into correct > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index e73bec6..944a421 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -660,6 +660,10 @@ bool intel_dsi_init(struct drm_device *dev) > > DRM_DEBUG_KMS("\n"); > > + /* There is no detection method for MIPI so rely on VBT */ > + if (!dev_priv->vbt.has_mipi) > + return false; > + > intel_dsi = kzalloc(sizeof(*intel_dsi), GFP_KERNEL); > if (!intel_dsi) > return false; > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9ef6712..ef7ab0a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1208,6 +1208,7 @@ struct intel_vbt_data { unsigned int lvds_use_ssc:1; unsigned int display_clock_mode:1; unsigned int fdi_rx_polarity_inverted:1; + unsigned int has_mipi:1; int lvds_ssc_freq; unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */ @@ -1231,6 +1232,7 @@ struct intel_vbt_data { /* MIPI DSI */ 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..6f6b6c6 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -744,6 +744,10 @@ parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb) int i, panel_id, seq_size; u16 block_size; + /* parse MIPI blocks only if LFP type is MIPI */ + if (!dev_priv->vbt.has_mipi) + return; + /* Initialize this to undefined indicating no generic MIPI support */ dev_priv->vbt.dsi.panel_id = MIPI_DSI_UNDEFINED_PANEL_ID; @@ -1059,6 +1063,16 @@ parse_device_mapping(struct drm_i915_private *dev_priv, /* skip the device block if device type is invalid */ continue; } + + if (p_child->common.dvo_port >= DVO_PORT_MIPIA && p_child->common.dvo_port <= DVO_PORT_MIPID) { + /* 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.has_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, diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h index 6009deb..b986677 100644 --- a/drivers/gpu/drm/i915/intel_bios.h +++ b/drivers/gpu/drm/i915/intel_bios.h @@ -743,6 +743,10 @@ int intel_parse_bios(struct drm_device *dev); #define DVO_PORT_DPC 8 #define DVO_PORT_DPD 9 #define DVO_PORT_DPA 10 +#define DVO_PORT_MIPIA 21 +#define DVO_PORT_MIPIB 22 +#define DVO_PORT_MIPIC 23 +#define DVO_PORT_MIPID 24 /* Block 52 contains MIPI Panel info * 6 such enteries will there. Index into correct diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index e73bec6..944a421 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -660,6 +660,10 @@ bool intel_dsi_init(struct drm_device *dev) DRM_DEBUG_KMS("\n"); + /* There is no detection method for MIPI so rely on VBT */ + if (!dev_priv->vbt.has_mipi) + return false; + intel_dsi = kzalloc(sizeof(*intel_dsi), GFP_KERNEL); if (!intel_dsi) return 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. v2: Addressed review comments by Damien - Moved PORT definitions to intel_bios.h and renamed as DVO_PORT_MIPIA - renamed is_mipi to has_mipi and moved definition as suggested - Check has_mipi inside parse_mipi and intel_dsi_init insted of outside v3: Make has_mipi as a bitfield as suggested Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_bios.c | 14 ++++++++++++++ drivers/gpu/drm/i915/intel_bios.h | 4 ++++ drivers/gpu/drm/i915/intel_dsi.c | 4 ++++ 4 files changed, 24 insertions(+)