Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933011AbcKNKob (ORCPT ); Mon, 14 Nov 2016 05:44:31 -0500 Received: from mga02.intel.com ([134.134.136.20]:13019 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751750AbcKNKoa (ORCPT ); Mon, 14 Nov 2016 05:44:30 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,638,1473145200"; d="scan'208";a="786154042" From: Jani Nikula To: Tomeu Vizoso , linux-kernel@vger.kernel.org Cc: Ville =?utf-8?B?U3lyasOkbMOk?= , Sean Paul , Daniel Vetter , Emil Velikov , Thierry Reding , Tomeu Vizoso , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, David Airlie Subject: Re: [PATCH v11 3/4] drm/i915: Use new CRC debugfs API In-Reply-To: <1475767268-14379-4-git-send-email-tomeu.vizoso@collabora.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <1475767268-14379-1-git-send-email-tomeu.vizoso@collabora.com> <1475767268-14379-4-git-send-email-tomeu.vizoso@collabora.com> Date: Mon, 14 Nov 2016 12:44:25 +0200 Message-ID: <87inrq2vee.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6240 Lines: 175 On Thu, 06 Oct 2016, Tomeu Vizoso 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