Re: [PATCH v11 3/4] drm/i915: Use new CRC debugfs API

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 06 Oct 2016, Tomeu Vizoso <tomeu.vizoso@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx