On Thu, 06 Oct 2016, Tomeu Vizoso <tomeu.vizoso at collabora.com> wrote:
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 23a6c7213eca..7412a05fa5d9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14636,6 +14636,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = 
> {
>       .page_flip = intel_crtc_page_flip,
>       .atomic_duplicate_state = intel_crtc_duplicate_state,
>       .atomic_destroy_state = intel_crtc_destroy_state,
> +     .set_crc_source = intel_crtc_set_crc_source,
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 737261b09110..31894b7c6517 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1844,6 +1844,14 @@ void intel_color_load_luts(struct drm_crtc_state 
> *crtc_state);
>  /* intel_pipe_crc.c */
>  int intel_pipe_crc_create(struct drm_minor *minor);
>  void intel_pipe_crc_cleanup(struct drm_minor *minor);
> +#ifdef CONFIG_DEBUG_FS
> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> +                           size_t *values_cnt);
> +#else
> +static inline int intel_crtc_set_crc_source(struct drm_crtc *crtc,
> +                                         const char *source_name,
> +                                         size_t *values_cnt) { return 0; }
> +#endif

"inline" here doesn't work because it's used as a function pointer.

Is it better to have a function that returns 0 for .set_crc_source, or
to set .set_crc_source to NULL when CONFIG_DEBUG_FS=n?

BR,
Jani.


>  extern const struct file_operations i915_display_crc_ctl_fops;
>  
>  #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c 
> b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 6c38d4fdcaef..1a51e174e9e5 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -615,6 +615,22 @@ static int ivb_pipe_crc_ctl_reg(struct drm_i915_private 
> *dev_priv,
>       return 0;
>  }
>  
> +static int get_new_crc_ctl_reg(struct drm_i915_private *dev_priv,
> +                            enum pipe pipe,
> +                            enum intel_pipe_crc_source *source, u32 *val)
> +{
> +     if (IS_GEN2(dev_priv))
> +             return i8xx_pipe_crc_ctl_reg(source, val);
> +     else if (INTEL_GEN(dev_priv) < 5)
> +             return i9xx_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> +     else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +             return vlv_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> +     else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
> +             return ilk_pipe_crc_ctl_reg(source, val);
> +     else
> +             return ivb_pipe_crc_ctl_reg(dev_priv, pipe, source, val);
> +}
> +
>  static int pipe_crc_set_source(struct drm_i915_private *dev_priv,
>                              enum pipe pipe,
>                              enum intel_pipe_crc_source source)
> @@ -640,17 +656,7 @@ static int pipe_crc_set_source(struct drm_i915_private 
> *dev_priv,
>               return -EIO;
>       }
>  
> -     if (IS_GEN2(dev_priv))
> -             ret = i8xx_pipe_crc_ctl_reg(&source, &val);
> -     else if (INTEL_GEN(dev_priv) < 5)
> -             ret = i9xx_pipe_crc_ctl_reg(dev_priv, pipe, &source, &val);
> -     else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -             ret = vlv_pipe_crc_ctl_reg(dev_priv, pipe, &source, &val);
> -     else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
> -             ret = ilk_pipe_crc_ctl_reg(&source, &val);
> -     else
> -             ret = ivb_pipe_crc_ctl_reg(dev_priv, pipe, &source, &val);
> -
> +     ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val);
>       if (ret != 0)
>               goto out;
>  
> @@ -691,7 +697,7 @@ static int pipe_crc_set_source(struct drm_i915_private 
> *dev_priv,
>       POSTING_READ(PIPE_CRC_CTL(pipe));
>  
>       /* real source -> none transition */
> -     if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
> +     if (!source) {
>               struct intel_pipe_crc_entry *entries;
>               struct intel_crtc *crtc =
>                       to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> @@ -813,6 +819,11 @@ display_crc_ctl_parse_source(const char *buf, enum 
> intel_pipe_crc_source *s)
>  {
>       int i;
>  
> +     if (!buf) {
> +             *s = INTEL_PIPE_CRC_SOURCE_NONE;
> +             return 0;
> +     }
> +
>       for (i = 0; i < ARRAY_SIZE(pipe_crc_sources); i++)
>               if (!strcmp(buf, pipe_crc_sources[i])) {
>                       *s = i;
> @@ -941,3 +952,62 @@ void intel_pipe_crc_cleanup(struct drm_minor *minor)
>               drm_debugfs_remove_files(info_list, 1, minor);
>       }
>  }
> +
> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> +                           size_t *values_cnt)
> +{
> +     struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> +     struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
> +     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +     enum intel_display_power_domain power_domain;
> +     enum intel_pipe_crc_source source;
> +     u32 val = 0; /* shut up gcc */
> +     int ret = 0;
> +
> +     if (display_crc_ctl_parse_source(source_name, &source) < 0) {
> +             DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
> +             return -EINVAL;
> +     }
> +
> +     power_domain = POWER_DOMAIN_PIPE(crtc->index);
> +     if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) {
> +             DRM_DEBUG_KMS("Trying to capture CRC while pipe is off\n");
> +             return -EIO;
> +     }
> +
> +     ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val);
> +     if (ret != 0)
> +             goto out;
> +
> +     if (source) {
> +             /*
> +              * When IPS gets enabled, the pipe CRC changes. Since IPS gets
> +              * enabled and disabled dynamically based on package C states,
> +              * user space can't make reliable use of the CRCs, so let's just
> +              * completely disable it.
> +              */
> +             hsw_disable_ips(intel_crtc);
> +     }
> +
> +     I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> +     POSTING_READ(PIPE_CRC_CTL(crtc->index));
> +
> +     if (!source) {
> +             if (IS_G4X(dev_priv))
> +                     g4x_undo_pipe_scramble_reset(dev_priv, crtc->index);
> +             else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +                     vlv_undo_pipe_scramble_reset(dev_priv, crtc->index);
> +             else if (IS_HASWELL(dev_priv) && crtc->index == PIPE_A)
> +                     hsw_trans_edp_pipe_A_crc_wa(dev_priv, false);
> +
> +             hsw_enable_ips(intel_crtc);
> +     }
> +
> +     pipe_crc->skipped = 0;
> +     *values_cnt = 5;
> +
> +out:
> +     intel_display_power_put(dev_priv, power_domain);
> +
> +     return ret;
> +}

-- 
Jani Nikula, Intel Open Source Technology Center

Reply via email to