Re: [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 29 Mar 2016, Ramalingam C <ramalingam.c@intel.com> wrote:
> At BXT DSI, PIPE registers are inactive. So we can't get the
> PIPE's mode parameters from them. The possible option is
> retriving them from the PORT registers.
>
> The required changes are added for BXT in intel_dsi_get_config
> (encoder->get_config).
>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
> Previously reviewed at https://patchwork.freedesktop.org/patch/75301/
>
>  drivers/gpu/drm/i915/i915_reg.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |   44 ++++++++++++++
>  drivers/gpu/drm/i915/intel_dsi.c     |  104 ++++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c839ce9..da3cdef 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8210,6 +8210,7 @@ enum skl_disp_power_wells {
>  #define  BXT_PIPE_SELECT_SHIFT				7
>  #define  BXT_PIPE_SELECT_MASK				(7 << 7)
>  #define  BXT_PIPE_SELECT(pipe)				((pipe) << 7)
> +#define  BXT_PORT_TO_PIPE(ctrl)		((ctrl & BXT_PIPE_SELECT_MASK) >> 7)
>  
>  #define _MIPIA_DATA_ADDRESS		(dev_priv->mipi_mmio_base + 0xb108)
>  #define _MIPIC_DATA_ADDRESS		(dev_priv->mipi_mmio_base + 0xb908)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 29aa64b..c0627d6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9941,11 +9941,40 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  	}
>  }
>  
> +struct intel_encoder *bxt_get_dsi_encoder_for_crtc(struct intel_crtc *crtc,
> +					struct intel_crtc_state *pipe_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_encoder *intel_encoder;
> +	struct intel_dsi *intel_dsi;
> +	enum port port;
> +	u32 tmp;
> +
> +	for_each_intel_encoder(dev, intel_encoder) {
> +		if (intel_encoder->type == INTEL_OUTPUT_DSI) {
> +			intel_dsi = enc_to_intel_dsi(&intel_encoder->base);
> +			for_each_dsi_port(port, intel_dsi->ports) {
> +				if (!(I915_READ(BXT_MIPI_PORT_CTRL(port)) &
> +								DPI_ENABLE))
> +					break;
> +
> +				tmp = I915_READ(MIPI_CTRL(port));
> +				if ((tmp & BXT_PIPE_SELECT_MASK) ==
> +						BXT_PIPE_SELECT(crtc->pipe))
> +					return intel_encoder;
> +			}
> +		}
> +	}
> +	return NULL;
> +}
> +
>  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  				    struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_encoder *intel_encoder, *attached_encoder = NULL;
>  	enum intel_display_power_domain power_domain;
>  	unsigned long power_domain_mask;
>  	bool active;
> @@ -9965,6 +9994,21 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  		WARN_ON(active && pipe_config->has_dsi_encoder);
>  		if (pipe_config->has_dsi_encoder)
>  			active = true;
> +
> +		for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder)
> +			attached_encoder = intel_encoder;
> +
> +		/*
> +		 * attached_encoder will be NULL, if there is no modeset from
> +		 * the kernel bootup.
> +		 */
> +		if (!attached_encoder && pipe_config->has_dsi_encoder)
> +			attached_encoder =
> +				bxt_get_dsi_encoder_for_crtc(crtc, pipe_config);
> +
> +		if (attached_encoder && attached_encoder->get_config)
> +			attached_encoder->get_config(attached_encoder,
> +								pipe_config);

No, you must not add a new call to the encoder->get_config() hook.

haswell_get_pipe_config() is called through the
dev_priv->display.get_pipe_config() function pointer. This happens in
check_crtc_state() and intel_modeset_readout_hw_state(). In both places,
encoder->get_config() is called afterwards, if encoder->get_hw_state()
returns true for the encoder.

The infrastructure is there, you only need to update DSI ->get_config().

>  	}
>  
>  	if (!active)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 0de74e1..69a801e 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -740,14 +740,118 @@ out_put_power:
>  	return active;
>  }
>  
> +/* return pixels equvalent to txbyteclkhs */
> +static u16 pixels_from_txbyteclkhs(u16 clk_hs, int bpp, int lane_count,
> +		       u16 burst_mode_ratio)
> +{
> +	return DIV_ROUND_UP((clk_hs * lane_count * 8 * 100),
> +						(bpp * burst_mode_ratio));
> +}
> +
> +static void bxt_dsi_get_pipe_config(struct intel_encoder *encoder,
> +				 struct intel_crtc_state *pipe_config)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_display_mode *adjusted_mode =
> +					&pipe_config->base.adjusted_mode;
> +	struct intel_dsi *intel_dsi =
> +				enc_to_intel_dsi(&encoder->base);
> +	unsigned int bpp = mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format);
> +	unsigned int lane_count = intel_dsi->lane_count;
> +	enum port port;
> +	enum pipe dsi_pipe;
> +	u16 hactive, hfp, hsync, hbp, vfp, vsync, vbp;
> +	uint32_t tmp;
> +
> +	for_each_dsi_port(port, intel_dsi->ports) {
> +		if (!(I915_READ(BXT_MIPI_PORT_CTRL(port)) & DPI_ENABLE))
> +			continue;

Please use the loop just for figuring out the port to use for reading
the registers. After that, you can do the rest at a lower indentation
level, and it's clearer the rest only gets done once, for one port.

> +
> +		/* In terms of pixels */
> +		adjusted_mode->crtc_hdisplay =
> +					I915_READ(BXT_MIPI_TRANS_HACTIVE(port));
> +		adjusted_mode->crtc_vdisplay =
> +					I915_READ(BXT_MIPI_TRANS_VACTIVE(port));
> +		adjusted_mode->crtc_vtotal =
> +					I915_READ(BXT_MIPI_TRANS_VTOTAL(port));
> +
> +		hactive = adjusted_mode->crtc_hdisplay;
> +		hfp = I915_READ(MIPI_HFP_COUNT(port));
> +
> +		/*
> +		 * meaningful for video mode non-burst sync pulse mode only,
> +		 * can be zero for non-burst sync events and burst modes
> +		 */
> +		hsync = I915_READ(MIPI_HSYNC_PADDING_COUNT(port));
> +		hbp = I915_READ(MIPI_HBP_COUNT(port));
> +
> +		/* horizontal values are in terms of high speed byte clock */
> +		hfp = pixels_from_txbyteclkhs(hfp, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +		hsync = pixels_from_txbyteclkhs(hsync, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +		hbp = pixels_from_txbyteclkhs(hbp, bpp, lane_count,
> +						intel_dsi->burst_mode_ratio);
> +
> +		if (intel_dsi->dual_link) {
> +			hfp *= 2;
> +			hsync *= 2;
> +			hbp *= 2;
> +		}
> +
> +		/* vertical values are in terms of lines */
> +		vfp = I915_READ(MIPI_VFP_COUNT(port));
> +		vsync = I915_READ(MIPI_VSYNC_PADDING_COUNT(port));
> +		vbp = I915_READ(MIPI_VBP_COUNT(port));
> +
> +		adjusted_mode->crtc_htotal = hactive + hfp + hsync + hbp;
> +		adjusted_mode->crtc_hsync_start =
> +					hfp + adjusted_mode->crtc_hdisplay;
> +		adjusted_mode->crtc_hsync_end =
> +					hsync + adjusted_mode->crtc_hsync_start;
> +		adjusted_mode->crtc_hblank_start = adjusted_mode->crtc_hdisplay;
> +		adjusted_mode->crtc_hblank_end = adjusted_mode->crtc_htotal;
> +
> +		adjusted_mode->crtc_vsync_start =
> +					vfp + adjusted_mode->crtc_vdisplay;
> +		adjusted_mode->crtc_vsync_end =
> +					vsync + adjusted_mode->crtc_vsync_start;
> +		adjusted_mode->crtc_vblank_start = adjusted_mode->crtc_vdisplay;
> +		adjusted_mode->crtc_vblank_end = adjusted_mode->crtc_vtotal;
> +
> +		pipe_config->pipe_bpp =
> +			mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format);

I guess this should be read from the register.

> +
> +		dsi_pipe = BXT_PORT_TO_PIPE(I915_READ(MIPI_CTRL(port)));
> +		if (dsi_pipe > PIPE_C) {
> +			DRM_ERROR("Invalid PIPE configured\n");
> +			break;
> +		}
> +
> +		tmp = I915_READ(PIPESRC(dsi_pipe));
> +		pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
> +		pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
> +
> +		pipe_config->base.mode.vdisplay = pipe_config->pipe_src_h;
> +		pipe_config->base.mode.hdisplay = pipe_config->pipe_src_w;

This part is already covered by intel_get_pipe_src_size() for DSI.


BR,
Jani.


> +		break;
> +	}
> +}
> +
> +
>  static void intel_dsi_get_config(struct intel_encoder *encoder,
>  				 struct intel_crtc_state *pipe_config)
>  {
> +	struct drm_device *dev = encoder->base.dev;
>  	u32 pclk;
>  	DRM_DEBUG_KMS("\n");
>  
>  	pipe_config->has_dsi_encoder = true;
>  
> +	if (IS_BROXTON(dev))
> +		bxt_dsi_get_pipe_config(encoder, pipe_config);
> +
>  	/*
>  	 * DPLL_MD is not used in case of DSI, reading will get some default value
>  	 * set dpll_md = 0

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