drm/drm_debugfs_crc.c: Document that .verify_crc_source vfunc is required for enabling CRC support.

Message ID 20190703150330.21992-1-Liviu.Dudau@arm.com
State New
Headers show
Series
  • drm/drm_debugfs_crc.c: Document that .verify_crc_source vfunc is required for enabling CRC support.
Related show

Commit Message

Liviu Dudau July 3, 2019, 3:03 p.m.
drm_debugfs_crtc_crc_add() function checks that both .set_crc_source and
.verify_crc_source hooks are provided before enabling debugfs support for
reading per-frame CRC data. Make that explicit in the documentation.

Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Daniel Vetter July 3, 2019, 3:21 p.m. | #1
On Wed, Jul 03, 2019 at 04:03:30PM +0100, Liviu Dudau wrote:
> drm_debugfs_crtc_crc_add() function checks that both .set_crc_source and
> .verify_crc_source hooks are provided before enabling debugfs support for
> reading per-frame CRC data. Make that explicit in the documentation.
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I think we have some refactoring room here if we make verify_crc_source
optional (and only allow "auto" for that case). But would only remove like
3-4 implementations, so I guess that's for when the next trivial one shows
up.
-Daniel

> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 7ca486d750e9..6604ed223160 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -66,9 +66,9 @@
>   * the reported CRCs of frames that should have the same contents.
>   *
>   * On the driver side the implementation effort is minimal, drivers only need to
> - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically
> - * set up if that vfunc is set. CRC samples need to be captured in the driver by
> - * calling drm_crtc_add_crc_entry().
> + * implement &drm_crtc_funcs.set_crc_source and &drm_crtc_funcs.verify_crc_source.
> + * The debugfs files are automatically set up if those vfuncs are set. CRC samples
> + * need to be captured in the driver by calling drm_crtc_add_crc_entry().
>   */
>  
>  static int crc_control_show(struct seq_file *m, void *data)
> -- 
> 2.21.0
>
Liviu Dudau July 3, 2019, 3:59 p.m. | #2
On Wed, Jul 03, 2019 at 05:21:20PM +0200, Daniel Vetter wrote:
> On Wed, Jul 03, 2019 at 04:03:30PM +0100, Liviu Dudau wrote:
> > drm_debugfs_crtc_crc_add() function checks that both .set_crc_source and
> > .verify_crc_source hooks are provided before enabling debugfs support for
> > reading per-frame CRC data. Make that explicit in the documentation.
> > 
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I think we have some refactoring room here if we make verify_crc_source
> optional (and only allow "auto" for that case). But would only remove like
> 3-4 implementations, so I guess that's for when the next trivial one shows
> up.

I'm preparing a patch to add CRC support for Komeda, does this means I need
to look at that refactoring?

Best regards,
Liviu


> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_debugfs_crc.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> > index 7ca486d750e9..6604ed223160 100644
> > --- a/drivers/gpu/drm/drm_debugfs_crc.c
> > +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> > @@ -66,9 +66,9 @@
> >   * the reported CRCs of frames that should have the same contents.
> >   *
> >   * On the driver side the implementation effort is minimal, drivers only need to
> > - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically
> > - * set up if that vfunc is set. CRC samples need to be captured in the driver by
> > - * calling drm_crtc_add_crc_entry().
> > + * implement &drm_crtc_funcs.set_crc_source and &drm_crtc_funcs.verify_crc_source.
> > + * The debugfs files are automatically set up if those vfuncs are set. CRC samples
> > + * need to be captured in the driver by calling drm_crtc_add_crc_entry().
> >   */
> >  
> >  static int crc_control_show(struct seq_file *m, void *data)
> > -- 
> > 2.21.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter July 3, 2019, 4:14 p.m. | #3
On Wed, Jul 3, 2019 at 5:59 PM Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>
> On Wed, Jul 03, 2019 at 05:21:20PM +0200, Daniel Vetter wrote:
> > On Wed, Jul 03, 2019 at 04:03:30PM +0100, Liviu Dudau wrote:
> > > drm_debugfs_crtc_crc_add() function checks that both .set_crc_source and
> > > .verify_crc_source hooks are provided before enabling debugfs support for
> > > reading per-frame CRC data. Make that explicit in the documentation.
> > >
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > I think we have some refactoring room here if we make verify_crc_source
> > optional (and only allow "auto" for that case). But would only remove like
> > 3-4 implementations, so I guess that's for when the next trivial one shows
> > up.
>
> I'm preparing a patch to add CRC support for Komeda, does this means I need
> to look at that refactoring?

If all you do is add support for the "auto" source, then I think that
would be nice. Shouldn't be really more work than another copypasta
version, and then if you do the 3 or so patches to remove the copies
from drivers you also have people you can volunteer to review the
entire thing :-)
-Daniel

>
> Best regards,
> Liviu
>
>
> > -Daniel
> >
> > > ---
> > >  drivers/gpu/drm/drm_debugfs_crc.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> > > index 7ca486d750e9..6604ed223160 100644
> > > --- a/drivers/gpu/drm/drm_debugfs_crc.c
> > > +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> > > @@ -66,9 +66,9 @@
> > >   * the reported CRCs of frames that should have the same contents.
> > >   *
> > >   * On the driver side the implementation effort is minimal, drivers only need to
> > > - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically
> > > - * set up if that vfunc is set. CRC samples need to be captured in the driver by
> > > - * calling drm_crtc_add_crc_entry().
> > > + * implement &drm_crtc_funcs.set_crc_source and &drm_crtc_funcs.verify_crc_source.
> > > + * The debugfs files are automatically set up if those vfuncs are set. CRC samples
> > > + * need to be captured in the driver by calling drm_crtc_add_crc_entry().
> > >   */
> > >
> > >  static int crc_control_show(struct seq_file *m, void *data)
> > > --
> > > 2.21.0
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

Patch

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 7ca486d750e9..6604ed223160 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -66,9 +66,9 @@ 
  * the reported CRCs of frames that should have the same contents.
  *
  * On the driver side the implementation effort is minimal, drivers only need to
- * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically
- * set up if that vfunc is set. CRC samples need to be captured in the driver by
- * calling drm_crtc_add_crc_entry().
+ * implement &drm_crtc_funcs.set_crc_source and &drm_crtc_funcs.verify_crc_source.
+ * The debugfs files are automatically set up if those vfuncs are set. CRC samples
+ * need to be captured in the driver by calling drm_crtc_add_crc_entry().
  */
 
 static int crc_control_show(struct seq_file *m, void *data)