> -----Original Message-----
> From: Nikula, Jani
> Sent: Tuesday, October 10, 2017 11:27 PM
> To: Chauhan, Madhav <madhav.chauhan@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Hiremath, Shashidhar <shashidhar.hiremath@xxxxxxxxx>; Shankar, Uma
> <uma.shankar@xxxxxxxxx>
> Subject: RE: [PATCH 1/2] drm/i915: Parse DSI backlight/cabc ports.
>
> On Tue, 10 Oct 2017, "Chauhan, Madhav" <madhav.chauhan@xxxxxxxxx>
> wrote:
> >> -----Original Message-----
> >> From: Nikula, Jani
> >> Sent: Tuesday, October 10, 2017 12:47 PM
> >> To: Chauhan, Madhav <madhav.chauhan@xxxxxxxxx>; intel-
> >> gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: Hiremath, Shashidhar <shashidhar.hiremath@xxxxxxxxx>; Shankar,
> >> Uma <uma.shankar@xxxxxxxxx>; Chauhan, Madhav
> >> <madhav.chauhan@xxxxxxxxx>
> >> Subject: Re: [PATCH 1/2] drm/i915: Parse DSI backlight/cabc ports.
> >>
> >> On Tue, 03 Oct 2017, Madhav Chauhan <madhav.chauhan@xxxxxxxxx>
> >> wrote:
> >> > This patch parse DSI backlight/cabc ports info from VBT and save
> >> > them inside local strucutre. This saved info can be directly used
> >> > while initializing DSI for different platforms instead of parsing
> >> > for each platform.
> >> >
> >> > Signed-off-by: Madhav Chauhan <madhav.chauhan@xxxxxxxxx>
> >> > ---
> >> > drivers/gpu/drm/i915/i915_drv.h | 2 ++
> >> > drivers/gpu/drm/i915/intel_bios.c | 63
> >> > ++++++++++++++++++++++++++++++++-------
> >> > 2 files changed, 55 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> > b/drivers/gpu/drm/i915/i915_drv.h index b7cba89..fc472bb 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -1751,6 +1751,8 @@ struct intel_vbt_data {
> >> > u8 seq_version;
> >> > u32 size;
> >> > u8 *data;
> >> > + u16 bl_ports;
> >> > + u16 cabc_ports;
> >>
> >> This is right in the middle of the sequence data. Please move up e.g.
> >> between pps and seq_version.
> >
> > Ok.
> >
> >>
> >> > const u8 *sequence[MIPI_SEQ_MAX];
> >> > } dsi;
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_bios.c
> >> > b/drivers/gpu/drm/i915/intel_bios.c
> >> > index 3747d8d..88a72cc 100644
> >> > --- a/drivers/gpu/drm/i915/intel_bios.c
> >> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> > @@ -730,6 +730,56 @@ parse_psr(struct drm_i915_private *dev_priv,
> >> const struct bdb_header *bdb)
> >> > dev_priv->vbt.psr.tp2_tp3_wakeup_time =
> >> > psr_table->tp2_tp3_wakeup_time; }
> >> >
> >> > +static void parse_dsi_backlight_ports(struct drm_i915_private
> *dev_priv,
> >> > + u16 version, enum port port) {
> >> > + if (dev_priv->vbt.dsi.config->dual_link && version < 197) {
> >> > + /*
> >> > + * These fields are introduced from the VBT version 197
> >> onwards,
> >> > + * so making sure that these bits are set zero in the previous
> >> > + * versions.
> >> > + */
> >> > + dev_priv->vbt.dsi.config->dl_dcs_cabc_ports = 0;
> >> > + dev_priv->vbt.dsi.config->dl_dcs_backlight_ports = 0;
> >>
> >> You could remove this in patch 2. Nobody should be looking at it
> anymore.
> >
> > Patch 2 means?? Next version of this patch or patch 2 of this series.
> > Patch 2 of this series doesn't use these variables. Please clarify.
>
> After patch 2 of this series, the initialization of these two fields is
> unnecessary, so please remove the above lines in patch 2.
Ok. Thanks!!
Regards,
Madhav
>
> >
> >>
> >> > + dev_priv->vbt.dsi.bl_ports = 0;
> >> > + dev_priv->vbt.dsi.cabc_ports = 0;
> >>
> >> This you don't have to do anyway, it's all zeros by default.
> >
> > Ok.
> >
> >>
> >> > + return;
> >> > + } else if (dev_priv->vbt.dsi.config->dual_link) {
> >> > + switch (dev_priv->vbt.dsi.config->dl_dcs_backlight_ports) {
> >> > + case DL_DCS_PORT_A:
> >> > + dev_priv->vbt.dsi.bl_ports = BIT(PORT_A);
> >> > + break;
> >> > + case DL_DCS_PORT_C:
> >> > + dev_priv->vbt.dsi.bl_ports = BIT(PORT_C);
> >> > + break;
> >> > + default:
> >> > + case DL_DCS_PORT_A_AND_C:
> >> > + dev_priv->vbt.dsi.bl_ports = BIT(PORT_A) |
> >> BIT(PORT_C);
> >> > + break;
> >> > + }
> >> > +
> >> > + switch (dev_priv->vbt.dsi.config->dl_dcs_cabc_ports) {
> >> > + case DL_DCS_PORT_A:
> >> > + dev_priv->vbt.dsi.cabc_ports = BIT(PORT_A);
> >> > + break;
> >> > + case DL_DCS_PORT_C:
> >> > + dev_priv->vbt.dsi.cabc_ports = BIT(PORT_C);
> >> > + break;
> >> > + default:
> >> > + case DL_DCS_PORT_A_AND_C:
> >> > + dev_priv->vbt.dsi.cabc_ports =
> >> > + BIT(PORT_A) | BIT(PORT_C);
> >> > + break;
> >> > + }
> >> > + } else {
> >> > + dev_priv->vbt.dsi.bl_ports = BIT(port);
> >> > + dev_priv->vbt.dsi.cabc_ports = BIT(port);
> >> > + }
> >> > +
> >> > + if (!dev_priv->vbt.dsi.config->cabc_supported)
> >> > + dev_priv->vbt.dsi.cabc_ports = 0;
> >>
> >> Would seem reasonable to not initalize it in the first place if it's
> >> not supported.
> >
> > Agree. How about putting a check condition !cabc_supported before
> > parsing dl_dcs_cabc_ports in switch case??
> >
> >>If you do a series of early returns starting with the bdb version
> >>check, then !dual_link, then !cabc_supported, it might be easiest.
> >
> > But even if for !dual_link, still we need to fill newly added cabc_ports and
> bl_ports.
> >
> > Regards,
> > Madhav
> >
> >>
> >> > +}
> >> > +
> >> > static void
> >> > parse_mipi_config(struct drm_i915_private *dev_priv,
> >> > const struct bdb_header *bdb)
> >> > @@ -738,9 +788,10 @@ parse_mipi_config(struct drm_i915_private
> >> *dev_priv,
> >> > const struct mipi_config *config;
> >> > const struct mipi_pps_data *pps;
> >> > int panel_type = dev_priv->vbt.panel_type;
> >> > + enum port port;
> >> >
> >> > /* parse MIPI blocks only if LFP type is MIPI */
> >> > - if (!intel_bios_is_dsi_present(dev_priv, NULL))
> >> > + if (!intel_bios_is_dsi_present(dev_priv, &port))
> >> > return;
> >> >
> >> > /* Initialize this to undefined indicating no generic MIPI
> >> > support */ @@ -781,15 +832,7 @@ parse_mipi_config(struct
> >> > drm_i915_private
> >> *dev_priv,
> >> > return;
> >> > }
> >> >
> >> > - /*
> >> > - * These fields are introduced from the VBT version 197 onwards,
> >> > - * so making sure that these bits are set zero in the previous
> >> > - * versions.
> >> > - */
> >> > - if (dev_priv->vbt.dsi.config->dual_link && bdb->version < 197) {
> >> > - dev_priv->vbt.dsi.config->dl_dcs_cabc_ports = 0;
> >> > - dev_priv->vbt.dsi.config->dl_dcs_backlight_ports = 0;
> >> > - }
> >> > + parse_dsi_backlight_ports(dev_priv, bdb->version, port);
> >> >
> >> > /* We have mandatory mipi config blocks. Initialize as generic
> >> > panel
> >> */
> >> > dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
>
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
