dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] drm: add debug print to update_vblank_count
@ 2019-06-13 12:18 Oleg Vasilev
  2019-06-13 12:18 ` [PATCH 2/3] drm/vkms: stop generating CRCs on buffer overflow Oleg Vasilev
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Oleg Vasilev @ 2019-06-13 12:18 UTC (permalink / raw)
  To: dri-devel; +Cc: Shayenne Moura, Rodrigo Siqueira

Since we are logging all vblank counter updates 30 lines below,
it is also good to have some details whether and how vblank count
difference is calculated.

Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 0d704bddb1a6..603ab105125d 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -241,12 +241,16 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 		 * on the difference in the timestamps and the
 		 * frame/field duration.
 		 */
+
+		DRM_DEBUG_VBL("crtc %u: Calculating number of vblanks."
+			      " diff_ns = %lld, framedur_ns = %d)\n",
+			      pipe, (long long) diff_ns, framedur_ns);
+
 		diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);
 
 		if (diff == 0 && in_vblank_irq)
-			DRM_DEBUG_VBL("crtc %u: Redundant vblirq ignored."
-				      " diff_ns = %lld, framedur_ns = %d)\n",
-				      pipe, (long long) diff_ns, framedur_ns);
+			DRM_DEBUG_VBL("crtc %u: Redundant vblirq ignored\n",
+				      pipe);
 	} else {
 		/* some kind of default for drivers w/o accurate vbl timestamping */
 		diff = in_vblank_irq ? 1 : 0;
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] drm/vkms: stop generating CRCs on buffer overflow
  2019-06-13 12:18 [PATCH 1/3] drm: add debug print to update_vblank_count Oleg Vasilev
@ 2019-06-13 12:18 ` Oleg Vasilev
  2019-06-13 13:07   ` Daniel Vetter
  2019-06-13 12:18 ` [PATCH 3/3] drm/vkms: add crc sources list Oleg Vasilev
  2019-06-14 16:19 ` [PATCH 1/3] drm: add debug print to update_vblank_count Ville Syrjälä
  2 siblings, 1 reply; 9+ messages in thread
From: Oleg Vasilev @ 2019-06-13 12:18 UTC (permalink / raw)
  To: dri-devel; +Cc: Shayenne Moura, Rodrigo Siqueira

Because interrupts are generated artifitially, kernel bug may lead to
infinte attempts to submit CRC.

Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
---
 drivers/gpu/drm/vkms/vkms_crc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index d7b409a3c0f8..bc717a8888b5 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -167,6 +167,7 @@ void vkms_crc_work_handle(struct work_struct *work)
 	u32 crc32 = 0;
 	u64 frame_start, frame_end;
 	unsigned long flags;
+	int ret;
 
 	spin_lock_irqsave(&out->state_lock, flags);
 	frame_start = crtc_state->frame_start;
@@ -202,7 +203,14 @@ void vkms_crc_work_handle(struct work_struct *work)
 	 * missing frames
 	 */
 	while (frame_start <= frame_end)
-		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
+	{
+		ret = drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
+		if (ret) {
+			DRM_WARN("VKMS stop generating CRCs\n");
+			out->crc_enabled = false;
+			break;
+		}
+	}
 
 out:
 	/* to avoid using the same value for frame number again */
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] drm/vkms: add crc sources list
  2019-06-13 12:18 [PATCH 1/3] drm: add debug print to update_vblank_count Oleg Vasilev
  2019-06-13 12:18 ` [PATCH 2/3] drm/vkms: stop generating CRCs on buffer overflow Oleg Vasilev
@ 2019-06-13 12:18 ` Oleg Vasilev
  2019-06-18  2:14   ` Rodrigo Siqueira
  2019-06-19  2:10   ` Rodrigo Siqueira
  2019-06-14 16:19 ` [PATCH 1/3] drm: add debug print to update_vblank_count Ville Syrjälä
  2 siblings, 2 replies; 9+ messages in thread
From: Oleg Vasilev @ 2019-06-13 12:18 UTC (permalink / raw)
  To: dri-devel; +Cc: Shayenne Moura, Rodrigo Siqueira

Other drivers are able to list crc sources when accessing
/sys/kernel/debug/dri/.../crtc-0/crc/control

Even though VKMS now supports only 'auto' mode, it is more consistent to
have the list available to the userspace.

Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
---
 drivers/gpu/drm/vkms/vkms_crc.c  | 9 +++++++++
 drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
 drivers/gpu/drm/vkms/vkms_drv.h  | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
index bc717a8888b5..819313ef80b6 100644
--- a/drivers/gpu/drm/vkms/vkms_crc.c
+++ b/drivers/gpu/drm/vkms/vkms_crc.c
@@ -220,6 +220,15 @@ void vkms_crc_work_handle(struct work_struct *work)
 	spin_unlock_irqrestore(&out->state_lock, flags);
 }
 
+static const char * const pipe_crc_sources[] = {"auto"};
+
+const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
+					size_t *count)
+{
+	*count = ARRAY_SIZE(pipe_crc_sources);
+	return pipe_crc_sources;
+}
+
 static int vkms_crc_parse_source(const char *src_name, bool *enabled)
 {
 	int ret = 0;
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 1bbe099b7db8..4d11292bc6f3 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -147,6 +147,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
 	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
 	.enable_vblank		= vkms_enable_vblank,
 	.disable_vblank		= vkms_disable_vblank,
+	.get_crc_sources	= vkms_get_crc_sources,
 	.set_crc_source		= vkms_set_crc_source,
 	.verify_crc_source	= vkms_verify_crc_source,
 };
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 81f1cfbeb936..891f2d63d74f 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -136,6 +136,8 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
 void vkms_gem_vunmap(struct drm_gem_object *obj);
 
 /* CRC Support */
+const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
+					size_t *count);
 int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name);
 int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 			   size_t *values_cnt);
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] drm/vkms: stop generating CRCs on buffer overflow
  2019-06-13 12:18 ` [PATCH 2/3] drm/vkms: stop generating CRCs on buffer overflow Oleg Vasilev
@ 2019-06-13 13:07   ` Daniel Vetter
  2019-06-18  1:53     ` Rodrigo Siqueira
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2019-06-13 13:07 UTC (permalink / raw)
  To: Oleg Vasilev; +Cc: Shayenne Moura, Rodrigo Siqueira, dri-devel

On Thu, Jun 13, 2019 at 03:18:01PM +0300, Oleg Vasilev wrote:
> Because interrupts are generated artifitially, kernel bug may lead to
> infinte attempts to submit CRC.
> 
> Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> ---
>  drivers/gpu/drm/vkms/vkms_crc.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> index d7b409a3c0f8..bc717a8888b5 100644
> --- a/drivers/gpu/drm/vkms/vkms_crc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> @@ -167,6 +167,7 @@ void vkms_crc_work_handle(struct work_struct *work)
>  	u32 crc32 = 0;
>  	u64 frame_start, frame_end;
>  	unsigned long flags;
> +	int ret;
>  
>  	spin_lock_irqsave(&out->state_lock, flags);
>  	frame_start = crtc_state->frame_start;
> @@ -202,7 +203,14 @@ void vkms_crc_work_handle(struct work_struct *work)
>  	 * missing frames
>  	 */
>  	while (frame_start <= frame_end)
> -		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> +	{

Bikeshed: Kernel codingstyle puts this on the same line as the closing )
of the while/if/for.

Aside from that not sure that's useful here, we've fixed the bug by now
...
-Daniel

> +		ret = drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> +		if (ret) {
> +			DRM_WARN("VKMS stop generating CRCs\n");
> +			out->crc_enabled = false;
> +			break;
> +		}
> +	}
>  
>  out:
>  	/* to avoid using the same value for frame number again */
> -- 
> 2.21.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] drm: add debug print to update_vblank_count
  2019-06-13 12:18 [PATCH 1/3] drm: add debug print to update_vblank_count Oleg Vasilev
  2019-06-13 12:18 ` [PATCH 2/3] drm/vkms: stop generating CRCs on buffer overflow Oleg Vasilev
  2019-06-13 12:18 ` [PATCH 3/3] drm/vkms: add crc sources list Oleg Vasilev
@ 2019-06-14 16:19 ` Ville Syrjälä
  2 siblings, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2019-06-14 16:19 UTC (permalink / raw)
  To: Oleg Vasilev; +Cc: Shayenne Moura, Rodrigo Siqueira, dri-devel

On Thu, Jun 13, 2019 at 03:18:00PM +0300, Oleg Vasilev wrote:
> Since we are logging all vblank counter updates 30 lines below,
> it is also good to have some details whether and how vblank count
> difference is calculated.
> 
> Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 0d704bddb1a6..603ab105125d 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -241,12 +241,16 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
>  		 * on the difference in the timestamps and the
>  		 * frame/field duration.
>  		 */
> +
> +		DRM_DEBUG_VBL("crtc %u: Calculating number of vblanks."
> +			      " diff_ns = %lld, framedur_ns = %d)\n",
> +			      pipe, (long long) diff_ns, framedur_ns);

You can generally extract that info from the timestamps that already
get dumped, but I guess that is a bit of a hassle. So might as well
print the stuff the kernel already calculated for you.

The only downside is that lots of printks from the irq handler bogs
the machine down quite a bit. But these shouldn't be used outside of
short vbl debugging sessions anyway.

Pushed this one to drm-misc-next. Thanks for the patch.

> +
>  		diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);
>  
>  		if (diff == 0 && in_vblank_irq)
> -			DRM_DEBUG_VBL("crtc %u: Redundant vblirq ignored."
> -				      " diff_ns = %lld, framedur_ns = %d)\n",
> -				      pipe, (long long) diff_ns, framedur_ns);
> +			DRM_DEBUG_VBL("crtc %u: Redundant vblirq ignored\n",
> +				      pipe);
>  	} else {
>  		/* some kind of default for drivers w/o accurate vbl timestamping */
>  		diff = in_vblank_irq ? 1 : 0;
> -- 
> 2.21.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] drm/vkms: stop generating CRCs on buffer overflow
  2019-06-13 13:07   ` Daniel Vetter
@ 2019-06-18  1:53     ` Rodrigo Siqueira
  2019-06-18  8:38       ` Daniel Vetter
  0 siblings, 1 reply; 9+ messages in thread
From: Rodrigo Siqueira @ 2019-06-18  1:53 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Shayenne Moura, Oleg Vasilev, dri-devel

Hi Oleg,

First of all, thank you for your patchset.

On 06/13, Daniel Vetter wrote:
> On Thu, Jun 13, 2019 at 03:18:01PM +0300, Oleg Vasilev wrote:
> > Because interrupts are generated artifitially, kernel bug may lead to
> > infinte attempts to submit CRC.
> > 
> > Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> > ---
> >  drivers/gpu/drm/vkms/vkms_crc.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > index d7b409a3c0f8..bc717a8888b5 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > @@ -167,6 +167,7 @@ void vkms_crc_work_handle(struct work_struct *work)
> >  	u32 crc32 = 0;
> >  	u64 frame_start, frame_end;
> >  	unsigned long flags;
> > +	int ret;
> >  
> >  	spin_lock_irqsave(&out->state_lock, flags);
> >  	frame_start = crtc_state->frame_start;
> > @@ -202,7 +203,14 @@ void vkms_crc_work_handle(struct work_struct *work)
> >  	 * missing frames
> >  	 */
> >  	while (frame_start <= frame_end)
> > -		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> > +	{
> 
> Bikeshed: Kernel codingstyle puts this on the same line as the closing )
> of the while/if/for.

I recommend you to create a post-commit script in your git repo.
Something like this:

touch .git/hooks/post-commit

In the post-commit, add:

exec git show --format=email HEAD | ./scripts/checkpatch.pl --strict --codespell
 
> Aside from that not sure that's useful here, we've fixed the bug by now
> ...

IMHO, this patch is useful because it handles the return value from
drm_crtc_add_crc_entry() which make the code a little bit more reliable.
Additionally, it avoids polluting the dmesg output in case of a
problem.

> -Daniel
> 
> > +		ret = drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> > +		if (ret) {
> > +			DRM_WARN("VKMS stop generating CRCs\n");
> > +			out->crc_enabled = false;
> > +			break;
> > +		}
> > +	}
> >  
> >  out:
> >  	/* to avoid using the same value for frame number again */
> > -- 
> > 2.21.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Rodrigo Siqueira
https://siqueira.tech
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] drm/vkms: add crc sources list
  2019-06-13 12:18 ` [PATCH 3/3] drm/vkms: add crc sources list Oleg Vasilev
@ 2019-06-18  2:14   ` Rodrigo Siqueira
  2019-06-19  2:10   ` Rodrigo Siqueira
  1 sibling, 0 replies; 9+ messages in thread
From: Rodrigo Siqueira @ 2019-06-18  2:14 UTC (permalink / raw)
  To: Oleg Vasilev; +Cc: Shayenne Moura, dri-devel

On 06/13, Oleg Vasilev wrote:
> Other drivers are able to list crc sources when accessing
> /sys/kernel/debug/dri/.../crtc-0/crc/control
> 
> Even though VKMS now supports only 'auto' mode, it is more consistent to
> have the list available to the userspace.
> 
> Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> ---
>  drivers/gpu/drm/vkms/vkms_crc.c  | 9 +++++++++
>  drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
>  drivers/gpu/drm/vkms/vkms_drv.h  | 2 ++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> index bc717a8888b5..819313ef80b6 100644
> --- a/drivers/gpu/drm/vkms/vkms_crc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> @@ -220,6 +220,15 @@ void vkms_crc_work_handle(struct work_struct *work)
>  	spin_unlock_irqrestore(&out->state_lock, flags);
>  }
>  
> +static const char * const pipe_crc_sources[] = {"auto"};
> +
> +const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
> +					size_t *count)
> +{
> +	*count = ARRAY_SIZE(pipe_crc_sources);
> +	return pipe_crc_sources;
> +}
> +
>  static int vkms_crc_parse_source(const char *src_name, bool *enabled)
>  {
>  	int ret = 0;
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 1bbe099b7db8..4d11292bc6f3 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -147,6 +147,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
>  	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
>  	.enable_vblank		= vkms_enable_vblank,
>  	.disable_vblank		= vkms_disable_vblank,
> +	.get_crc_sources	= vkms_get_crc_sources,
>  	.set_crc_source		= vkms_set_crc_source,
>  	.verify_crc_source	= vkms_verify_crc_source,
>  };
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 81f1cfbeb936..891f2d63d74f 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -136,6 +136,8 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
>  void vkms_gem_vunmap(struct drm_gem_object *obj);
>  
>  /* CRC Support */
> +const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
> +					size_t *count);

LGTM

Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

>  int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name);
>  int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			   size_t *values_cnt);
> -- 
> 2.21.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Rodrigo Siqueira
https://siqueira.tech
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] drm/vkms: stop generating CRCs on buffer overflow
  2019-06-18  1:53     ` Rodrigo Siqueira
@ 2019-06-18  8:38       ` Daniel Vetter
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2019-06-18  8:38 UTC (permalink / raw)
  To: Rodrigo Siqueira; +Cc: Shayenne Moura, Oleg Vasilev, dri-devel

On Mon, Jun 17, 2019 at 10:53:46PM -0300, Rodrigo Siqueira wrote:
> Hi Oleg,
> 
> First of all, thank you for your patchset.
> 
> On 06/13, Daniel Vetter wrote:
> > On Thu, Jun 13, 2019 at 03:18:01PM +0300, Oleg Vasilev wrote:
> > > Because interrupts are generated artifitially, kernel bug may lead to
> > > infinte attempts to submit CRC.
> > > 
> > > Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> > > ---
> > >  drivers/gpu/drm/vkms/vkms_crc.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > > index d7b409a3c0f8..bc717a8888b5 100644
> > > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > > @@ -167,6 +167,7 @@ void vkms_crc_work_handle(struct work_struct *work)
> > >  	u32 crc32 = 0;
> > >  	u64 frame_start, frame_end;
> > >  	unsigned long flags;
> > > +	int ret;
> > >  
> > >  	spin_lock_irqsave(&out->state_lock, flags);
> > >  	frame_start = crtc_state->frame_start;
> > > @@ -202,7 +203,14 @@ void vkms_crc_work_handle(struct work_struct *work)
> > >  	 * missing frames
> > >  	 */
> > >  	while (frame_start <= frame_end)
> > > -		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> > > +	{
> > 
> > Bikeshed: Kernel codingstyle puts this on the same line as the closing )
> > of the while/if/for.
> 
> I recommend you to create a post-commit script in your git repo.
> Something like this:
> 
> touch .git/hooks/post-commit
> 
> In the post-commit, add:
> 
> exec git show --format=email HEAD | ./scripts/checkpatch.pl --strict --codespell
>  
> > Aside from that not sure that's useful here, we've fixed the bug by now
> > ...
> 
> IMHO, this patch is useful because it handles the return value from
> drm_crtc_add_crc_entry() which make the code a little bit more reliable.
> Additionally, it avoids polluting the dmesg output in case of a
> problem.

If it's the return value that annoys you, there's an easy fix: Let's
remove it! None of the other drivers look at it either.

Imo that's clearer than trying to do something reasonable when facing
driver bugs (which this is here).
-Daniel

> 
> > -Daniel
> > 
> > > +		ret = drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> > > +		if (ret) {
> > > +			DRM_WARN("VKMS stop generating CRCs\n");
> > > +			out->crc_enabled = false;
> > > +			break;
> > > +		}
> > > +	}
> > >  
> > >  out:
> > >  	/* to avoid using the same value for frame number again */
> > > -- 
> > > 2.21.0
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] drm/vkms: add crc sources list
  2019-06-13 12:18 ` [PATCH 3/3] drm/vkms: add crc sources list Oleg Vasilev
  2019-06-18  2:14   ` Rodrigo Siqueira
@ 2019-06-19  2:10   ` Rodrigo Siqueira
  1 sibling, 0 replies; 9+ messages in thread
From: Rodrigo Siqueira @ 2019-06-19  2:10 UTC (permalink / raw)
  To: Oleg Vasilev; +Cc: Shayenne Moura, dri-devel


[-- Attachment #1.1: Type: text/plain, Size: 2708 bytes --]

On 06/13, Oleg Vasilev wrote:
> Other drivers are able to list crc sources when accessing
> /sys/kernel/debug/dri/.../crtc-0/crc/control
> 
> Even though VKMS now supports only 'auto' mode, it is more consistent to
> have the list available to the userspace.
> 
> Signed-off-by: Oleg Vasilev <oleg.vasilev@intel.com>
> ---
>  drivers/gpu/drm/vkms/vkms_crc.c  | 9 +++++++++
>  drivers/gpu/drm/vkms/vkms_crtc.c | 1 +
>  drivers/gpu/drm/vkms/vkms_drv.h  | 2 ++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> index bc717a8888b5..819313ef80b6 100644
> --- a/drivers/gpu/drm/vkms/vkms_crc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> @@ -220,6 +220,15 @@ void vkms_crc_work_handle(struct work_struct *work)
>  	spin_unlock_irqrestore(&out->state_lock, flags);
>  }
>  
> +static const char * const pipe_crc_sources[] = {"auto"};
> +
> +const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
> +					size_t *count)
> +{
> +	*count = ARRAY_SIZE(pipe_crc_sources);
> +	return pipe_crc_sources;
> +}
> +
>  static int vkms_crc_parse_source(const char *src_name, bool *enabled)
>  {
>  	int ret = 0;
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 1bbe099b7db8..4d11292bc6f3 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -147,6 +147,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
>  	.atomic_destroy_state   = vkms_atomic_crtc_destroy_state,
>  	.enable_vblank		= vkms_enable_vblank,
>  	.disable_vblank		= vkms_disable_vblank,
> +	.get_crc_sources	= vkms_get_crc_sources,
>  	.set_crc_source		= vkms_set_crc_source,
>  	.verify_crc_source	= vkms_verify_crc_source,
>  };
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 81f1cfbeb936..891f2d63d74f 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -136,6 +136,8 @@ int vkms_gem_vmap(struct drm_gem_object *obj);
>  void vkms_gem_vunmap(struct drm_gem_object *obj);
>  
>  /* CRC Support */
> +const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
> +					size_t *count);
>  int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name);
>  int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>  			   size_t *values_cnt);
> -- 
> 2.21.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Applied to drm-misc-next.

Thanks

-- 
Rodrigo Siqueira
https://siqueira.tech

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-06-19  2:10 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-13 12:18 [PATCH 1/3] drm: add debug print to update_vblank_count Oleg Vasilev
2019-06-13 12:18 ` [PATCH 2/3] drm/vkms: stop generating CRCs on buffer overflow Oleg Vasilev
2019-06-13 13:07   ` Daniel Vetter
2019-06-18  1:53     ` Rodrigo Siqueira
2019-06-18  8:38       ` Daniel Vetter
2019-06-13 12:18 ` [PATCH 3/3] drm/vkms: add crc sources list Oleg Vasilev
2019-06-18  2:14   ` Rodrigo Siqueira
2019-06-19  2:10   ` Rodrigo Siqueira
2019-06-14 16:19 ` [PATCH 1/3] drm: add debug print to update_vblank_count Ville Syrjälä

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).