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
