[Intel-gfx] [PATCH v3 3/3] drm/i915: Use new CRC debugfs API
On 3 August 2016 at 10:01, Daniel Vetter wrote:
> On Fri, Jul 22, 2016 at 04:10:45PM +0200, Tomeu Vizoso wrote:
>> 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.
>>
>> 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.
>
> This should be a separate patch.
Ok.
>> - Make sure we don't mess with the state of the legacy CRC capture
>> ABI implementation.
>
> Hm, as for keeping the legacy debugfs interface alive I think it'd be nice
> if we can implement the old one internal with the new one. I.e. call the
> set_crc_sourc function (after minimal parsing), use the same ringbuffer
> and rework the read function to use the same ringbuffer. Or maybe that's
> not worth it since the plan is to nuke all that code anyway after a few
> months?
I think that if the old code isn't to stay, there's not much point in
changing it to reuse parts of the new one.
Regards,
Tomeu
> -Daniel
>
>>
>> Signed-off-by: Tomeu Vizoso
>
>> ---
>>
>> drivers/gpu/drm/i915/i915_irq.c | 69 ---
>> drivers/gpu/drm/i915/intel_display.c | 1 +
>> drivers/gpu/drm/i915/intel_drv.h | 2 +
>> drivers/gpu/drm/i915/intel_pipe_crc.c | 124
>> --
>> 4 files changed, 133 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index b77d808b71cd..f2726171f7c8 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1492,41 +1492,58 @@ 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;
>>
>> 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 {
>> + spin_unlock(_crc->lock);
>> + spin_lock(>crc.lock);
>> + crcs[0] = crc0;
>> + crcs[1] = crc1;
>> + crcs[2] = crc2;
>> + crcs[3] = crc3;
>> + crcs[4] = crc4;
>> + ret = drm_crtc_add_crc_entry(crtc, true,
>> + drm_accurate_vblank_count(crtc),
>> + crcs);
>> + spin_unlock(>crc.lock);
>> + if (!ret)
>> + wake_up_interruptible(>crc.wq);
>> + }
>> }
>> #else
>> static inline void
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>
[PATCH v3 3/3] drm/i915: Use new CRC debugfs API
On Fri, Jul 22, 2016 at 04:10:45PM +0200, Tomeu Vizoso wrote:
> 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.
>
> 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.
This should be a separate patch.
> - Make sure we don't mess with the state of the legacy CRC capture
> ABI implementation.
Hm, as for keeping the legacy debugfs interface alive I think it'd be nice
if we can implement the old one internal with the new one. I.e. call the
set_crc_sourc function (after minimal parsing), use the same ringbuffer
and rework the read function to use the same ringbuffer. Or maybe that's
not worth it since the plan is to nuke all that code anyway after a few
months?
-Daniel
>
> Signed-off-by: Tomeu Vizoso
> ---
>
> drivers/gpu/drm/i915/i915_irq.c | 69 ---
> drivers/gpu/drm/i915/intel_display.c | 1 +
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> drivers/gpu/drm/i915/intel_pipe_crc.c | 124
> --
> 4 files changed, 133 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b77d808b71cd..f2726171f7c8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1492,41 +1492,58 @@ 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;
>
> 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 {
> + spin_unlock(_crc->lock);
> + spin_lock(>crc.lock);
> + crcs[0] = crc0;
> + crcs[1] = crc1;
> + crcs[2] = crc2;
> + crcs[3] = crc3;
> + crcs[4] = crc4;
> + ret = drm_crtc_add_crc_entry(crtc, true,
> + drm_accurate_vblank_count(crtc),
> + crcs);
> + spin_unlock(>crc.lock);
> + if (!ret)
> + wake_up_interruptible(>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 111b350d1d7e..d91a2f779fb1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14019,6 +14019,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs =
> {
> .page_flip = intel_crtc_page_flip,
>
[PATCH v3 3/3] drm/i915: Use new CRC debugfs API
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.
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.
Signed-off-by: Tomeu Vizoso
---
drivers/gpu/drm/i915/i915_irq.c | 69 ---
drivers/gpu/drm/i915/intel_display.c | 1 +
drivers/gpu/drm/i915/intel_drv.h | 2 +
drivers/gpu/drm/i915/intel_pipe_crc.c | 124 --
4 files changed, 133 insertions(+), 63 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b77d808b71cd..f2726171f7c8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1492,41 +1492,58 @@ 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;
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 {
+ spin_unlock(_crc->lock);
+ spin_lock(>crc.lock);
+ crcs[0] = crc0;
+ crcs[1] = crc1;
+ crcs[2] = crc2;
+ crcs[3] = crc3;
+ crcs[4] = crc4;
+ ret = drm_crtc_add_crc_entry(crtc, true,
+drm_accurate_vblank_count(crtc),
+crcs);
+ spin_unlock(>crc.lock);
+ if (!ret)
+ wake_up_interruptible(>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 111b350d1d7e..d91a2f779fb1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14019,6 +14019,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 5d06cb2425a1..4302cf2cfb04 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1782,6 +1782,8 @@ 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
