Re: [BXT DSI timing fixes v1 2/3] drm/i915/bxt: Get pipe timing for BXT DSI

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 13, 2015 at 01:24:27PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Tuesday, October 13, 2015 4:54 PM
> >To: Shankar, Uma
> >Cc: intel-gfx@lists.freedesktop.org; Kumar, Shobhit
> >Subject: Re: [Intel-gfx] [BXT DSI timing fixes v1 2/3] drm/i915/bxt: Get pipe
> >timing for BXT DSI
> >
> >On Tue, Oct 13, 2015 at 11:03:27AM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >> >Sent: Monday, October 12, 2015 10:54 PM
> >> >To: Shankar, Uma
> >> >Cc: intel-gfx@lists.freedesktop.org; Kumar, Shobhit
> >> >Subject: Re: [Intel-gfx] [BXT DSI timing fixes v1 2/3] drm/i915/bxt:
> >> >Get pipe timing for BXT DSI
> >> >
> >> >On Mon, Oct 12, 2015 at 10:55:02PM +0530, Uma Shankar wrote:
> >> >> For BXT DSI, vtotal, vactive, hactive registers are different.
> >> >> Making changes to intel_crtc_mode_get() and get_pipe_timings(), to
> >> >> read the correct registers for BXT DSI.
> >> >>
> >> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> >> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/intel_display.c |   48
> >> >+++++++++++++++++++++++++++++++---
> >> >>  1 file changed, 45 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c
> >> >> b/drivers/gpu/drm/i915/intel_display.c
> >> >> index 75c60b8..2717823 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> >> @@ -7708,6 +7708,7 @@ static void intel_get_pipe_timings(struct
> >> >> intel_crtc
> >> >*crtc,
> >> >>  	struct drm_device *dev = crtc->base.dev;
> >> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> >>  	enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
> >> >> +	bool is_dsi = intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI);
> >> >>  	uint32_t tmp;
> >> >>
> >> >>  	tmp = I915_READ(HTOTAL(cpu_transcoder));
> >> >> @@ -7736,6 +7737,26 @@ static void intel_get_pipe_timings(struct
> >> >> intel_crtc
> >> >*crtc,
> >> >>  		pipe_config->base.adjusted_mode.crtc_vblank_end += 1;
> >> >>  	}
> >> >>
> >> >> +	if (IS_BROXTON(dev) && is_dsi) {
> >> >> +		struct intel_encoder *encoder;
> >> >> +
> >> >> +		for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> >> >> +			struct intel_dsi *intel_dsi =
> >> >> +				enc_to_intel_dsi(&encoder->base);
> >> >> +			enum port port;
> >> >> +
> >> >> +			for_each_dsi_port(port, intel_dsi->ports) {
> >> >> +				pipe_config->base.adjusted_mode.crtc_hdisplay
> >> >=
> >> >> +
> >> >	I915_READ(BXT_MIPI_TRANS_HACTIVE(port));
> >> >> +				pipe_config->base.adjusted_mode.crtc_vdisplay
> >> >=
> >> >> +
> >> >	I915_READ(BXT_MIPI_TRANS_VACTIVE(port));
> >> >> +				pipe_config->base.adjusted_mode.crtc_vtotal =
> >> >> +
> >> >	I915_READ(BXT_MIPI_TRANS_VTOTAL(port));
> >> >> +			}
> >> >> +		}
> >> >> +
> >> >> +	}
> >> >
> >> >I think I already asked this earlier when this patch was posted;
> >> >Don't the normal pipe timing registers contain the same values? And
> >> >if so, why would you need to read them out from the DSI speciific registers?
> >> >
> >>
> >> Normal pipe timing registers like HTOTAL etc. is not used by MIPI DSI
> >> for BXT. Hence getting the data from DSI specific registers for BXT. Separate
> >MIPI transcoder is used for BXT.
> >
> >So calling intel_set_pipe_timings() is pointless (apart from PIPESRC)?
> >
> >If that's the case then it seems the readback should also read all of it from the
> >DSI registers instead of reading some from the pipe timings and some from DSI
> >registers.
> >
> 
> Yes set_pipe_timings is doing only the pipesrc programming which is useful for DSI, rest is not needed.
> 
> Readback can be rectified to read all other details as well from DSI specific registers apart from the ones
> currently being used.

Also this kind of is_dsi check is a bit a hack and won't really work for
fastboot case (intel_pipe_has_type doesn't work at that point yet).
Instead you need to push this into a dsi-specific readout function.
-Daniel

> 
> >>
> >> Regards,
> >> Uma Shankar
> >>
> >> >> +
> >> >>  	tmp = I915_READ(PIPESRC(crtc->pipe));
> >> >>  	pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
> >> >>  	pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1; @@ -10664,6
> >> >> +10685,7 @@ struct drm_display_mode *intel_crtc_mode_get(struct
> >> >drm_device *dev,
> >> >>  	int vtot = I915_READ(VTOTAL(cpu_transcoder));
> >> >>  	int vsync = I915_READ(VSYNC(cpu_transcoder));
> >> >>  	enum pipe pipe = intel_crtc->pipe;
> >> >> +	bool is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
> >> >>
> >> >>  	mode = kzalloc(sizeof(*mode), GFP_KERNEL);
> >> >>  	if (!mode)
> >> >> @@ -10684,15 +10706,35 @@ struct drm_display_mode
> >> >*intel_crtc_mode_get(struct drm_device *dev,
> >> >>  	i9xx_crtc_clock_get(intel_crtc, &pipe_config);
> >> >>
> >> >>  	mode->clock = pipe_config.port_clock / pipe_config.pixel_multiplier;
> >> >> -	mode->hdisplay = (htot & 0xffff) + 1;
> >> >>  	mode->htotal = ((htot & 0xffff0000) >> 16) + 1;
> >> >>  	mode->hsync_start = (hsync & 0xffff) + 1;
> >> >>  	mode->hsync_end = ((hsync & 0xffff0000) >> 16) + 1;
> >> >> -	mode->vdisplay = (vtot & 0xffff) + 1;
> >> >> -	mode->vtotal = ((vtot & 0xffff0000) >> 16) + 1;
> >> >>  	mode->vsync_start = (vsync & 0xffff) + 1;
> >> >>  	mode->vsync_end = ((vsync & 0xffff0000) >> 16) + 1;
> >> >>
> >> >> +	if (IS_BROXTON(dev) && is_dsi) {
> >> >> +		struct intel_encoder *encoder;
> >> >> +
> >> >> +		for_each_encoder_on_crtc(dev, &intel_crtc->base, encoder) {
> >> >> +			struct intel_dsi *intel_dsi =
> >> >> +				enc_to_intel_dsi(&encoder->base);
> >> >> +			enum port port;
> >> >> +
> >> >> +			for_each_dsi_port(port, intel_dsi->ports) {
> >> >> +				mode->vtotal =
> >> >> +
> >> >	I915_READ(BXT_MIPI_TRANS_VTOTAL(port));
> >> >> +				mode->hdisplay =
> >> >> +
> >> >	I915_READ(BXT_MIPI_TRANS_HACTIVE(port));
> >> >> +				mode->vdisplay =
> >> >> +
> >> >	I915_READ(BXT_MIPI_TRANS_VACTIVE(port));
> >> >> +			}
> >> >> +		}
> >> >> +	} else {
> >> >> +		mode->vtotal = ((vtot & 0xffff0000) >> 16) + 1;
> >> >> +		mode->hdisplay = (htot & 0xffff) + 1;
> >> >> +		mode->vdisplay = (vtot & 0xffff) + 1;
> >> >> +	}
> >> >> +
> >> >>  	drm_mode_set_name(mode);
> >> >>
> >> >>  	return mode;
> >> >> --
> >> >> 1.7.9.5
> >> >>
> >> >> _______________________________________________
> >> >> Intel-gfx mailing list
> >> >> Intel-gfx@lists.freedesktop.org
> >> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >
> >> >--
> >> >Ville Syrjälä
> >> >Intel OTC
> >
> >--
> >Ville Syrjälä
> >Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx