Re: [PATCH] drm: sti: implement CRC capture API

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

 



2017-01-06 9:22 GMT+01:00 Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>:
> On 5 January 2017 at 12:12, Benjamin Gaignard
> <benjamin.gaignard@xxxxxxxxxx> wrote:
>> Use CRC API to retrieve the 3 crc values from hardware.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx>
>> ---
>> This patch should be applied on top of drm-misc branch where Tomeu
>> has change crc.lock.
>>
>> I think that wake_up_interruptible() could also be call at the end
>> of drm_crtc_add_crc_entry() to avoid putting it in all drivers.
>
> Agreed, I can send such a patch if you don't have time.

I just finish to test the patches I will send them asap

>
> Btw, do any tests from iGT that make use of CRCs pass with this? If
> so, would be good to note it in the commit message.

I don't run IGT just modetest and cat on crtc-0/crc/data

>
>> Cc: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> ---
>>  drivers/gpu/drm/sti/sti_crtc.c  | 27 +++++++++++++++++++++++
>>  drivers/gpu/drm/sti/sti_mixer.c | 47 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/sti/sti_mixer.h |  4 ++++
>>  3 files changed, 78 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/sti/sti_crtc.c b/drivers/gpu/drm/sti/sti_crtc.c
>> index e992bed..47022b4 100644
>> --- a/drivers/gpu/drm/sti/sti_crtc.c
>> +++ b/drivers/gpu/drm/sti/sti_crtc.c
>> @@ -20,6 +20,8 @@
>>  #include "sti_vid.h"
>>  #include "sti_vtg.h"
>>
>> +#define CRC_SAMPLES 3
>> +
>>  static void sti_crtc_enable(struct drm_crtc *crtc)
>>  {
>>         struct sti_mixer *mixer = to_sti_mixer(crtc);
>> @@ -253,6 +255,8 @@ int sti_crtc_vblank_cb(struct notifier_block *nb,
>>         unsigned long flags;
>>         struct sti_private *priv;
>>         unsigned int pipe;
>> +       u32 crcs[CRC_SAMPLES];
>> +       int ret;
>>
>>         priv = crtc->dev->dev_private;
>>         pipe = drm_crtc_index(crtc);
>> @@ -275,6 +279,12 @@ int sti_crtc_vblank_cb(struct notifier_block *nb,
>>         }
>>         spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>
>> +       if (!sti_mixer_read_crcs(mixer, &crcs[0], &crcs[1], &crcs[2])) {
>
> nit: I would place the return value of that function into ret, so it's
> easier to read and easier to instrument when debugging (and maybe log
> a debug message if it fails?).

sti_mixer_read_crcs() is called even if crc isn't enabled so status
bit could be set
at 0 without being an error...

>> +               ret = drm_crtc_add_crc_entry(crtc, false, 0, crcs);
>
> Doesn't the frame number returned by drm_accurate_vblank_count()
> correspond to the fb contents characterized by these crcs?

My driver doesn't implement get_vblank_timestamp() so
drm_accurate_vblank_count()
won't work.

> When the CRCs come from a sink, we don't have a good way of knowing to
> which frame count they correspond, but in this case I would expect
> that the mixer was programmed with the new fb contents for this vblank
> count.
>
> Tests can be more extensive if there are frame numbers.
>
>> +               if (!ret)
>> +                       wake_up_interruptible(&crtc->crc.wq);
>> +       }
>> +
>>         if (mixer->status == STI_MIXER_DISABLING) {
>>                 struct drm_plane *p;
>>
>> @@ -343,6 +353,22 @@ static int sti_crtc_late_register(struct drm_crtc *crtc)
>>         return 0;
>>  }
>>
>> +int sti_set_crc_source(struct drm_crtc *crtc, const char *source,
>> +                      size_t *values_cnt)
>> +{
>> +       struct sti_mixer *mixer = to_sti_mixer(crtc);
>> +
>> +       *values_cnt = CRC_SAMPLES;
>> +
>> +       if (!source)
>> +               return sti_mixer_set_crc_status(mixer, false);
>> +
>> +       if (source && strcmp(source, "auto") == 0)
>> +               return sti_mixer_set_crc_status(mixer, true);
>> +
>> +       return -EINVAL;
>> +}
>> +
>>  static const struct drm_crtc_funcs sti_crtc_funcs = {
>>         .set_config = drm_atomic_helper_set_config,
>>         .page_flip = drm_atomic_helper_page_flip,
>> @@ -352,6 +378,7 @@ static int sti_crtc_late_register(struct drm_crtc *crtc)
>>         .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>>         .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>         .late_register = sti_crtc_late_register,
>> +       .set_crc_source = sti_set_crc_source,
>>  };
>>
>>  bool sti_crtc_is_main(struct drm_crtc *crtc)
>> diff --git a/drivers/gpu/drm/sti/sti_mixer.c b/drivers/gpu/drm/sti/sti_mixer.c
>> index 4ddc58f..4eb5cc5 100644
>> --- a/drivers/gpu/drm/sti/sti_mixer.c
>> +++ b/drivers/gpu/drm/sti/sti_mixer.c
>> @@ -27,6 +27,13 @@
>>  #define GAM_MIXER_ACT      0x38
>>  #define GAM_MIXER_MBP      0x3C
>>  #define GAM_MIXER_MX0      0x80
>> +#define GAM_MIXER_MISR_CTL 0xA0
>> +#define GAM_MIXER_MISR_STA 0xA4
>> +#define GAM_MIXER_SIGN1    0xA8
>> +#define GAM_MIXER_SIGN2    0xAC
>> +#define GAM_MIXER_SIGN3    0xB0
>> +#define GAM_MIXER_MISR_AVO 0xB4
>> +#define GAM_MIXER_MISR_AVS 0xB8
>>
>>  /* id for depth of CRB reg */
>>  #define GAM_DEPTH_VID0_ID  1
>> @@ -162,6 +169,13 @@ static int mixer_dbg_show(struct seq_file *s, void *arg)
>>         DBGFS_DUMP(GAM_MIXER_MBP);
>>         DBGFS_DUMP(GAM_MIXER_MX0);
>>         mixer_dbg_mxn(s, mixer->regs + GAM_MIXER_MX0);
>> +       DBGFS_DUMP(GAM_MIXER_MISR_CTL);
>> +       DBGFS_DUMP(GAM_MIXER_MISR_STA);
>> +       DBGFS_DUMP(GAM_MIXER_SIGN1);
>> +       DBGFS_DUMP(GAM_MIXER_SIGN2);
>> +       DBGFS_DUMP(GAM_MIXER_SIGN3);
>> +       DBGFS_DUMP(GAM_MIXER_MISR_AVO);
>> +       DBGFS_DUMP(GAM_MIXER_MISR_AVS);
>>         seq_puts(s, "\n");
>>
>>         return 0;
>> @@ -202,6 +216,36 @@ int sti_mixer_debugfs_init(struct sti_mixer *mixer, struct drm_minor *minor)
>>                                         minor->debugfs_root, minor);
>>  }
>>
>> +int sti_mixer_set_crc_status(struct sti_mixer *mixer, bool enable)
>> +{
>> +       if (enable) {
>> +               sti_mixer_reg_read(mixer, GAM_MIXER_MISR_STA);
>> +               sti_mixer_reg_read(mixer, GAM_MIXER_SIGN1);
>> +               sti_mixer_reg_read(mixer, GAM_MIXER_SIGN2);
>> +               sti_mixer_reg_read(mixer, GAM_MIXER_SIGN3);
>> +               sti_mixer_reg_write(mixer, GAM_MIXER_MISR_CTL, 0x0F);
>> +       } else {
>> +               sti_mixer_reg_write(mixer, GAM_MIXER_MISR_CTL, 0x00);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +int sti_mixer_read_crcs(struct sti_mixer *mixer,
>> +                       u32 *sign1, u32 *sign2, u32 *sign3)
>> +{
>> +       u32 status = sti_mixer_reg_read(mixer, GAM_MIXER_MISR_STA);
>> +
>> +       if (!(status & 0x1))
>> +               return -EINVAL;
>
> Does it mean that the HW isn't able to return a CRC yet? If so, maybe
> -EAGAIN would be more appropriate?

Yes I will change that

> In any case, this should be more explicit for increased readability,
> maybe add a macro for 0x1 with a descriptive name?

Done in v2

>
>> +       *sign1 = sti_mixer_reg_read(mixer, GAM_MIXER_SIGN1);
>> +       *sign2 = sti_mixer_reg_read(mixer, GAM_MIXER_SIGN2);
>> +       *sign3 = sti_mixer_reg_read(mixer, GAM_MIXER_SIGN3);
>
> Just out of curiosity: what is sign meant to mean here?

it means signature i.e. it is the result of R, G, B (or Cr,Cb,Luma) computation.

>
> Looks very good, thanks!
>
> Tomeu
>
>> +
>> +       return 0;
>> +}
>> +
>>  void sti_mixer_set_background_status(struct sti_mixer *mixer, bool enable)
>>  {
>>         u32 val = sti_mixer_reg_read(mixer, GAM_MIXER_CTL);
>> @@ -301,6 +345,9 @@ int sti_mixer_active_video_area(struct sti_mixer *mixer,
>>         sti_mixer_reg_write(mixer, GAM_MIXER_AVO, ydo << 16 | xdo);
>>         sti_mixer_reg_write(mixer, GAM_MIXER_AVS, yds << 16 | xds);
>>
>> +       sti_mixer_reg_write(mixer, GAM_MIXER_MISR_AVO, ydo << 16 | xdo);
>> +       sti_mixer_reg_write(mixer, GAM_MIXER_MISR_AVS, yds << 16 | xds);
>> +
>>         sti_mixer_set_background_color(mixer, bkg_color);
>>
>>         sti_mixer_set_background_area(mixer, mode);
>> diff --git a/drivers/gpu/drm/sti/sti_mixer.h b/drivers/gpu/drm/sti/sti_mixer.h
>> index 830a3c4..b16feb1 100644
>> --- a/drivers/gpu/drm/sti/sti_mixer.h
>> +++ b/drivers/gpu/drm/sti/sti_mixer.h
>> @@ -55,6 +55,10 @@ int sti_mixer_active_video_area(struct sti_mixer *mixer,
>>
>>  void sti_mixer_set_background_status(struct sti_mixer *mixer, bool enable);
>>
>> +int sti_mixer_set_crc_status(struct sti_mixer *mixer, bool enable);
>> +int sti_mixer_read_crcs(struct sti_mixer *mixer,
>> +                       u32 *sign1, u32 *sign2, u32 *sign3);
>> +
>>  int sti_mixer_debugfs_init(struct sti_mixer *mixer, struct drm_minor *minor);
>>
>>  /* depth in Cross-bar control = z order */
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux