[PATCH] drm: sti: implement CRC capture API

[PATCH] drm: sti: implement CRC capture API

[Benjamin Gaignard]

Use CRC API to retrieve the 3 crc values from hardware.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@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.

Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Daniel Vetter <daniel.vetter@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])) {
+		ret = drm_crtc_add_crc_entry(crtc, false, 0, crcs);
+		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;
+
+	*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);
+
+	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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Thu, Jan 05, 2017 at 12:12:36PM +0100, Benjamin Gaignard wrote:
> Use CRC API to retrieve the 3 crc values from hardware.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@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.

+1 on moving the wake_up call into the drm_crtc_add_crc_entry function.
 
Care to spin that patch please?
-Daniel

> 
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Daniel Vetter <daniel.vetter@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])) {
> +		ret = drm_crtc_add_crc_entry(crtc, false, 0, crcs);
> +		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;
> +
> +	*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);
> +
> +	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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tomeu Vizoso]

On 5 January 2017 at 12:12, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> Use CRC API to retrieve the 3 crc values from hardware.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@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@collabora.com>
> Cc: Daniel Vetter <daniel.vetter@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Benjamin Gaignard]

2017-01-06 9:22 GMT+01:00 Tomeu Vizoso <tomeu.vizoso@collabora.com>:
> On 5 January 2017 at 12:12, Benjamin Gaignard
> <benjamin.gaignard@linaro.org> wrote:
>> Use CRC API to retrieve the 3 crc values from hardware.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@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.

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@collabora.com>
>> Cc: Daniel Vetter <daniel.vetter@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?).

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@lists.freedesktop.org
>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Fri, Jan 06, 2017 at 10:06:50AM +0100, Benjamin Gaignard wrote:
> 2017-01-06 9:22 GMT+01:00 Tomeu Vizoso <tomeu.vizoso@collabora.com>:
> > On 5 January 2017 at 12:12, Benjamin Gaignard
> > <benjamin.gaignard@linaro.org> wrote:
> >> Use CRC API to retrieve the 3 crc values from hardware.
> >>
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@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.
> 
> 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

Would be really good if you can give igt a spin on this ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Benjamin Gaignard]

2017-01-06 10:58 GMT+01:00 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Jan 06, 2017 at 10:06:50AM +0100, Benjamin Gaignard wrote:
>> 2017-01-06 9:22 GMT+01:00 Tomeu Vizoso <tomeu.vizoso@collabora.com>:
>> > On 5 January 2017 at 12:12, Benjamin Gaignard
>> > <benjamin.gaignard@linaro.org> wrote:
>> >> Use CRC API to retrieve the 3 crc values from hardware.
>> >>
>> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@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.
>>
>> 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
>
> Would be really good if you can give igt a spin on this ...

What is the status of IGT on ARM platforms ?
Last time (~6 months ago) I tested it, I had to include intel drm lib
and it just allow me
to check drm version. Does that have change ?


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

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

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

[Tomeu Vizoso]

On 01/06/2017 11:21 AM, Benjamin Gaignard wrote:
> 2017-01-06 10:58 GMT+01:00 Daniel Vetter <daniel@ffwll.ch>:
>> On Fri, Jan 06, 2017 at 10:06:50AM +0100, Benjamin Gaignard wrote:
>>> 2017-01-06 9:22 GMT+01:00 Tomeu Vizoso <tomeu.vizoso@collabora.com>:
>>>> On 5 January 2017 at 12:12, Benjamin Gaignard
>>>> <benjamin.gaignard@linaro.org> wrote:
>>>>> Use CRC API to retrieve the 3 crc values from hardware.
>>>>>
>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@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.
>>>
>>> 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
>>
>> Would be really good if you can give igt a spin on this ...
> 
> What is the status of IGT on ARM platforms ?
> Last time (~6 months ago) I tested it, I had to include intel drm lib
> and it just allow me
> to check drm version. Does that have change ?

Yup, this and other issues have been fixed. Have been running
tests/kms_pipe_crc_basic on a RK3288 Chromebook recently without any
problems.

Make sure though to rebase onto a recent drm-misc because of
e3d19d55676b ("drm: crc: Wait for a frame before returning from open()").

Regards,

Tomeu

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel