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

Jani Nikula
2016-03-30T10:43:39Z

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