[Intel-gfx] [PATCH 1/2] drm/i915/BXT: Get pipe conf from the port registers
Ramalingam C
ramalingam.c at intel.com
Wed Mar 30 13:28:42 UTC 2016
Jani,
Thanks for the review comments. Addressing them in the next patch ver.
On Wednesday 30 March 2016 04:13 PM, Jani Nikula wrote:
> On Tue, 29 Mar 2016, Ramalingam C <ramalingam.c at 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 at intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar at 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().
Thanks for catching such a redundant lines. dropping them.
>
>> }
>>
>> 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.
I take this suggestion for reducing the indentation level. including it
next patch ver.
>
>> +
>> + /* 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.
Can be read from the port register. Will do it.
>
>> +
>> + 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.
Dropping those lines from get_config.
>
>
> 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
--
Thanks,
--Ram
More information about the Intel-gfx
mailing list