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