| Message ID | 1458656708-31228-2-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 22 Mar 2016, Shashank Sharma <shashank.sharma@intel.com> wrote: > LSPCON can be configured on a port using VBT entry. > This patch adds code to parse VBT and detect presence of > LSPCON for a ddi port. > > Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_bios.c | 42 +++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_vbt_defs.h | 1 + > 3 files changed, 44 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index f330a53..cbd40de 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3373,6 +3373,7 @@ bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv); > bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin); > bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port); > bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port); > +bool intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port); > > /* intel_opregion.c */ > #ifdef CONFIG_ACPI > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > index 083003b..a04ab5c 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -1588,3 +1588,45 @@ bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, > > return false; > } > + Please add kernel-doc comment like all the other intel_bios_is_* functions have. > +bool > +intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port) You mean is *present*? Please pass struct drm_i915_private *dev_priv instead of dev. > +{ > + unsigned char i; > + enum port dvo_port = 0; > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + /* LSPCON is supported only for GEN 9 */ > + if (!IS_GEN9(dev)) > + return false; > + > + /* Check if lspcon is supported in VBT */ > + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { > + if (!dev_priv->vbt.child_dev[i].common.lspcon) I think to be defensive we should also check for bdb version >= 192. That we could check before the loop along with the gen check. > + continue; > + > + switch (dev_priv->vbt.child_dev[i].common.dvo_port) { > + case DVO_PORT_DPB: > + dvo_port = PORT_B; > + break; > + > + case DVO_PORT_DPC: > + dvo_port = PORT_C; > + break; > + > + case DVO_PORT_DPD: > + dvo_port = PORT_D; > + break; > + > + default: > + continue; > + } > + > + if (dvo_port == port) { > + DRM_DEBUG_DRIVER("LSPCON configured on port %c\n", > + port_name(port)); > + return true; > + } > + } > + return false; > +} > diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h > index 749dcea..0066b24 100644 > --- a/drivers/gpu/drm/i915/intel_vbt_defs.h > +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h > @@ -276,6 +276,7 @@ struct common_child_dev_config { > u8 flags_1; > u8 not_common3[13]; > u8 iboost_level; > + u8 lspcon:1; Huh? AFAICT from the spec, lspcon is bit 2 in flags_1. You could define flags_1 in terms of bitfields, including IBOOST_ENABLE. > } __packed;
Thanks for the review, Jani. Regards Shashank On 3/22/2016 9:24 PM, Jani Nikula wrote: > On Tue, 22 Mar 2016, Shashank Sharma <shashank.sharma@intel.com> wrote: >> LSPCON can be configured on a port using VBT entry. >> This patch adds code to parse VBT and detect presence of >> LSPCON for a ddi port. >> >> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_bios.c | 42 +++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_vbt_defs.h | 1 + >> 3 files changed, 44 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index f330a53..cbd40de 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -3373,6 +3373,7 @@ bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv); >> bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin); >> bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port); >> bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port); >> +bool intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port); >> >> /* intel_opregion.c */ >> #ifdef CONFIG_ACPI >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c >> index 083003b..a04ab5c 100644 >> --- a/drivers/gpu/drm/i915/intel_bios.c >> +++ b/drivers/gpu/drm/i915/intel_bios.c >> @@ -1588,3 +1588,45 @@ bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, >> >> return false; >> } >> + > > Please add kernel-doc comment like all the other intel_bios_is_* > functions have. > Ok, >> +bool >> +intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port) > > You mean is *present*? > Well, this is embarrassing :) yes it was supposed to be present. > Please pass struct drm_i915_private *dev_priv instead of dev. > Ok >> +{ >> + unsigned char i; >> + enum port dvo_port = 0; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + >> + /* LSPCON is supported only for GEN 9 */ >> + if (!IS_GEN9(dev)) >> + return false; >> + >> + /* Check if lspcon is supported in VBT */ >> + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { >> + if (!dev_priv->vbt.child_dev[i].common.lspcon) > > I think to be defensive we should also check for bdb version >= > 192. That we could check before the loop along with the gen check. > Yes, this can be done. >> + continue; >> + >> + switch (dev_priv->vbt.child_dev[i].common.dvo_port) { >> + case DVO_PORT_DPB: >> + dvo_port = PORT_B; >> + break; >> + >> + case DVO_PORT_DPC: >> + dvo_port = PORT_C; >> + break; >> + >> + case DVO_PORT_DPD: >> + dvo_port = PORT_D; >> + break; >> + >> + default: >> + continue; >> + } >> + >> + if (dvo_port == port) { >> + DRM_DEBUG_DRIVER("LSPCON configured on port %c\n", >> + port_name(port)); >> + return true; >> + } >> + } >> + return false; >> +} >> diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h >> index 749dcea..0066b24 100644 >> --- a/drivers/gpu/drm/i915/intel_vbt_defs.h >> +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h >> @@ -276,6 +276,7 @@ struct common_child_dev_config { >> u8 flags_1; >> u8 not_common3[13]; >> u8 iboost_level; >> + u8 lspcon:1; > > Huh? AFAICT from the spec, lspcon is bit 2 in flags_1. You could define > flags_1 in terms of bitfields, including IBOOST_ENABLE. > I am not sure about this, let me go to VBT spec again, if there is a recent change. Will update you on this. >> } __packed; >
On Tue, Mar 22, 2016 at 10:20:33PM +0530, Sharma, Shashank wrote: > Thanks for the review, Jani. > > Regards > Shashank > > On 3/22/2016 9:24 PM, Jani Nikula wrote: > > On Tue, 22 Mar 2016, Shashank Sharma <shashank.sharma@intel.com> wrote: > >> LSPCON can be configured on a port using VBT entry. > >> This patch adds code to parse VBT and detect presence of > >> LSPCON for a ddi port. > >> > >> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> > >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_drv.h | 1 + > >> drivers/gpu/drm/i915/intel_bios.c | 42 +++++++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_vbt_defs.h | 1 + > >> 3 files changed, 44 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >> index f330a53..cbd40de 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.h > >> +++ b/drivers/gpu/drm/i915/i915_drv.h > >> @@ -3373,6 +3373,7 @@ bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv); > >> bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin); > >> bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port); > >> bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port); > >> +bool intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port); > >> > >> /* intel_opregion.c */ > >> #ifdef CONFIG_ACPI > >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > >> index 083003b..a04ab5c 100644 > >> --- a/drivers/gpu/drm/i915/intel_bios.c > >> +++ b/drivers/gpu/drm/i915/intel_bios.c > >> @@ -1588,3 +1588,45 @@ bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, > >> > >> return false; > >> } > >> + > > > > Please add kernel-doc comment like all the other intel_bios_is_* > > functions have. > > > Ok, > >> +bool > >> +intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port) > > > > You mean is *present*? > > > Well, this is embarrassing :) yes it was supposed to be present. > > Please pass struct drm_i915_private *dev_priv instead of dev. > > > Ok > >> +{ > >> + unsigned char i; > >> + enum port dvo_port = 0; > >> + struct drm_i915_private *dev_priv = dev->dev_private; > >> + > >> + /* LSPCON is supported only for GEN 9 */ > >> + if (!IS_GEN9(dev)) > >> + return false; > >> + > >> + /* Check if lspcon is supported in VBT */ > >> + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { > >> + if (!dev_priv->vbt.child_dev[i].common.lspcon) > > > > I think to be defensive we should also check for bdb version >= > > 192. That we could check before the loop along with the gen check. > > > Yes, this can be done. > >> + continue; > >> + > >> + switch (dev_priv->vbt.child_dev[i].common.dvo_port) { > >> + case DVO_PORT_DPB: > >> + dvo_port = PORT_B; > >> + break; > >> + > >> + case DVO_PORT_DPC: > >> + dvo_port = PORT_C; > >> + break; > >> + > >> + case DVO_PORT_DPD: > >> + dvo_port = PORT_D; > >> + break; > >> + > >> + default: > >> + continue; > >> + } > >> + > >> + if (dvo_port == port) { > >> + DRM_DEBUG_DRIVER("LSPCON configured on port %c\n", > >> + port_name(port)); > >> + return true; > >> + } > >> + } > >> + return false; > >> +} > >> diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h > >> index 749dcea..0066b24 100644 > >> --- a/drivers/gpu/drm/i915/intel_vbt_defs.h > >> +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h > >> @@ -276,6 +276,7 @@ struct common_child_dev_config { > >> u8 flags_1; > >> u8 not_common3[13]; > >> u8 iboost_level; > >> + u8 lspcon:1; > > > > Huh? AFAICT from the spec, lspcon is bit 2 in flags_1. You could define > > flags_1 in terms of bitfields, including IBOOST_ENABLE. > > > I am not sure about this, let me go to VBT spec again, if there is a > recent change. Will update you on this. There was a patch [1] posted recently which added the lspcon bit and others, and that at least matches the spec at the time when I looked at it (or I made a mistake). The patch just needs a rebase and it should be good to go I think, though someone else double checking wouldn't hurt. [1] https://lists.freedesktop.org/archives/intel-gfx/2016-March/089759.html > >> } __packed; > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Regards Shashank On 3/22/2016 10:35 PM, Ville Syrjälä wrote: > On Tue, Mar 22, 2016 at 10:20:33PM +0530, Sharma, Shashank wrote: >> Thanks for the review, Jani. >> >> Regards >> Shashank >> >> On 3/22/2016 9:24 PM, Jani Nikula wrote: >>> On Tue, 22 Mar 2016, Shashank Sharma <shashank.sharma@intel.com> wrote: >>>> LSPCON can be configured on a port using VBT entry. >>>> This patch adds code to parse VBT and detect presence of >>>> LSPCON for a ddi port. >>>> >>>> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_drv.h | 1 + >>>> drivers/gpu/drm/i915/intel_bios.c | 42 +++++++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/i915/intel_vbt_defs.h | 1 + >>>> 3 files changed, 44 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>>> index f330a53..cbd40de 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -3373,6 +3373,7 @@ bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv); >>>> bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin); >>>> bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port); >>>> bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port); >>>> +bool intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port); >>>> >>>> /* intel_opregion.c */ >>>> #ifdef CONFIG_ACPI >>>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c >>>> index 083003b..a04ab5c 100644 >>>> --- a/drivers/gpu/drm/i915/intel_bios.c >>>> +++ b/drivers/gpu/drm/i915/intel_bios.c >>>> @@ -1588,3 +1588,45 @@ bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, >>>> >>>> return false; >>>> } >>>> + >>> >>> Please add kernel-doc comment like all the other intel_bios_is_* >>> functions have. >>> >> Ok, >>>> +bool >>>> +intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port) >>> >>> You mean is *present*? >>> >> Well, this is embarrassing :) yes it was supposed to be present. >>> Please pass struct drm_i915_private *dev_priv instead of dev. >>> >> Ok >>>> +{ >>>> + unsigned char i; >>>> + enum port dvo_port = 0; >>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>> + >>>> + /* LSPCON is supported only for GEN 9 */ >>>> + if (!IS_GEN9(dev)) >>>> + return false; >>>> + >>>> + /* Check if lspcon is supported in VBT */ >>>> + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { >>>> + if (!dev_priv->vbt.child_dev[i].common.lspcon) >>> >>> I think to be defensive we should also check for bdb version >= >>> 192. That we could check before the loop along with the gen check. >>> >> Yes, this can be done. >>>> + continue; >>>> + >>>> + switch (dev_priv->vbt.child_dev[i].common.dvo_port) { >>>> + case DVO_PORT_DPB: >>>> + dvo_port = PORT_B; >>>> + break; >>>> + >>>> + case DVO_PORT_DPC: >>>> + dvo_port = PORT_C; >>>> + break; >>>> + >>>> + case DVO_PORT_DPD: >>>> + dvo_port = PORT_D; >>>> + break; >>>> + >>>> + default: >>>> + continue; >>>> + } >>>> + >>>> + if (dvo_port == port) { >>>> + DRM_DEBUG_DRIVER("LSPCON configured on port %c\n", >>>> + port_name(port)); >>>> + return true; >>>> + } >>>> + } >>>> + return false; >>>> +} >>>> diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h >>>> index 749dcea..0066b24 100644 >>>> --- a/drivers/gpu/drm/i915/intel_vbt_defs.h >>>> +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h >>>> @@ -276,6 +276,7 @@ struct common_child_dev_config { >>>> u8 flags_1; >>>> u8 not_common3[13]; >>>> u8 iboost_level; >>>> + u8 lspcon:1; >>> >>> Huh? AFAICT from the spec, lspcon is bit 2 in flags_1. You could define >>> flags_1 in terms of bitfields, including IBOOST_ENABLE. >>> >> I am not sure about this, let me go to VBT spec again, if there is a >> recent change. Will update you on this. > > There was a patch [1] posted recently which added the lspcon bit and others, > and that at least matches the spec at the time when I looked at it (or I > made a mistake). The patch just needs a rebase and it should be good to > go I think, though someone else double checking wouldn't hurt. > > [1] https://lists.freedesktop.org/archives/intel-gfx/2016-March/089759.html > Thanks for pointing this out. I almost forgot about this patch, which Durga/Siva added. Will sync up on this patch first, and if a rebase is the only thing required, we can proceed on this one. > >>>> } __packed; >>> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://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 f330a53..cbd40de 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3373,6 +3373,7 @@ bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv); bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin); bool intel_bios_is_port_edp(struct drm_i915_private *dev_priv, enum port port); bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, enum port *port); +bool intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port); /* intel_opregion.c */ #ifdef CONFIG_ACPI diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 083003b..a04ab5c 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1588,3 +1588,45 @@ bool intel_bios_is_dsi_present(struct drm_i915_private *dev_priv, return false; } + +bool +intel_bios_is_lspcon_preset(struct drm_device *dev, enum port port) +{ + unsigned char i; + enum port dvo_port = 0; + struct drm_i915_private *dev_priv = dev->dev_private; + + /* LSPCON is supported only for GEN 9 */ + if (!IS_GEN9(dev)) + return false; + + /* Check if lspcon is supported in VBT */ + for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { + if (!dev_priv->vbt.child_dev[i].common.lspcon) + continue; + + switch (dev_priv->vbt.child_dev[i].common.dvo_port) { + case DVO_PORT_DPB: + dvo_port = PORT_B; + break; + + case DVO_PORT_DPC: + dvo_port = PORT_C; + break; + + case DVO_PORT_DPD: + dvo_port = PORT_D; + break; + + default: + continue; + } + + if (dvo_port == port) { + DRM_DEBUG_DRIVER("LSPCON configured on port %c\n", + port_name(port)); + return true; + } + } + return false; +} diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h index 749dcea..0066b24 100644 --- a/drivers/gpu/drm/i915/intel_vbt_defs.h +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h @@ -276,6 +276,7 @@ struct common_child_dev_config { u8 flags_1; u8 not_common3[13]; u8 iboost_level; + u8 lspcon:1; } __packed;