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

[Ramalingam C <ramalingam.c@intel.com>]

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@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().
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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx