[v11,3/4] drm/i915: Use new CRC debugfs API
diff mbox

Message ID 1475767268-14379-4-git-send-email-tomeu.vizoso@collabora.com
State New
Headers show

Commit Message

Tomeu Vizoso Oct. 6, 2016, 3:21 p.m. UTC
The core provides now an ABI to userspace for generation of frame CRCs,
so implement the ->set_crc_source() callback and reuse as much code as
possible with the previous ABI implementation.

When handling the pageflip interrupt, we skip 1 or 2 frames depending on
the HW because they contain wrong values. For the legacy ABI for
generating frame CRCs, this was done in userspace but now that we have a
generic ABI it's better if it's not exposed by the kernel.

v2:
    - Leave the legacy implementation in place as the ABI implementation
      in the core is incompatible with it.
v3:
    - Use the "cooked" vblank counter so we have a whole 32 bits.
    - Make sure we don't mess with the state of the legacy CRC capture
      ABI implementation.
v4:
    - Keep use of get_vblank_counter as in the legacy code, will be
      changed in a followup commit.

v5:
    - Skip first frame or two as it's known that they contain wrong
      data.
    - A few fixes suggested by Emil Velikov.

v6:
    - Rework programming of the HW registers to preserve previous
      behavior.

v7:
    - Address whitespace issue.
    - Added a comment on why in the implementation of the new ABI we
      skip the 1st or 2nd frames.

v9:
    - Add stub for intel_crtc_set_crc_source.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
---

 drivers/gpu/drm/i915/i915_drv.h       |  1 +
 drivers/gpu/drm/i915/i915_irq.c       | 83 +++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c  |  1 +
 drivers/gpu/drm/i915/intel_drv.h      |  8 +++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 94 ++++++++++++++++++++++++++++++-----
 5 files changed, 149 insertions(+), 38 deletions(-)

Comments

Jani Nikula Nov. 14, 2016, 10:44 a.m. UTC | #1
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;
> +}
David Weinehall Nov. 15, 2016, 7:16 a.m. UTC | #2
On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
> 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?

I'd say that whenever we have a function pointer we should have a dummy
function without side-effects for this kind of things.


Kind regards, David
Jani Nikula Nov. 15, 2016, 8:27 a.m. UTC | #3
On Tue, 15 Nov 2016, David Weinehall <tao@kernel.org> wrote:
> On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
>> 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?
>
> I'd say that whenever we have a function pointer we should have a dummy
> function without side-effects for this kind of things.

Whoever calls .set_crc_source could do smarter things depending on the
hook not being there vs. just silently plunging on.

BR,
Jani.


>
>
> Kind regards, David
Tomeu Vizoso Nov. 16, 2016, 12:24 p.m. UTC | #4
On 15 November 2016 at 09:27, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 15 Nov 2016, David Weinehall <tao@kernel.org> wrote:
>> On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
>>> 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?
>>
>> I'd say that whenever we have a function pointer we should have a dummy
>> function without side-effects for this kind of things.
>
> Whoever calls .set_crc_source could do smarter things depending on the
> hook not being there vs. just silently plunging on.

In this specific case, when CONFIG_DEBUG_FS=n it doesn't make any
sense to call that callback, so I think we should have a dummy
implementation to avoid adding an ifdef to the .c.

Regards,

Tomeu
Jani Nikula Nov. 16, 2016, 12:58 p.m. UTC | #5
On Wed, 16 Nov 2016, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> On 15 November 2016 at 09:27, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Tue, 15 Nov 2016, David Weinehall <tao@kernel.org> wrote:
>>> On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
>>>> 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?
>>>
>>> I'd say that whenever we have a function pointer we should have a dummy
>>> function without side-effects for this kind of things.
>>
>> Whoever calls .set_crc_source could do smarter things depending on the
>> hook not being there vs. just silently plunging on.
>
> In this specific case, when CONFIG_DEBUG_FS=n it doesn't make any
> sense to call that callback, so I think we should have a dummy
> implementation to avoid adding an ifdef to the .c.

We don't want the ifdef to the .c file, but we could do

#ifdef CONFIG_DEBUG_FS
int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
				size_t *values_cnt);
#else
#define intel_crtc_set_crc_source NULL
#endif

BR,
Jani.
Tomeu Vizoso Nov. 16, 2016, 2:04 p.m. UTC | #6
On 16 November 2016 at 13:58, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 16 Nov 2016, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> On 15 November 2016 at 09:27, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>> On Tue, 15 Nov 2016, David Weinehall <tao@kernel.org> wrote:
>>>> On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
>>>>> 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?
>>>>
>>>> I'd say that whenever we have a function pointer we should have a dummy
>>>> function without side-effects for this kind of things.
>>>
>>> Whoever calls .set_crc_source could do smarter things depending on the
>>> hook not being there vs. just silently plunging on.
>>
>> In this specific case, when CONFIG_DEBUG_FS=n it doesn't make any
>> sense to call that callback, so I think we should have a dummy
>> implementation to avoid adding an ifdef to the .c.
>
> We don't want the ifdef to the .c file, but we could do
>
> #ifdef CONFIG_DEBUG_FS
> int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>                                 size_t *values_cnt);
> #else
> #define intel_crtc_set_crc_source NULL
> #endif

Sounds good to me, and though I don't have any objections, wonder why
this isn't a common idiom in the DRM subsystem. I was able to find
only one instance: drm_compat_ioctl.

Regards,

Tomeu
Jani Nikula Nov. 16, 2016, 2:08 p.m. UTC | #7
On Wed, 16 Nov 2016, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> On 16 November 2016 at 13:58, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Wed, 16 Nov 2016, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>> On 15 November 2016 at 09:27, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>>> On Tue, 15 Nov 2016, David Weinehall <tao@kernel.org> wrote:
>>>>> On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
>>>>>> 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?
>>>>>
>>>>> I'd say that whenever we have a function pointer we should have a dummy
>>>>> function without side-effects for this kind of things.
>>>>
>>>> Whoever calls .set_crc_source could do smarter things depending on the
>>>> hook not being there vs. just silently plunging on.
>>>
>>> In this specific case, when CONFIG_DEBUG_FS=n it doesn't make any
>>> sense to call that callback, so I think we should have a dummy
>>> implementation to avoid adding an ifdef to the .c.
>>
>> We don't want the ifdef to the .c file, but we could do
>>
>> #ifdef CONFIG_DEBUG_FS
>> int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>>                                 size_t *values_cnt);
>> #else
>> #define intel_crtc_set_crc_source NULL
>> #endif
>
> Sounds good to me, and though I don't have any objections, wonder why
> this isn't a common idiom in the DRM subsystem. I was able to find
> only one instance: drm_compat_ioctl.

Heh, and it was I who suggested that too. Maybe get a second opinion. ;)

BR,
Jani.

>
> Regards,
>
> Tomeu
Daniel Vetter Nov. 16, 2016, 2:13 p.m. UTC | #8
On Wed, Nov 16, 2016 at 04:08:30PM +0200, Jani Nikula wrote:
> On Wed, 16 Nov 2016, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> > On 16 November 2016 at 13:58, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >> On Wed, 16 Nov 2016, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> >>> On 15 November 2016 at 09:27, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>>> On Tue, 15 Nov 2016, David Weinehall <tao@kernel.org> wrote:
> >>>>> On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
> >>>>>> 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?
> >>>>>
> >>>>> I'd say that whenever we have a function pointer we should have a dummy
> >>>>> function without side-effects for this kind of things.
> >>>>
> >>>> Whoever calls .set_crc_source could do smarter things depending on the
> >>>> hook not being there vs. just silently plunging on.
> >>>
> >>> In this specific case, when CONFIG_DEBUG_FS=n it doesn't make any
> >>> sense to call that callback, so I think we should have a dummy
> >>> implementation to avoid adding an ifdef to the .c.
> >>
> >> We don't want the ifdef to the .c file, but we could do
> >>
> >> #ifdef CONFIG_DEBUG_FS
> >> int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> >>                                 size_t *values_cnt);
> >> #else
> >> #define intel_crtc_set_crc_source NULL
> >> #endif
> >
> > Sounds good to me, and though I don't have any objections, wonder why
> > this isn't a common idiom in the DRM subsystem. I was able to find
> > only one instance: drm_compat_ioctl.
> 
> Heh, and it was I who suggested that too. Maybe get a second opinion. ;)

Personally I like drm_compat_ioctl, we should spread it far&wide.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f8c66eea06bc..20522f0a4c57 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1713,6 +1713,7 @@  struct intel_pipe_crc {
 	enum intel_pipe_crc_source source;
 	int head, tail;
 	wait_queue_head_t wq;
+	int skipped;
 };
 
 struct i915_frontbuffer_tracking {
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bd6c8b0eeaef..1549cc4f88ec 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1491,41 +1491,72 @@  static void display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
 {
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
 	struct intel_pipe_crc_entry *entry;
-	int head, tail;
+	struct drm_crtc *crtc = intel_get_crtc_for_pipe(&dev_priv->drm, pipe);
+	struct drm_driver *driver = dev_priv->drm.driver;
+	uint32_t crcs[5];
+	int head, tail, ret;
+	u32 frame;
 
 	spin_lock(&pipe_crc->lock);
+	if (pipe_crc->source) {
+		if (!pipe_crc->entries) {
+			spin_unlock(&pipe_crc->lock);
+			DRM_DEBUG_KMS("spurious interrupt\n");
+			return;
+		}
 
-	if (!pipe_crc->entries) {
-		spin_unlock(&pipe_crc->lock);
-		DRM_DEBUG_KMS("spurious interrupt\n");
-		return;
-	}
-
-	head = pipe_crc->head;
-	tail = pipe_crc->tail;
+		head = pipe_crc->head;
+		tail = pipe_crc->tail;
 
-	if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
-		spin_unlock(&pipe_crc->lock);
-		DRM_ERROR("CRC buffer overflowing\n");
-		return;
-	}
+		if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
+			spin_unlock(&pipe_crc->lock);
+			DRM_ERROR("CRC buffer overflowing\n");
+			return;
+		}
 
-	entry = &pipe_crc->entries[head];
+		entry = &pipe_crc->entries[head];
 
-	entry->frame = dev_priv->drm.driver->get_vblank_counter(&dev_priv->drm,
-								 pipe);
-	entry->crc[0] = crc0;
-	entry->crc[1] = crc1;
-	entry->crc[2] = crc2;
-	entry->crc[3] = crc3;
-	entry->crc[4] = crc4;
+		entry->frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
+		entry->crc[0] = crc0;
+		entry->crc[1] = crc1;
+		entry->crc[2] = crc2;
+		entry->crc[3] = crc3;
+		entry->crc[4] = crc4;
 
-	head = (head + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
-	pipe_crc->head = head;
+		head = (head + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1);
+		pipe_crc->head = head;
 
-	spin_unlock(&pipe_crc->lock);
+		spin_unlock(&pipe_crc->lock);
 
-	wake_up_interruptible(&pipe_crc->wq);
+		wake_up_interruptible(&pipe_crc->wq);
+	} else {
+		/*
+		 * For some not yet identified reason, the first CRC is
+		 * bonkers. So let's just wait for the next vblank and read
+		 * out the buggy result.
+		 *
+		 * On CHV sometimes the second CRC is bonkers as well, so
+		 * don't trust that one either.
+		 */
+		if (pipe_crc->skipped == 0 ||
+		    (IS_CHERRYVIEW(dev_priv) && pipe_crc->skipped == 1)) {
+			pipe_crc->skipped++;
+			spin_unlock(&pipe_crc->lock);
+			return;
+		}
+		spin_unlock(&pipe_crc->lock);
+		spin_lock(&crtc->crc.lock);
+		crcs[0] = crc0;
+		crcs[1] = crc1;
+		crcs[2] = crc2;
+		crcs[3] = crc3;
+		crcs[4] = crc4;
+		frame = driver->get_vblank_counter(&dev_priv->drm, pipe);
+		ret = drm_crtc_add_crc_entry(crtc, true, frame, crcs);
+		spin_unlock(&crtc->crc.lock);
+		if (!ret)
+			wake_up_interruptible(&crtc->crc.wq);
+	}
 }
 #else
 static inline void
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
 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;
+}