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@xxxxxxxxx> 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@xxxxxxxxx>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
