[Intel-gfx] [PATCH 1/2] drm/i915: Parse DSI backlight/cabc ports.

[Jani Nikula]

On Tue, 10 Oct 2017, "Chauhan, Madhav" <madhav.chauhan at intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani
>> Sent: Tuesday, October 10, 2017 12:47 PM
>> To: Chauhan, Madhav <madhav.chauhan at intel.com>; intel-
>> gfx at lists.freedesktop.org
>> Cc: Hiremath, Shashidhar <shashidhar.hiremath at intel.com>; Shankar, Uma
>> <uma.shankar at intel.com>; Chauhan, Madhav
>> <madhav.chauhan at intel.com>
>> Subject: Re: [PATCH 1/2] drm/i915: Parse DSI backlight/cabc ports.
>> 
>> On Tue, 03 Oct 2017, Madhav Chauhan <madhav.chauhan at intel.com>
>> 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 at intel.com>
>> > ---
>> >  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.

>
>> 
>> > +		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