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

2016-11-16 Thread Daniel Vetter 
On Wed, Nov 16, 2016 at 04:08:30PM +0200, Jani Nikula wrote:
> On Wed, 16 Nov 2016, Tomeu Vizoso  wrote:
> > On 16 November 2016 at 13:58, Jani Nikula  
> > wrote:
> >> On Wed, 16 Nov 2016, Tomeu Vizoso  wrote:
> >>> On 15 November 2016 at 09:27, Jani Nikula  
> >>> wrote:
>  On Tue, 15 Nov 2016, David Weinehall  wrote:
> > On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
> >> 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?
> >
> > 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
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-11-16 Thread Jani Nikula 
On Wed, 16 Nov 2016, Tomeu Vizoso  wrote:
> On 16 November 2016 at 13:58, Jani Nikula  wrote:
>> On Wed, 16 Nov 2016, Tomeu Vizoso  wrote:
>>> On 15 November 2016 at 09:27, Jani Nikula  
>>> wrote:
 On Tue, 15 Nov 2016, David Weinehall  wrote:
> On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
>> 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?
>
> 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

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-11-16 Thread Tomeu Vizoso 
On 16 November 2016 at 13:58, Jani Nikula  wrote:
> On Wed, 16 Nov 2016, Tomeu Vizoso  wrote:
>> On 15 November 2016 at 09:27, Jani Nikula  
>> wrote:
>>> On Tue, 15 Nov 2016, David Weinehall  wrote:
 On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
> 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?

 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-11-16 Thread Jani Nikula 
On Wed, 16 Nov 2016, Tomeu Vizoso  wrote:
> On 15 November 2016 at 09:27, Jani Nikula  wrote:
>> On Tue, 15 Nov 2016, David Weinehall  wrote:
>>> On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
 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?
>>>
>>> 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.

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-11-16 Thread Tomeu Vizoso 
On 15 November 2016 at 09:27, Jani Nikula  wrote:
> On Tue, 15 Nov 2016, David Weinehall  wrote:
>> On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
>>> 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?
>>
>> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-11-15 Thread Jani Nikula 
On Tue, 15 Nov 2016, David Weinehall  wrote:
> On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
>> 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?
>
> 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

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-11-14 Thread David Weinehall 
On Mon, Nov 14, 2016 at 12:44:25PM +0200, Jani Nikula wrote:
> 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?

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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2016-11-14 Thread Jani Nikula 
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(, );
> - else if (INTEL_GEN(dev_priv) < 5)
> - ret = i9xx_pipe_crc_ctl_reg(dev_priv, pipe, , );
> - else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> - ret = vlv_pipe_crc_ctl_reg(dev_priv, pipe, , );
> - else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
> - ret = ilk_pipe_crc_ctl_reg(, );
> - else
> - ret = ivb_pipe_crc_ctl_reg(dev_priv, pipe, , );
> -
> + ret = get_new_crc_ctl_reg(dev_priv, pipe, , );
>   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-gfx] [PATCH v11 3/4] drm/i915: Use new CRC debugfs API

2016-10-06 Thread Tomeu Vizoso 
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 
Reviewed-by: Emil Velikov 
---

 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(-)

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 = _priv->pipe_crc[pipe];
struct intel_pipe_crc_entry *entry;
-   int head, tail;
+   struct drm_crtc *crtc = intel_get_crtc_for_pipe(_priv->drm, pipe);
+   struct drm_driver *driver = dev_priv->drm.driver;
+   uint32_t crcs[5];
+   int head, tail, ret;
+   u32 frame;
 
spin_lock(_crc->lock);
+   if (pipe_crc->source) {
+   if (!pipe_crc->entries) {
+   spin_unlock(_crc->lock);
+   DRM_DEBUG_KMS("spurious interrupt\n");
+   return;
+   }
 
-   if (!pipe_crc->entries) {
-   spin_unlock(_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(_crc->lock);
-   DRM_ERROR("CRC buffer overflowing\n");
-   return;
-   }
+   if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
+   spin_unlock(_crc->lock);
+   DRM_ERROR("CRC buffer overflowing\n");
+   return;
+   }
 
-   entry = _crc->entries[head];
+   entry = _crc->entries[head];
 
-   entry->frame = dev_priv->drm.driver->get_vblank_counter(_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(_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(_crc->lock);
+   spin_unlock(_crc->lock);
 
-   wake_up_interruptible(_crc->wq);
+   wake_up_interruptible(_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
+