| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87inrq2vee.fsf@intel.com>
Date: Mon, 14 Nov 2016 12:44:25 +0200
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Tomeu Vizoso <tomeu.vizoso@...labora.com>,
linux-kernel@...r.kernel.org
Cc: Ville Syrjälä <ville.syrjala@...ux.intel.com>,
Sean Paul <seanpaul@...omium.org>,
Daniel Vetter <daniel.vetter@...el.com>,
Emil Velikov <emil.l.velikov@...il.com>,
Thierry Reding <thierry.reding@...il.com>,
Tomeu Vizoso <tomeu.vizoso@...labora.com>,
intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
David Airlie <airlied@...ux.ie>
Subject: Re: [PATCH v11 3/4] drm/i915: Use new CRC debugfs API
On Thu, 06 Oct 2016, Tomeu Vizoso <tomeu.vizoso@...labora.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
Powered by blists - more mailing lists