| Message ID | 20190613121802.2193-2-oleg.vasilev@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm: add debug print to update_vblank_count | expand |
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 >
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
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
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 */
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(-)