2014-06-27 20:04 GMT-03:00 <ville.syrjala@xxxxxxxxxxxxxxx>:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> CHV has a third pipe so we need to compute the watermarks for its
> planes. Add cherryview_update_wm() to do just that.
Ok, so basically the only real difference between this code and VLV's
code is when you enable CXSR: on VLV you just enable CXSR after the
other WM registers are already written. I wonder if this is to prevent
any intermediate situations where the previous WM values did not allow
CXSR, so enabling it first would result in errors/underruns. On this
case, the CHV function would need to do the same thing as VLV, right?
Do you have any specific reason for keeping the CXSR code different on
CHV?
Also, instead of adding a new function, you could probably just
rewrite vlv_update_wm to use for_each_pipe() instead of the current
method. You'd define plane_wm[num_pipes] arrays instead of one
variable per pipe, then you would be able to use the same function for
both VLV and CHV. Anyway, I don't think we should block your patch
based on this suggestion, so if you just provide a good explanation
for the CXSR question - or a new patch - I'll give a R-B tag.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 77 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index cb0b4b4..346dced 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1364,6 +1364,81 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
> (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> }
>
> +static void cherryview_update_wm(struct drm_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->dev;
> + static const int sr_latency_ns = 12000;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + int planea_wm, planeb_wm, planec_wm;
> + int cursora_wm, cursorb_wm, cursorc_wm;
> + int plane_sr, cursor_sr;
> + int ignore_plane_sr, ignore_cursor_sr;
> + unsigned int enabled = 0;
> +
> + vlv_update_drain_latency(dev);
> +
> + if (g4x_compute_wm0(dev, PIPE_A,
> + &valleyview_wm_info, latency_ns,
> + &valleyview_cursor_wm_info, latency_ns,
> + &planea_wm, &cursora_wm))
> + enabled |= 1 << PIPE_A;
> +
> + if (g4x_compute_wm0(dev, PIPE_B,
> + &valleyview_wm_info, latency_ns,
> + &valleyview_cursor_wm_info, latency_ns,
> + &planeb_wm, &cursorb_wm))
> + enabled |= 1 << PIPE_B;
> +
> + if (g4x_compute_wm0(dev, PIPE_C,
> + &valleyview_wm_info, latency_ns,
> + &valleyview_cursor_wm_info, latency_ns,
> + &planec_wm, &cursorc_wm))
> + enabled |= 1 << PIPE_C;
> +
> + if (single_plane_enabled(enabled) &&
> + g4x_compute_srwm(dev, ffs(enabled) - 1,
> + sr_latency_ns,
> + &valleyview_wm_info,
> + &valleyview_cursor_wm_info,
> + &plane_sr, &ignore_cursor_sr) &&
> + g4x_compute_srwm(dev, ffs(enabled) - 1,
> + 2*sr_latency_ns,
> + &valleyview_wm_info,
> + &valleyview_cursor_wm_info,
> + &ignore_plane_sr, &cursor_sr)) {
> + I915_WRITE(FW_BLC_SELF_VLV, FW_CSPWRDWNEN);
> + } else {
> + I915_WRITE(FW_BLC_SELF_VLV,
> + I915_READ(FW_BLC_SELF_VLV) & ~FW_CSPWRDWNEN);
> + plane_sr = cursor_sr = 0;
> + }
> +
> + DRM_DEBUG_KMS("Setting FIFO watermarks - A: plane=%d, cursor=%d, "
> + "B: plane=%d, cursor=%d, C: plane=%d, cursor=%d, "
> + "SR: plane=%d, cursor=%d\n",
> + planea_wm, cursora_wm,
> + planeb_wm, cursorb_wm,
> + planec_wm, cursorc_wm,
> + plane_sr, cursor_sr);
> +
> + I915_WRITE(DSPFW1,
> + (plane_sr << DSPFW_SR_SHIFT) |
> + (cursorb_wm << DSPFW_CURSORB_SHIFT) |
> + (planeb_wm << DSPFW_PLANEB_SHIFT) |
> + (planea_wm << DSPFW_PLANEA_SHIFT));
> + I915_WRITE(DSPFW2,
> + (I915_READ(DSPFW2) & ~DSPFW_CURSORA_MASK) |
> + (cursora_wm << DSPFW_CURSORA_SHIFT));
> + I915_WRITE(DSPFW3,
> + (I915_READ(DSPFW3) & ~DSPFW_CURSOR_SR_MASK) |
> + (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
> + I915_WRITE(DSPFW9_CHV,
> + (I915_READ(DSPFW9_CHV) & ~(DSPFW_PLANEC_MASK |
> + DSPFW_CURSORC_MASK)) |
> + (planec_wm << DSPFW_PLANEC_SHIFT) |
> + (cursorc_wm << DSPFW_CURSORC_SHIFT));
> +}
> +
> static void g4x_update_wm(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> @@ -7046,7 +7121,7 @@ void intel_init_pm(struct drm_device *dev)
> else if (INTEL_INFO(dev)->gen == 8)
> dev_priv->display.init_clock_gating = gen8_init_clock_gating;
> } else if (IS_CHERRYVIEW(dev)) {
> - dev_priv->display.update_wm = valleyview_update_wm;
> + dev_priv->display.update_wm = cherryview_update_wm;
> dev_priv->display.init_clock_gating =
> cherryview_init_clock_gating;
> } else if (IS_VALLEYVIEW(dev)) {
> --
> 1.8.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
