Re: [PATCH v3 3/3] drm/i915: Use new CRC debugfs API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3 August 2016 at 10:01, Daniel Vetter <daniel@xxxxxxxx> 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 <tomeu.vizoso@xxxxxxxxxxxxx>
>
>> ---
>>
>>  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 = &dev_priv->pipe_crc[pipe];
>>       struct intel_pipe_crc_entry *entry;
>> -     int head, tail;
>> +     struct drm_crtc *crtc = intel_get_crtc_for_pipe(&dev_priv->drm, pipe);
>> +     struct drm_driver *driver = dev_priv->drm.driver;
>> +     uint32_t crcs[5];
>> +     int head, tail, ret;
>>
>>       spin_lock(&pipe_crc->lock);
>> +     if (pipe_crc->source) {
>> +             if (!pipe_crc->entries) {
>> +                     spin_unlock(&pipe_crc->lock);
>> +                     DRM_DEBUG_KMS("spurious interrupt\n");
>> +                     return;
>> +             }
>>
>> -     if (!pipe_crc->entries) {
>> -             spin_unlock(&pipe_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(&pipe_crc->lock);
>> -             DRM_ERROR("CRC buffer overflowing\n");
>> -             return;
>> -     }
>> +             if (CIRC_SPACE(head, tail, INTEL_PIPE_CRC_ENTRIES_NR) < 1) {
>> +                     spin_unlock(&pipe_crc->lock);
>> +                     DRM_ERROR("CRC buffer overflowing\n");
>> +                     return;
>> +             }
>>
>> -     entry = &pipe_crc->entries[head];
>> +             entry = &pipe_crc->entries[head];
>>
>> -     entry->frame = dev_priv->drm.driver->get_vblank_counter(&dev_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(&dev_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(&pipe_crc->lock);
>> +             spin_unlock(&pipe_crc->lock);
>>
>> -     wake_up_interruptible(&pipe_crc->wq);
>> +             wake_up_interruptible(&pipe_crc->wq);
>> +     } else {
>> +             spin_unlock(&pipe_crc->lock);
>> +             spin_lock(&crtc->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(&crtc->crc.lock);
>> +             if (!ret)
>> +                     wake_up_interruptible(&crtc->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 *minor);
>> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>> +                           size_t *values_cnt);
>>  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 4e6c28ce8ecf..d2f06a1a919a 100644
>> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
>> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
>> @@ -624,15 +624,62 @@ static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
>>       return 0;
>>  }
>>
>> +static int do_set_crc_source(struct drm_device *dev, enum pipe pipe,
>> +                          enum intel_pipe_crc_source source)
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev,
>> +                                                                     pipe));
>> +     u32 val = 0; /* shut up gcc */
>> +     int ret;
>> +
>> +     if (IS_GEN2(dev))
>> +             ret = i8xx_pipe_crc_ctl_reg(&source, &val);
>> +     else if (INTEL_INFO(dev)->gen < 5)
>> +             ret = i9xx_pipe_crc_ctl_reg(dev, pipe, &source, &val);
>> +     else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
>> +             ret = vlv_pipe_crc_ctl_reg(dev, pipe, &source, &val);
>> +     else if (IS_GEN5(dev) || IS_GEN6(dev))
>> +             ret = ilk_pipe_crc_ctl_reg(&source, &val);
>> +     else
>> +             ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source, &val);
>> +
>> +     if (ret)
>> +             return ret;
>> +
>> +     if (source) {
>> +             /*
>> +              * When IPS gets enabled, the pipe CRC changes. Since IPS gets
>> +              * enabled and disabled dynamically based on package C states,
>> +              * user space can't make reliable use of the CRCs, so let's just
>> +              * completely disable it.
>> +              */
>> +             hsw_disable_ips(crtc);
>> +     }
>> +
>> +     I915_WRITE(PIPE_CRC_CTL(pipe), val);
>> +     POSTING_READ(PIPE_CRC_CTL(pipe));
>> +
>> +     if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
>> +             if (IS_G4X(dev))
>> +                     g4x_undo_pipe_scramble_reset(dev, pipe);
>> +             else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
>> +                     vlv_undo_pipe_scramble_reset(dev, pipe);
>> +             else if (IS_HASWELL(dev) && pipe == PIPE_A)
>> +                     hsw_trans_edp_pipe_A_crc_wa(dev, false);
>> +
>> +             hsw_enable_ips(crtc);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>  static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>>                              enum intel_pipe_crc_source source)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[pipe];
>> -     struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev,
>> -                                                                     pipe));
>>       enum intel_display_power_domain power_domain;
>> -     u32 val = 0; /* shut up gcc */
>>       int ret;
>>
>>       if (pipe_crc->source == source)
>> @@ -648,20 +695,6 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>>               return -EIO;
>>       }
>>
>> -     if (IS_GEN2(dev))
>> -             ret = i8xx_pipe_crc_ctl_reg(&source, &val);
>> -     else if (INTEL_INFO(dev)->gen < 5)
>> -             ret = i9xx_pipe_crc_ctl_reg(dev, pipe, &source, &val);
>> -     else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
>> -             ret = vlv_pipe_crc_ctl_reg(dev, pipe, &source, &val);
>> -     else if (IS_GEN5(dev) || IS_GEN6(dev))
>> -             ret = ilk_pipe_crc_ctl_reg(&source, &val);
>> -     else
>> -             ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source, &val);
>> -
>> -     if (ret != 0)
>> -             goto out;
>> -
>>       /* none -> real source transition */
>>       if (source) {
>>               struct intel_pipe_crc_entry *entries;
>> @@ -677,14 +710,6 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>>                       goto out;
>>               }
>>
>> -             /*
>> -              * When IPS gets enabled, the pipe CRC changes. Since IPS gets
>> -              * enabled and disabled dynamically based on package C states,
>> -              * user space can't make reliable use of the CRCs, so let's just
>> -              * completely disable it.
>> -              */
>> -             hsw_disable_ips(crtc);
>> -
>>               spin_lock_irq(&pipe_crc->lock);
>>               kfree(pipe_crc->entries);
>>               pipe_crc->entries = entries;
>> @@ -693,10 +718,11 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>>               spin_unlock_irq(&pipe_crc->lock);
>>       }
>>
>> -     pipe_crc->source = source;
>> +     ret = do_set_crc_source(dev, pipe, source);
>> +     if (ret)
>> +             goto out;
>>
>> -     I915_WRITE(PIPE_CRC_CTL(pipe), val);
>> -     POSTING_READ(PIPE_CRC_CTL(pipe));
>> +     pipe_crc->source = source;
>>
>>       /* real source -> none transition */
>>       if (source == INTEL_PIPE_CRC_SOURCE_NONE) {
>> @@ -720,15 +746,6 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>>               spin_unlock_irq(&pipe_crc->lock);
>>
>>               kfree(entries);
>> -
>> -             if (IS_G4X(dev))
>> -                     g4x_undo_pipe_scramble_reset(dev, pipe);
>> -             else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
>> -                     vlv_undo_pipe_scramble_reset(dev, pipe);
>> -             else if (IS_HASWELL(dev) && pipe == PIPE_A)
>> -                     hsw_trans_edp_pipe_A_crc_wa(dev, false);
>> -
>> -             hsw_enable_ips(crtc);
>>       }
>>
>>       ret = 0;
>> @@ -821,6 +838,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;
>> @@ -949,3 +971,31 @@ 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;
>> +     enum intel_display_power_domain power_domain;
>> +     enum intel_pipe_crc_source source;
>> +     int ret;
>> +
>> +     if (display_crc_ctl_parse_source(source_name, &source) < 0) {
>> +             DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
>> +             return -EINVAL;
>> +     }
>> +
>> +     power_domain = POWER_DOMAIN_PIPE(crtc->index);
>> +     if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) {
>> +             DRM_DEBUG_KMS("Trying to capture CRC while pipe is off\n");
>> +             return -EIO;
>> +     }
>> +
>> +     ret = do_set_crc_source(crtc->dev, crtc->index, source);
>> +
>> +     intel_display_power_put(dev_priv, power_domain);
>> +
>> +     *values_cnt = 5;
>> +
>> +     return ret;
>> +}
>> --
>> 2.5.5
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux