[Intel-gfx] [PATCH 07/31] drm/i915: IPS Sysfs interface.

[Daniel Stone]

Hi Rodrigo,

On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
> diff --git a/drivers/gpu/drm/i915/intel_ips.c b/drivers/gpu/drm/i915/intel_ips.c
> index b867aba..6bc5c55 100644
> --- a/drivers/gpu/drm/i915/intel_ips.c
> +++ b/drivers/gpu/drm/i915/intel_ips.c
> @@ -105,18 +105,21 @@ bool intel_ips_ready(struct intel_crtc *crtc,
>   * This function is called to enable IPS on certain pipe.
>   * All needed conditions should've checked already by intel_ips_ready.
>   */
> -void intel_ips_enable(struct intel_crtc *crtc)
> +int intel_ips_enable(struct intel_crtc *crtc)
>  {
>         struct drm_device *dev = crtc->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       int ret = 0;
>
>         if (!crtc->config->ips_ready)
> -               return;
> +               return -EINVAL;
>
>         mutex_lock(&dev_priv->display_ips.lock);
>
> -       if (dev_priv->display_ips.enabled)
> +       if (dev_priv->display_ips.enabled) {
> +               ret = -EALREADY;
>                 goto out;
> +       }
>
>         /*
>          * We can only enable IPS after we enable a plane
> @@ -147,6 +150,7 @@ void intel_ips_enable(struct intel_crtc *crtc)
>                  */
>                 if (wait_for(I915_READ_NOTRACE(IPS_CTL) & IPS_ENABLE, 50)) {
>                         DRM_ERROR("Timed out waiting for IPS enable\n");
> +                       ret = -ETIMEDOUT;
>                         goto out;
>                 }
>         }
> @@ -154,6 +158,7 @@ void intel_ips_enable(struct intel_crtc *crtc)
>         dev_priv->display_ips.enabled = true;
>  out:
>         mutex_unlock(&dev_priv->display_ips.lock);
> +       return ret;
>  }
>
>  /**
> @@ -162,16 +167,22 @@ out:
>   *
>   * This function is called to disable IPS on certain pipe whenever it is needed
>   * to disable IPS on the pipe.
> + *
> + * Returns:
> + * 0 on success and -errno otherwise.
>   */
> -void intel_ips_disable(struct intel_crtc *crtc)
> +int intel_ips_disable(struct intel_crtc *crtc)
>  {
>         struct drm_device *dev = crtc->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       int ret = 0;
>
>         mutex_lock(&dev_priv->display_ips.lock);
>
> -       if (!dev_priv->display_ips.enabled)
> +       if (!dev_priv->display_ips.enabled) {
> +               ret = -EALREADY;
>                 goto out;
> +       }
>
>         assert_plane_enabled(dev_priv, crtc->plane);
>         if (IS_BROADWELL(dev)) {
> @@ -196,6 +207,7 @@ void intel_ips_disable(struct intel_crtc *crtc)
>         dev_priv->display_ips.enabled = false;
>  out:
>         mutex_unlock(&dev_priv->display_ips.lock);
> +       return ret;
>  }

It would be nice to have these from the beginning, rather than
modifying them part-way through.

> @@ -206,6 +218,9 @@ out:
>   * It checks if there is any other plane enabled on the pipe when primary is
>   * going to be disabled. In this case IPS can continue enabled, but it needs
>   * to be disabled otherwise.
> + *
> + * Returns:
> + * 0 on success and -errno otherwise.
>   */
>  void intel_ips_disable_if_alone(struct intel_crtc *crtc)
>  {

... this one is still left void.

Cheers,
Daniel