[PATCH] drm: sti: implement CRC capture API
Tomeu Vizoso
tomeu.vizoso at collabora.com
Fri Jan 6 08:22:29 UTC 2017
On 5 January 2017 at 12:12, Benjamin Gaignard
<benjamin.gaignard at linaro.org> wrote:
> Use CRC API to retrieve the 3 crc values from hardware.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard at linaro.org>
> ---
> 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.
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.
> Cc: Tomeu Vizoso <tomeu.vizoso at collabora.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> 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?).
> + 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?
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?
In any case, this should be more explicit for increased readability,
maybe add a macro for 0x1 with a descriptive name?
> + *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?
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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list