| Message ID | 1483614756-3544-1-git-send-email-benjamin.gaignard@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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 >
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
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
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
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
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
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 */
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(+)