Re: [PATCH 04/12] drm/i915/bxt: DSI prepare changes for BXT

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 22 May 2015, Uma Shankar <uma.shankar@xxxxxxxxx> wrote:
> From: Shashank Sharma <shashank.sharma@xxxxxxxxx>
>
> This patch modifies dsi_prepare() function to support the same
> modeset prepare sequence for BXT also. Main changes are:
> 1. BXT port control register is different than VLV.
> 2. BXT modeset sequence needs vdisplay and hdisplay programmed
>    for transcoder.
> 3. BXT can select PIPE for MIPI transcoders.
> 4. BXT needs to program register MIPI_INIT_COUNT for both the ports,
>    even if only one is being used.
>
> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |   22 +++++++++++
>  drivers/gpu/drm/i915/intel_dsi.c |   79 +++++++++++++++++++++++++++++++++-----
>  2 files changed, 91 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b63bdce..57c3a48 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7387,6 +7387,22 @@ enum skl_disp_power_wells {
>  #define GEN9_FUSE_PG1_ENABLED                          (1 << 26)
>  #define GEN9_FUSE_PG0_ENABLED                          (1 << 27)
>  
> +/* BXT MIPI mode configure */
> +#define _BXT_MIPIA_TRANS_HACTIVE                       0x6B0F8
> +#define _BXT_MIPIC_TRANS_HACTIVE                       0x6B8F8
> +#define BXT_MIPI_TRANS_HACTIVE(tc)      _TRANSCODER(tc, \
> +		_BXT_MIPIA_TRANS_HACTIVE, _BXT_MIPIC_TRANS_HACTIVE)
> +
> +#define _BXT_MIPIA_TRANS_VACTIVE                       0x6B0FC
> +#define _BXT_MIPIC_TRANS_VACTIVE                       0x6B8FC
> +#define BXT_MIPI_TRANS_VACTIVE(tc)      _TRANSCODER(tc, \
> +		_BXT_MIPIA_TRANS_VACTIVE, _BXT_MIPIC_TRANS_VACTIVE)
> +
> +#define _BXT_MIPIA_TRANS_VTOTAL                       0x6B100
> +#define _BXT_MIPIC_TRANS_VTOTAL                       0x6B900
> +#define BXT_MIPI_TRANS_VTOTAL(tc)       _TRANSCODER(tc, \
> +		_BXT_MIPIA_TRANS_VTOTAL, _BXT_MIPIC_TRANS_VTOTAL)

Shouldn't these use _MIPI_PORT, not _TRANSCODER?

> +
>  #define _MIPI_PORT(port, a, c)	_PORT3(port, a, 0, c)	/* ports A and C only */
>  
>  #define _MIPIA_PORT_CTRL			(VLV_DISPLAY_BASE + 0x61190)
> @@ -7802,6 +7818,12 @@ enum skl_disp_power_wells {
>  #define  READ_REQUEST_PRIORITY_HIGH			(3 << 3)
>  #define  RGB_FLIP_TO_BGR				(1 << 2)
>  
> +/* BXT has a pipe select field */

It's obvious from the defines. Please put the defines in decreasing bit
shift order, i.e. these will be the first ones.

> +#define BXT_PIPE_SELECT_A                               (0 << 7)
> +#define BXT_PIPE_SELECT_B                               (1 << 7)
> +#define BXT_PIPE_SELECT_C                               (2 << 7)
> +#define BXT_PIPE_SELECT_MASK                            (7 << 7)

Two spaces after "#define". _MASK goes first.

> +
>  #define _MIPIA_DATA_ADDRESS		(dev_priv->mipi_mmio_base + 0xb108)
>  #define _MIPIC_DATA_ADDRESS		(dev_priv->mipi_mmio_base + 0xb908)
>  #define MIPI_DATA_ADDRESS(port)		_MIPI_PORT(port, _MIPIA_DATA_ADDRESS, \
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 2c0ea6f..892b518 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -726,6 +726,21 @@ static void set_dsi_timings(struct drm_encoder *encoder,
>  	hbp = txbyteclkhs(hbp, bpp, lane_count, intel_dsi->burst_mode_ratio);
>  
>  	for_each_dsi_port(port, intel_dsi->ports) {
> +		if (IS_BROXTON(dev)) {
> +			/*
> +			 * Program hdisplay and vdisplay on MIPI transcoder.
> +			 * This is different from calculated hactive and
> +			 * vactive, as they are calculated per channel basis,
> +			 * whereas these values should be based on resolution.
> +			 */
> +			I915_WRITE(BXT_MIPI_TRANS_HACTIVE(port),
> +					mode->hdisplay);
> +			I915_WRITE(BXT_MIPI_TRANS_VACTIVE(port),
> +					mode->vdisplay);
> +			I915_WRITE(BXT_MIPI_TRANS_VTOTAL(port),
> +					mode->vtotal);

With the current definition of BXT_MIPI_TRANS_* these will fail for port
C.

> +		}
> +
>  		I915_WRITE(MIPI_HACTIVE_AREA_COUNT(port), hactive);
>  		I915_WRITE(MIPI_HFP_COUNT(port), hfp);
>  
> @@ -766,16 +781,38 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder)
>  	}
>  
>  	for_each_dsi_port(port, intel_dsi->ports) {
> -		/* escape clock divider, 20MHz, shared for A and C.
> -		 * device ready must be off when doing this! txclkesc? */
> -		tmp = I915_READ(MIPI_CTRL(PORT_A));
> -		tmp &= ~ESCAPE_CLOCK_DIVIDER_MASK;
> -		I915_WRITE(MIPI_CTRL(PORT_A), tmp | ESCAPE_CLOCK_DIVIDER_1);
> -
> -		/* read request priority is per pipe */
> -		tmp = I915_READ(MIPI_CTRL(port));
> -		tmp &= ~READ_REQUEST_PRIORITY_MASK;
> -		I915_WRITE(MIPI_CTRL(port), tmp | READ_REQUEST_PRIORITY_HIGH);
> +		if (IS_VALLEYVIEW(dev)) {
> +			/*
> +			 * escape clock divider, 20MHz, shared for A and C.
> +			 * device ready must be off when doing this! txclkesc?
> +			 */
> +			tmp = I915_READ(MIPI_CTRL(0));
> +			tmp &= ~ESCAPE_CLOCK_DIVIDER_MASK;
> +			I915_WRITE(MIPI_CTRL(0), tmp |
> +					ESCAPE_CLOCK_DIVIDER_1);
> +
> +			/* read request priority is per pipe */
> +			tmp = I915_READ(MIPI_CTRL(port));
> +			tmp &= ~READ_REQUEST_PRIORITY_MASK;
> +			I915_WRITE(MIPI_CTRL(port), tmp |
> +					READ_REQUEST_PRIORITY_HIGH);
> +		} else if (IS_BROXTON(dev)) {
> +			/*
> +			 * FIXME:
> +			 * BXT can connect any PIPE to any MIPI transcoder.

Do you mean, to any DSI port?

> +			 * Select the pipe based on the MIPI port read from
> +			 * VBT for now. Pick PIPE A for MIPI port A and C
> +			 * for port C.
> +			 */
> +			tmp = I915_READ(MIPI_CTRL(port));
> +			tmp &= ~BXT_PIPE_SELECT_MASK;
> +			port ? (tmp |= BXT_PIPE_SELECT_C) :
> +				(tmp |= BXT_PIPE_SELECT_A);

Please compare with PORT_* explicitly instead of port != 0.

> +			I915_WRITE(MIPI_CTRL(port), tmp);
> +		} else {
> +			DRM_ERROR("Invalid DSI device to prepare seq\n");
> +			return;
> +		}

Please drop all such else branches.

>  
>  		/* XXX: why here, why like this? handling in irq handler?! */
>  		I915_WRITE(MIPI_INTR_STAT(port), 0xffffffff);
> @@ -852,6 +889,28 @@ static void intel_dsi_prepare(struct intel_encoder *intel_encoder)
>  		I915_WRITE(MIPI_INIT_COUNT(port),
>  				txclkesc(intel_dsi->escape_clk_div, 100));
>  
> +		if (IS_BROXTON(dev)) {
> +
> +			/*
> +			 * XXX:
> +			 * PIPE2D says LOW_CONTENTION_RECOVERY_DISABLE is
> +			 * reqd for BXT, but bspec doesn't mention it.
> +			 * Lets follow PIPE2D for now.
> +			 */
> +			val |= LOW_CONTENTION_RECOVERY_DISABLE;
> +
> +			if (!intel_dsi->dual_link) {
> +				/*
> +				 * BXT spec says write MIPI_INIT_COUNT for
> +				 * both the ports, even if only one is
> +				 * getting used. So write the other port
> +				 * if not in dual link mode.
> +				 */
> +				I915_WRITE(MIPI_INIT_COUNT(port ==
> +						PORT_A ? PORT_C : PORT_A),
> +							intel_dsi->init_count);

Hmm, why do you write different things to the other port? See above what
gets written normally.

BR,
Jani.

> +			}
> +		}
>  
>  		/* recovery disables */
>  		I915_WRITE(MIPI_EOT_DISABLE(port), tmp);
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux