2019-08-01 11:40:10

by Liviu Dudau

Subject: drm/komeda: Add support for generation of CRC data per frame.

Komeda has support to generate per-frame CRC values in the DOU
backend subsystem. Implement necessary hooks to expose the CRC
"control" and "data" file over debugfs and program the DOUx_BS
accordingly.

This patch makes use of PL1 (programmable line 1) interrupt to
know when the CRC generation has finished.

Patch is also dependent on the series that adds dual-link support
for komeda: https://patchwork.freedesktop.org/series/62280/

Cc: "james qian wang (Arm Technology China)" <[email protected]>
Signed-off-by: Liviu Dudau <[email protected]>
---
.../arm/display/komeda/d71/d71_component.c | 2 +-
.../gpu/drm/arm/display/komeda/d71/d71_dev.c | 29 ++++++++-
.../gpu/drm/arm/display/komeda/komeda_crtc.c | 61 ++++++++++++++++++-
.../gpu/drm/arm/display/komeda/komeda_dev.h | 2 +
.../gpu/drm/arm/display/komeda/komeda_kms.h | 3 +
.../drm/arm/display/komeda/komeda_pipeline.h | 1 +
6 files changed, 94 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
index 55a8cc94808a1..3c45468848ee4 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
@@ -1061,7 +1061,7 @@ static void d71_timing_ctrlr_update(struct komeda_component *c,
malidp_write32(reg, BS_PREFETCH_LINE, D71_DEFAULT_PREPRETCH_LINE);

/* configure bs control register */
- value = BS_CTRL_EN | BS_CTRL_VM;
+ value = BS_CTRL_EN | BS_CTRL_VM | BS_CTRL_CRC;
if (c->pipeline->dual_link) {
malidp_write32(reg, BS_DRIFT_TO, hfront_porch + 16);
value |= BS_CTRL_DL;
diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
index d567ab7ed314e..05bfd9891c540 100644
--- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
@@ -115,6 +115,8 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
raw_status = malidp_read32(reg, BLK_IRQ_RAW_STATUS);
if (raw_status & DOU_IRQ_PL0)
evts |= KOMEDA_EVENT_VSYNC;
+ if (raw_status & DOU_IRQ_PL1)
+ evts |= KOMEDA_EVENT_CRCDONE;
if (raw_status & DOU_IRQ_UND)
evts |= KOMEDA_EVENT_URUN;

@@ -149,7 +151,7 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)

static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
{
- u32 evts = 0ULL;
+ u64 evts = 0ULL;

if (gcu_status & (GLB_IRQ_STATUS_LPU0 | GLB_IRQ_STATUS_LPU1))
evts |= get_lpu_event(d71_pipeline);
@@ -163,6 +165,26 @@ static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
return evts;
}

+static void get_frame_crcs(struct d71_pipeline *d71_pipeline, u32 pipe,
+ struct komeda_events *evts)
+{
+ if (evts->pipes[pipe] & KOMEDA_EVENT_CRCDONE) {
+ struct komeda_component *c;
+
+ c = komeda_pipeline_get_component(&d71_pipeline->base,
+ KOMEDA_COMPONENT_TIMING_CTRLR);
+ if (!c)
+ return;
+
+ evts->crcs[pipe][0] = malidp_read32(c->reg, BS_CRC0_LOW);
+ evts->crcs[pipe][1] = malidp_read32(c->reg, BS_CRC0_HIGH);
+ if (d71_pipeline->base.dual_link) {
+ evts->crcs[pipe][2] = malidp_read32(c->reg, BS_CRC1_LOW);
+ evts->crcs[pipe][3] = malidp_read32(c->reg, BS_CRC1_HIGH);
+ }
+ }
+}
+
static irqreturn_t
d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
{
@@ -195,6 +217,9 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
if (gcu_status & GLB_IRQ_STATUS_PIPE1)
evts->pipes[1] |= get_pipeline_event(d71->pipes[1], gcu_status);

+ get_frame_crcs(d71->pipes[0], 0, evts);
+ get_frame_crcs(d71->pipes[1], 1, evts);
+
return gcu_status ? IRQ_HANDLED : IRQ_NONE;
}

@@ -202,7 +227,7 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
GCU_IRQ_MODE | GCU_IRQ_ERR)
#define ENABLED_LPU_IRQS (LPU_IRQ_IBSY | LPU_IRQ_ERR | LPU_IRQ_EOW)
#define ENABLED_CU_IRQS (CU_IRQ_OVR | CU_IRQ_ERR)
-#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR)
+#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR | DOU_IRQ_PL1)

static int d71_enable_irq(struct komeda_dev *mdev)
{
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index fa9a4593bb375..4b9f5d33e999d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -207,10 +207,13 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
drm_crtc_send_vblank_event(crtc, event);
} else {
DRM_WARN("CRTC[%d]: FLIP happen but no pending commit.\n",
- drm_crtc_index(&kcrtc->base));
+ drm_crtc_index(crtc));
}
spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
}
+
+ if ((kcrtc->crc_enabled) && (events & KOMEDA_EVENT_CRCDONE))
+ drm_crtc_add_crc_entry(crtc, false, 0, evts->crcs[kcrtc->master->id]);
}

static void
@@ -487,6 +490,59 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc)
mdev->funcs->on_off_vblank(mdev, kcrtc->master->id, false);
}

+static const char * const komeda_pipe_crc_sources[] = {"auto"};
+
+static const char *const *komeda_crtc_get_crc_sources(struct drm_crtc *crtc,
+ size_t *count)
+{
+ *count = ARRAY_SIZE(komeda_pipe_crc_sources);
+ return komeda_pipe_crc_sources;
+}
+
+static int komeda_crtc_parse_crc_source(const char *source)
+{
+ if (!source)
+ return 0;
+ if (strcmp(source, "auto") == 0)
+ return 1;
+
+ return -EINVAL;
+}
+
+static int komeda_crtc_verify_crc_source(struct drm_crtc *crtc,
+ const char *source_name,
+ size_t *values_count)
+{
+ struct komeda_crtc *kcrtc = to_kcrtc(crtc);
+ int source = komeda_crtc_parse_crc_source(source_name);
+
+ if (source < 0) {
+ DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
+ drm_crtc_index(crtc));
+ return -EINVAL;
+ }
+
+ *values_count = kcrtc->master->dual_link ? 4 : 2;
+
+ return 0;
+}
+
+static int komeda_crtc_set_crc_source(struct drm_crtc *crtc, const char *source)
+{
+ struct komeda_crtc *kcrtc = to_kcrtc(crtc);
+ int src = komeda_crtc_parse_crc_source(source);
+
+ if (src < 0) {
+ DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
+ drm_crtc_index(crtc));
+ return -EINVAL;
+ }
+
+ kcrtc->crc_enabled = src & 1;
+
+ return 0;
+}
+
static const struct drm_crtc_funcs komeda_crtc_funcs = {
.gamma_set = drm_atomic_helper_legacy_gamma_set,
.destroy = drm_crtc_cleanup,
@@ -497,6 +553,9 @@ static const struct drm_crtc_funcs komeda_crtc_funcs = {
.atomic_destroy_state = komeda_crtc_atomic_destroy_state,
.enable_vblank = komeda_crtc_vblank_enable,
.disable_vblank = komeda_crtc_vblank_disable,
+ .set_crc_source = komeda_crtc_set_crc_source,
+ .verify_crc_source = komeda_crtc_verify_crc_source,
+ .get_crc_sources = komeda_crtc_get_crc_sources,
};

int komeda_kms_setup_crtcs(struct komeda_kms_dev *kms,
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
index d1c86b6174c80..244227b945f63 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
@@ -20,6 +20,7 @@
#define KOMEDA_EVENT_OVR BIT_ULL(4)
#define KOMEDA_EVENT_EOW BIT_ULL(5)
#define KOMEDA_EVENT_MODE BIT_ULL(6)
+#define KOMEDA_EVENT_CRCDONE BIT_ULL(7)

#define KOMEDA_ERR_TETO BIT_ULL(14)
#define KOMEDA_ERR_TEMR BIT_ULL(15)
@@ -69,6 +70,7 @@ struct komeda_dev;
struct komeda_events {
u64 global;
u64 pipes[KOMEDA_MAX_PIPELINES];
+ u32 crcs[KOMEDA_MAX_PIPELINES][KOMEDA_MAX_CRCS];
};

/**
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 45c498e15e7ae..de7c93b2d0a11 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -84,6 +84,9 @@ struct komeda_crtc {

/** @disable_done: this flip_done is for tracing the disable */
struct completion *disable_done;
+
+ /** @crc_enabled: true if per-frame generation of CRC is enabled */
+ bool crc_enabled;
};

/**
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
index a7a84e66549d6..dfe2482c6274b 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
@@ -16,6 +16,7 @@
#define KOMEDA_PIPELINE_MAX_LAYERS 4
#define KOMEDA_PIPELINE_MAX_SCALERS 2
#define KOMEDA_COMPONENT_N_INPUTS 5
+#define KOMEDA_MAX_CRCS 4

/* pipeline component IDs */
enum {
--
2.22.0

2019-08-02 10:16:07

by Brian Starkey

Subject: Re: drm/komeda: Add support for generation of CRC data per frame.

Hi Daniel,

On Fri, Aug 02, 2019 at 11:52:11AM +0200, Daniel Vetter wrote:
> On Fri, Aug 2, 2019 at 11:39 AM Brian Starkey <[email protected]> wrote:
> >
> > Hi Liviu,
> >
> > On Thu, Aug 01, 2019 at 11:42:31AM +0100, Liviu Dudau wrote:
> > > Komeda has support to generate per-frame CRC values in the DOU
> > > backend subsystem. Implement necessary hooks to expose the CRC
> > > "control" and "data" file over debugfs and program the DOUx_BS
> > > accordingly.
> > >
> > > This patch makes use of PL1 (programmable line 1) interrupt to
> > > know when the CRC generation has finished.
> > >
> > > Patch is also dependent on the series that adds dual-link support
> > > for komeda: https://patchwork.freedesktop.org/series/62280/
> > >
> > > Cc: "james qian wang (Arm Technology China)" <[email protected]>
> > > Signed-off-by: Liviu Dudau <[email protected]>
> > > ---
> > > .../arm/display/komeda/d71/d71_component.c | 2 +-
> > > .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 29 ++++++++-
> > > .../gpu/drm/arm/display/komeda/komeda_crtc.c | 61 ++++++++++++++++++-
> > > .../gpu/drm/arm/display/komeda/komeda_dev.h | 2 +
> > > .../gpu/drm/arm/display/komeda/komeda_kms.h | 3 +
> > > .../drm/arm/display/komeda/komeda_pipeline.h | 1 +
> > > 6 files changed, 94 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > index 55a8cc94808a1..3c45468848ee4 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > @@ -1061,7 +1061,7 @@ static void d71_timing_ctrlr_update(struct komeda_component *c,
> > > malidp_write32(reg, BS_PREFETCH_LINE, D71_DEFAULT_PREPRETCH_LINE);
> > >
> > > /* configure bs control register */
> > > - value = BS_CTRL_EN | BS_CTRL_VM;
> > > + value = BS_CTRL_EN | BS_CTRL_VM | BS_CTRL_CRC;
> > > if (c->pipeline->dual_link) {
> > > malidp_write32(reg, BS_DRIFT_TO, hfront_porch + 16);
> > > value |= BS_CTRL_DL;
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > index d567ab7ed314e..05bfd9891c540 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > @@ -115,6 +115,8 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
> > > raw_status = malidp_read32(reg, BLK_IRQ_RAW_STATUS);
> > > if (raw_status & DOU_IRQ_PL0)
> > > evts |= KOMEDA_EVENT_VSYNC;
> > > + if (raw_status & DOU_IRQ_PL1)
> > > + evts |= KOMEDA_EVENT_CRCDONE;
> > > if (raw_status & DOU_IRQ_UND)
> > > evts |= KOMEDA_EVENT_URUN;
> > >
> > > @@ -149,7 +151,7 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
> > >
> > > static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> > > {
> > > - u32 evts = 0ULL;
> > > + u64 evts = 0ULL;
> > >
> > > if (gcu_status & (GLB_IRQ_STATUS_LPU0 | GLB_IRQ_STATUS_LPU1))
> > > evts |= get_lpu_event(d71_pipeline);
> > > @@ -163,6 +165,26 @@ static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> > > return evts;
> > > }
> > >
> > > +static void get_frame_crcs(struct d71_pipeline *d71_pipeline, u32 pipe,
> > > + struct komeda_events *evts)
> > > +{
> > > + if (evts->pipes[pipe] & KOMEDA_EVENT_CRCDONE) {
> > > + struct komeda_component *c;
> > > +
> > > + c = komeda_pipeline_get_component(&d71_pipeline->base,
> > > + KOMEDA_COMPONENT_TIMING_CTRLR);
> > > + if (!c)
> > > + return;
> > > +
> > > + evts->crcs[pipe][0] = malidp_read32(c->reg, BS_CRC0_LOW);
> > > + evts->crcs[pipe][1] = malidp_read32(c->reg, BS_CRC0_HIGH);
> > > + if (d71_pipeline->base.dual_link) {
> > > + evts->crcs[pipe][2] = malidp_read32(c->reg, BS_CRC1_LOW);
> > > + evts->crcs[pipe][3] = malidp_read32(c->reg, BS_CRC1_HIGH);
> > > + }
> > > + }
> > > +}
> > > +
> > > static irqreturn_t
> > > d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > {
> > > @@ -195,6 +217,9 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > if (gcu_status & GLB_IRQ_STATUS_PIPE1)
> > > evts->pipes[1] |= get_pipeline_event(d71->pipes[1], gcu_status);
> > >
> > > + get_frame_crcs(d71->pipes[0], 0, evts);
> > > + get_frame_crcs(d71->pipes[1], 1, evts);
> > > +
> > > return gcu_status ? IRQ_HANDLED : IRQ_NONE;
> > > }
> > >
> > > @@ -202,7 +227,7 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > GCU_IRQ_MODE | GCU_IRQ_ERR)
> > > #define ENABLED_LPU_IRQS (LPU_IRQ_IBSY | LPU_IRQ_ERR | LPU_IRQ_EOW)
> > > #define ENABLED_CU_IRQS (CU_IRQ_OVR | CU_IRQ_ERR)
> > > -#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR)
> > > +#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR | DOU_IRQ_PL1)
> > >
> > > static int d71_enable_irq(struct komeda_dev *mdev)
> > > {
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > index fa9a4593bb375..4b9f5d33e999d 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > @@ -207,10 +207,13 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> > > drm_crtc_send_vblank_event(crtc, event);
> > > } else {
> > > DRM_WARN("CRTC[%d]: FLIP happen but no pending commit.\n",
> > > - drm_crtc_index(&kcrtc->base));
> > > + drm_crtc_index(crtc));
> > > }
> > > spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > > }
> > > +
> > > + if ((kcrtc->crc_enabled) && (events & KOMEDA_EVENT_CRCDONE))
> > > + drm_crtc_add_crc_entry(crtc, false, 0, evts->crcs[kcrtc->master->id]);
> > > }
> > >
> > > static void
> > > @@ -487,6 +490,59 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc)
> > > mdev->funcs->on_off_vblank(mdev, kcrtc->master->id, false);
> > > }
> > >
> > > +static const char * const komeda_pipe_crc_sources[] = {"auto"};
> > > +
> > > +static const char *const *komeda_crtc_get_crc_sources(struct drm_crtc *crtc,
> > > + size_t *count)
> > > +{
> > > + *count = ARRAY_SIZE(komeda_pipe_crc_sources);
> > > + return komeda_pipe_crc_sources;
> > > +}
> > > +
> > > +static int komeda_crtc_parse_crc_source(const char *source)
> > > +{
> > > + if (!source)
> > > + return 0;
> > > + if (strcmp(source, "auto") == 0)
> > > + return 1;
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static int komeda_crtc_verify_crc_source(struct drm_crtc *crtc,
> > > + const char *source_name,
> > > + size_t *values_count)
> > > +{
> > > + struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> > > + int source = komeda_crtc_parse_crc_source(source_name);
> > > +
> > > + if (source < 0) {
> > > + DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
> > > + drm_crtc_index(crtc));
> > > + return -EINVAL;
> > > + }
> > > +
> > > + *values_count = kcrtc->master->dual_link ? 4 : 2;
> >
> > Can CRC generation continue across a modeset? If so I think we could
> > end up with a case where dual_link changes while CRC is active. Maybe
> > we should just always return 4 values, but set the third and fourth
> > values to 0 in the event handler, if dual-link isn't active.
>
> Modeset is allowed to destry the crc setup. Maybe not the smartest
> decision (it prevents us from making sure the first frame is perfect),
> but pretty deeply in-grained into tests by now. If we want to move
> this I think first step would be to add new basic testcases to igt to
> validate crc generation against modesets (maybe start with dpms
> off/on, then suspend/resume, then full modesets). Really not sure
> there's a need, and I've seen too many cases like this where crc
> generation changes depending upon modeset (bpp, routing, ...) to
> assume we can just make it work.
>
> I'd not bother and leave things as-is.

OK good, glad we're not alone. Do we need to do anything specific to
stop/reconfigure generation over modeset, or we just rely on userspace
to reconfigure?

Thanks,
-Brian

> -Daniel
> >
> > Cheers,
> > -Brian
> >

2019-08-02 13:28:33

by Brian Starkey

Subject: Re: drm/komeda: Add support for generation of CRC data per frame.

Hi Liviu,

On Thu, Aug 01, 2019 at 11:42:31AM +0100, Liviu Dudau wrote:
> Komeda has support to generate per-frame CRC values in the DOU
> backend subsystem. Implement necessary hooks to expose the CRC
> "control" and "data" file over debugfs and program the DOUx_BS
> accordingly.
>
> This patch makes use of PL1 (programmable line 1) interrupt to
> know when the CRC generation has finished.
>
> Patch is also dependent on the series that adds dual-link support
> for komeda: https://patchwork.freedesktop.org/series/62280/
>
> Cc: "james qian wang (Arm Technology China)" <[email protected]>
> Signed-off-by: Liviu Dudau <[email protected]>
> ---
> .../arm/display/komeda/d71/d71_component.c | 2 +-
> .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 29 ++++++++-
> .../gpu/drm/arm/display/komeda/komeda_crtc.c | 61 ++++++++++++++++++-
> .../gpu/drm/arm/display/komeda/komeda_dev.h | 2 +
> .../gpu/drm/arm/display/komeda/komeda_kms.h | 3 +
> .../drm/arm/display/komeda/komeda_pipeline.h | 1 +
> 6 files changed, 94 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> index 55a8cc94808a1..3c45468848ee4 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> @@ -1061,7 +1061,7 @@ static void d71_timing_ctrlr_update(struct komeda_component *c,
> malidp_write32(reg, BS_PREFETCH_LINE, D71_DEFAULT_PREPRETCH_LINE);
>
> /* configure bs control register */
> - value = BS_CTRL_EN | BS_CTRL_VM;
> + value = BS_CTRL_EN | BS_CTRL_VM | BS_CTRL_CRC;
> if (c->pipeline->dual_link) {
> malidp_write32(reg, BS_DRIFT_TO, hfront_porch + 16);
> value |= BS_CTRL_DL;
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> index d567ab7ed314e..05bfd9891c540 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> @@ -115,6 +115,8 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
> raw_status = malidp_read32(reg, BLK_IRQ_RAW_STATUS);
> if (raw_status & DOU_IRQ_PL0)
> evts |= KOMEDA_EVENT_VSYNC;
> + if (raw_status & DOU_IRQ_PL1)
> + evts |= KOMEDA_EVENT_CRCDONE;
> if (raw_status & DOU_IRQ_UND)
> evts |= KOMEDA_EVENT_URUN;
>
> @@ -149,7 +151,7 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
>
> static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> {
> - u32 evts = 0ULL;
> + u64 evts = 0ULL;
>
> if (gcu_status & (GLB_IRQ_STATUS_LPU0 | GLB_IRQ_STATUS_LPU1))
> evts |= get_lpu_event(d71_pipeline);
> @@ -163,6 +165,26 @@ static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> return evts;
> }
>
> +static void get_frame_crcs(struct d71_pipeline *d71_pipeline, u32 pipe,
> + struct komeda_events *evts)
> +{
> + if (evts->pipes[pipe] & KOMEDA_EVENT_CRCDONE) {
> + struct komeda_component *c;
> +
> + c = komeda_pipeline_get_component(&d71_pipeline->base,
> + KOMEDA_COMPONENT_TIMING_CTRLR);
> + if (!c)
> + return;
> +
> + evts->crcs[pipe][0] = malidp_read32(c->reg, BS_CRC0_LOW);
> + evts->crcs[pipe][1] = malidp_read32(c->reg, BS_CRC0_HIGH);
> + if (d71_pipeline->base.dual_link) {
> + evts->crcs[pipe][2] = malidp_read32(c->reg, BS_CRC1_LOW);
> + evts->crcs[pipe][3] = malidp_read32(c->reg, BS_CRC1_HIGH);
> + }
> + }
> +}
> +
> static irqreturn_t
> d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> {
> @@ -195,6 +217,9 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> if (gcu_status & GLB_IRQ_STATUS_PIPE1)
> evts->pipes[1] |= get_pipeline_event(d71->pipes[1], gcu_status);
>
> + get_frame_crcs(d71->pipes[0], 0, evts);
> + get_frame_crcs(d71->pipes[1], 1, evts);
> +
> return gcu_status ? IRQ_HANDLED : IRQ_NONE;
> }
>
> @@ -202,7 +227,7 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> GCU_IRQ_MODE | GCU_IRQ_ERR)
> #define ENABLED_LPU_IRQS (LPU_IRQ_IBSY | LPU_IRQ_ERR | LPU_IRQ_EOW)
> #define ENABLED_CU_IRQS (CU_IRQ_OVR | CU_IRQ_ERR)
> -#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR)
> +#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR | DOU_IRQ_PL1)
>
> static int d71_enable_irq(struct komeda_dev *mdev)
> {
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index fa9a4593bb375..4b9f5d33e999d 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -207,10 +207,13 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> drm_crtc_send_vblank_event(crtc, event);
> } else {
> DRM_WARN("CRTC[%d]: FLIP happen but no pending commit.\n",
> - drm_crtc_index(&kcrtc->base));
> + drm_crtc_index(crtc));
> }
> spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> }
> +
> + if ((kcrtc->crc_enabled) && (events & KOMEDA_EVENT_CRCDONE))
> + drm_crtc_add_crc_entry(crtc, false, 0, evts->crcs[kcrtc->master->id]);
> }
>
> static void
> @@ -487,6 +490,59 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc)
> mdev->funcs->on_off_vblank(mdev, kcrtc->master->id, false);
> }
>
> +static const char * const komeda_pipe_crc_sources[] = {"auto"};
> +
> +static const char *const *komeda_crtc_get_crc_sources(struct drm_crtc *crtc,
> + size_t *count)
> +{
> + *count = ARRAY_SIZE(komeda_pipe_crc_sources);
> + return komeda_pipe_crc_sources;
> +}
> +
> +static int komeda_crtc_parse_crc_source(const char *source)
> +{
> + if (!source)
> + return 0;
> + if (strcmp(source, "auto") == 0)
> + return 1;
> +
> + return -EINVAL;
> +}
> +
> +static int komeda_crtc_verify_crc_source(struct drm_crtc *crtc,
> + const char *source_name,
> + size_t *values_count)
> +{
> + struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> + int source = komeda_crtc_parse_crc_source(source_name);
> +
> + if (source < 0) {
> + DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
> + drm_crtc_index(crtc));
> + return -EINVAL;
> + }
> +
> + *values_count = kcrtc->master->dual_link ? 4 : 2;

Can CRC generation continue across a modeset? If so I think we could
end up with a case where dual_link changes while CRC is active. Maybe
we should just always return 4 values, but set the third and fourth
values to 0 in the event handler, if dual-link isn't active.

Cheers,
-Brian

> +
> + return 0;
> +}
> +
> +static int komeda_crtc_set_crc_source(struct drm_crtc *crtc, const char *source)
> +{
> + struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> + int src = komeda_crtc_parse_crc_source(source);
> +
> + if (src < 0) {
> + DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
> + drm_crtc_index(crtc));
> + return -EINVAL;
> + }
> +
> + kcrtc->crc_enabled = src & 1;
> +
> + return 0;
> +}
> +
> static const struct drm_crtc_funcs komeda_crtc_funcs = {
> .gamma_set = drm_atomic_helper_legacy_gamma_set,
> .destroy = drm_crtc_cleanup,
> @@ -497,6 +553,9 @@ static const struct drm_crtc_funcs komeda_crtc_funcs = {
> .atomic_destroy_state = komeda_crtc_atomic_destroy_state,
> .enable_vblank = komeda_crtc_vblank_enable,
> .disable_vblank = komeda_crtc_vblank_disable,
> + .set_crc_source = komeda_crtc_set_crc_source,
> + .verify_crc_source = komeda_crtc_verify_crc_source,
> + .get_crc_sources = komeda_crtc_get_crc_sources,
> };
>
> int komeda_kms_setup_crtcs(struct komeda_kms_dev *kms,
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> index d1c86b6174c80..244227b945f63 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> @@ -20,6 +20,7 @@
> #define KOMEDA_EVENT_OVR BIT_ULL(4)
> #define KOMEDA_EVENT_EOW BIT_ULL(5)
> #define KOMEDA_EVENT_MODE BIT_ULL(6)
> +#define KOMEDA_EVENT_CRCDONE BIT_ULL(7)
>
> #define KOMEDA_ERR_TETO BIT_ULL(14)
> #define KOMEDA_ERR_TEMR BIT_ULL(15)
> @@ -69,6 +70,7 @@ struct komeda_dev;
> struct komeda_events {
> u64 global;
> u64 pipes[KOMEDA_MAX_PIPELINES];
> + u32 crcs[KOMEDA_MAX_PIPELINES][KOMEDA_MAX_CRCS];
> };
>
> /**
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> index 45c498e15e7ae..de7c93b2d0a11 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> @@ -84,6 +84,9 @@ struct komeda_crtc {
>
> /** @disable_done: this flip_done is for tracing the disable */
> struct completion *disable_done;
> +
> + /** @crc_enabled: true if per-frame generation of CRC is enabled */
> + bool crc_enabled;
> };
>
> /**
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> index a7a84e66549d6..dfe2482c6274b 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> @@ -16,6 +16,7 @@
> #define KOMEDA_PIPELINE_MAX_LAYERS 4
> #define KOMEDA_PIPELINE_MAX_SCALERS 2
> #define KOMEDA_COMPONENT_N_INPUTS 5
> +#define KOMEDA_MAX_CRCS 4
>
> /* pipeline component IDs */
> enum {
> --
> 2.22.0
>

2019-08-02 18:34:35

by Daniel Vetter

Subject: Re: drm/komeda: Add support for generation of CRC data per frame.

On Fri, Aug 2, 2019 at 11:39 AM Brian Starkey <[email protected]> wrote:
>
> Hi Liviu,
>
> On Thu, Aug 01, 2019 at 11:42:31AM +0100, Liviu Dudau wrote:
> > Komeda has support to generate per-frame CRC values in the DOU
> > backend subsystem. Implement necessary hooks to expose the CRC
> > "control" and "data" file over debugfs and program the DOUx_BS
> > accordingly.
> >
> > This patch makes use of PL1 (programmable line 1) interrupt to
> > know when the CRC generation has finished.
> >
> > Patch is also dependent on the series that adds dual-link support
> > for komeda: https://patchwork.freedesktop.org/series/62280/
> >
> > Cc: "james qian wang (Arm Technology China)" <[email protected]>
> > Signed-off-by: Liviu Dudau <[email protected]>
> > ---
> > .../arm/display/komeda/d71/d71_component.c | 2 +-
> > .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 29 ++++++++-
> > .../gpu/drm/arm/display/komeda/komeda_crtc.c | 61 ++++++++++++++++++-
> > .../gpu/drm/arm/display/komeda/komeda_dev.h | 2 +
> > .../gpu/drm/arm/display/komeda/komeda_kms.h | 3 +
> > .../drm/arm/display/komeda/komeda_pipeline.h | 1 +
> > 6 files changed, 94 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > index 55a8cc94808a1..3c45468848ee4 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > @@ -1061,7 +1061,7 @@ static void d71_timing_ctrlr_update(struct komeda_component *c,
> > malidp_write32(reg, BS_PREFETCH_LINE, D71_DEFAULT_PREPRETCH_LINE);
> >
> > /* configure bs control register */
> > - value = BS_CTRL_EN | BS_CTRL_VM;
> > + value = BS_CTRL_EN | BS_CTRL_VM | BS_CTRL_CRC;
> > if (c->pipeline->dual_link) {
> > malidp_write32(reg, BS_DRIFT_TO, hfront_porch + 16);
> > value |= BS_CTRL_DL;
> > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > index d567ab7ed314e..05bfd9891c540 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > @@ -115,6 +115,8 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
> > raw_status = malidp_read32(reg, BLK_IRQ_RAW_STATUS);
> > if (raw_status & DOU_IRQ_PL0)
> > evts |= KOMEDA_EVENT_VSYNC;
> > + if (raw_status & DOU_IRQ_PL1)
> > + evts |= KOMEDA_EVENT_CRCDONE;
> > if (raw_status & DOU_IRQ_UND)
> > evts |= KOMEDA_EVENT_URUN;
> >
> > @@ -149,7 +151,7 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
> >
> > static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> > {
> > - u32 evts = 0ULL;
> > + u64 evts = 0ULL;
> >
> > if (gcu_status & (GLB_IRQ_STATUS_LPU0 | GLB_IRQ_STATUS_LPU1))
> > evts |= get_lpu_event(d71_pipeline);
> > @@ -163,6 +165,26 @@ static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> > return evts;
> > }
> >
> > +static void get_frame_crcs(struct d71_pipeline *d71_pipeline, u32 pipe,
> > + struct komeda_events *evts)
> > +{
> > + if (evts->pipes[pipe] & KOMEDA_EVENT_CRCDONE) {
> > + struct komeda_component *c;
> > +
> > + c = komeda_pipeline_get_component(&d71_pipeline->base,
> > + KOMEDA_COMPONENT_TIMING_CTRLR);
> > + if (!c)
> > + return;
> > +
> > + evts->crcs[pipe][0] = malidp_read32(c->reg, BS_CRC0_LOW);
> > + evts->crcs[pipe][1] = malidp_read32(c->reg, BS_CRC0_HIGH);
> > + if (d71_pipeline->base.dual_link) {
> > + evts->crcs[pipe][2] = malidp_read32(c->reg, BS_CRC1_LOW);
> > + evts->crcs[pipe][3] = malidp_read32(c->reg, BS_CRC1_HIGH);
> > + }
> > + }
> > +}
> > +
> > static irqreturn_t
> > d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > {
> > @@ -195,6 +217,9 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > if (gcu_status & GLB_IRQ_STATUS_PIPE1)
> > evts->pipes[1] |= get_pipeline_event(d71->pipes[1], gcu_status);
> >
> > + get_frame_crcs(d71->pipes[0], 0, evts);
> > + get_frame_crcs(d71->pipes[1], 1, evts);
> > +
> > return gcu_status ? IRQ_HANDLED : IRQ_NONE;
> > }
> >
> > @@ -202,7 +227,7 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > GCU_IRQ_MODE | GCU_IRQ_ERR)
> > #define ENABLED_LPU_IRQS (LPU_IRQ_IBSY | LPU_IRQ_ERR | LPU_IRQ_EOW)
> > #define ENABLED_CU_IRQS (CU_IRQ_OVR | CU_IRQ_ERR)
> > -#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR)
> > +#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR | DOU_IRQ_PL1)
> >
> > static int d71_enable_irq(struct komeda_dev *mdev)
> > {
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > index fa9a4593bb375..4b9f5d33e999d 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > @@ -207,10 +207,13 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> > drm_crtc_send_vblank_event(crtc, event);
> > } else {
> > DRM_WARN("CRTC[%d]: FLIP happen but no pending commit.\n",
> > - drm_crtc_index(&kcrtc->base));
> > + drm_crtc_index(crtc));
> > }
> > spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > }
> > +
> > + if ((kcrtc->crc_enabled) && (events & KOMEDA_EVENT_CRCDONE))
> > + drm_crtc_add_crc_entry(crtc, false, 0, evts->crcs[kcrtc->master->id]);
> > }
> >
> > static void
> > @@ -487,6 +490,59 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc)
> > mdev->funcs->on_off_vblank(mdev, kcrtc->master->id, false);
> > }
> >
> > +static const char * const komeda_pipe_crc_sources[] = {"auto"};
> > +
> > +static const char *const *komeda_crtc_get_crc_sources(struct drm_crtc *crtc,
> > + size_t *count)
> > +{
> > + *count = ARRAY_SIZE(komeda_pipe_crc_sources);
> > + return komeda_pipe_crc_sources;
> > +}
> > +
> > +static int komeda_crtc_parse_crc_source(const char *source)
> > +{
> > + if (!source)
> > + return 0;
> > + if (strcmp(source, "auto") == 0)
> > + return 1;
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int komeda_crtc_verify_crc_source(struct drm_crtc *crtc,
> > + const char *source_name,
> > + size_t *values_count)
> > +{
> > + struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> > + int source = komeda_crtc_parse_crc_source(source_name);
> > +
> > + if (source < 0) {
> > + DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
> > + drm_crtc_index(crtc));
> > + return -EINVAL;
> > + }
> > +
> > + *values_count = kcrtc->master->dual_link ? 4 : 2;
>
> Can CRC generation continue across a modeset? If so I think we could
> end up with a case where dual_link changes while CRC is active. Maybe
> we should just always return 4 values, but set the third and fourth
> values to 0 in the event handler, if dual-link isn't active.

Modeset is allowed to destry the crc setup. Maybe not the smartest
decision (it prevents us from making sure the first frame is perfect),
but pretty deeply in-grained into tests by now. If we want to move
this I think first step would be to add new basic testcases to igt to
validate crc generation against modesets (maybe start with dpms
off/on, then suspend/resume, then full modesets). Really not sure
there's a need, and I've seen too many cases like this where crc
generation changes depending upon modeset (bpp, routing, ...) to
assume we can just make it work.

I'd not bother and leave things as-is.
-Daniel
>
> Cheers,
> -Brian
>
> > +
> > + return 0;
> > +}
> > +
> > +static int komeda_crtc_set_crc_source(struct drm_crtc *crtc, const char *source)
> > +{
> > + struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> > + int src = komeda_crtc_parse_crc_source(source);
> > +
> > + if (src < 0) {
> > + DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
> > + drm_crtc_index(crtc));
> > + return -EINVAL;
> > + }
> > +
> > + kcrtc->crc_enabled = src & 1;
> > +
> > + return 0;
> > +}
> > +
> > static const struct drm_crtc_funcs komeda_crtc_funcs = {
> > .gamma_set = drm_atomic_helper_legacy_gamma_set,
> > .destroy = drm_crtc_cleanup,
> > @@ -497,6 +553,9 @@ static const struct drm_crtc_funcs komeda_crtc_funcs = {
> > .atomic_destroy_state = komeda_crtc_atomic_destroy_state,
> > .enable_vblank = komeda_crtc_vblank_enable,
> > .disable_vblank = komeda_crtc_vblank_disable,
> > + .set_crc_source = komeda_crtc_set_crc_source,
> > + .verify_crc_source = komeda_crtc_verify_crc_source,
> > + .get_crc_sources = komeda_crtc_get_crc_sources,
> > };
> >
> > int komeda_kms_setup_crtcs(struct komeda_kms_dev *kms,
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > index d1c86b6174c80..244227b945f63 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > @@ -20,6 +20,7 @@
> > #define KOMEDA_EVENT_OVR BIT_ULL(4)
> > #define KOMEDA_EVENT_EOW BIT_ULL(5)
> > #define KOMEDA_EVENT_MODE BIT_ULL(6)
> > +#define KOMEDA_EVENT_CRCDONE BIT_ULL(7)
> >
> > #define KOMEDA_ERR_TETO BIT_ULL(14)
> > #define KOMEDA_ERR_TEMR BIT_ULL(15)
> > @@ -69,6 +70,7 @@ struct komeda_dev;
> > struct komeda_events {
> > u64 global;
> > u64 pipes[KOMEDA_MAX_PIPELINES];
> > + u32 crcs[KOMEDA_MAX_PIPELINES][KOMEDA_MAX_CRCS];
> > };
> >
> > /**
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > index 45c498e15e7ae..de7c93b2d0a11 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > @@ -84,6 +84,9 @@ struct komeda_crtc {
> >
> > /** @disable_done: this flip_done is for tracing the disable */
> > struct completion *disable_done;
> > +
> > + /** @crc_enabled: true if per-frame generation of CRC is enabled */
> > + bool crc_enabled;
> > };
> >
> > /**
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > index a7a84e66549d6..dfe2482c6274b 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > @@ -16,6 +16,7 @@
> > #define KOMEDA_PIPELINE_MAX_LAYERS 4
> > #define KOMEDA_PIPELINE_MAX_SCALERS 2
> > #define KOMEDA_COMPONENT_N_INPUTS 5
> > +#define KOMEDA_MAX_CRCS 4
> >
> > /* pipeline component IDs */
> > enum {
> > --
> > 2.22.0
> >



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

2019-08-03 14:21:40

by Liviu Dudau

Subject: Re: drm/komeda: Add support for generation of CRC data per frame.

On Fri, Aug 02, 2019 at 11:52:11AM +0200, Daniel Vetter wrote:
> On Fri, Aug 2, 2019 at 11:39 AM Brian Starkey <[email protected]> wrote:
> >
> > Hi Liviu,
> >
> > On Thu, Aug 01, 2019 at 11:42:31AM +0100, Liviu Dudau wrote:
> > > Komeda has support to generate per-frame CRC values in the DOU
> > > backend subsystem. Implement necessary hooks to expose the CRC
> > > "control" and "data" file over debugfs and program the DOUx_BS
> > > accordingly.
> > >
> > > This patch makes use of PL1 (programmable line 1) interrupt to
> > > know when the CRC generation has finished.
> > >
> > > Patch is also dependent on the series that adds dual-link support
> > > for komeda: https://patchwork.freedesktop.org/series/62280/
> > >
> > > Cc: "james qian wang (Arm Technology China)" <[email protected]>
> > > Signed-off-by: Liviu Dudau <[email protected]>
> > > ---
> > > .../arm/display/komeda/d71/d71_component.c | 2 +-
> > > .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 29 ++++++++-
> > > .../gpu/drm/arm/display/komeda/komeda_crtc.c | 61 ++++++++++++++++++-
> > > .../gpu/drm/arm/display/komeda/komeda_dev.h | 2 +
> > > .../gpu/drm/arm/display/komeda/komeda_kms.h | 3 +
> > > .../drm/arm/display/komeda/komeda_pipeline.h | 1 +
> > > 6 files changed, 94 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > index 55a8cc94808a1..3c45468848ee4 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > @@ -1061,7 +1061,7 @@ static void d71_timing_ctrlr_update(struct komeda_component *c,
> > > malidp_write32(reg, BS_PREFETCH_LINE, D71_DEFAULT_PREPRETCH_LINE);
> > >
> > > /* configure bs control register */
> > > - value = BS_CTRL_EN | BS_CTRL_VM;
> > > + value = BS_CTRL_EN | BS_CTRL_VM | BS_CTRL_CRC;
> > > if (c->pipeline->dual_link) {
> > > malidp_write32(reg, BS_DRIFT_TO, hfront_porch + 16);
> > > value |= BS_CTRL_DL;
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > index d567ab7ed314e..05bfd9891c540 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > @@ -115,6 +115,8 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
> > > raw_status = malidp_read32(reg, BLK_IRQ_RAW_STATUS);
> > > if (raw_status & DOU_IRQ_PL0)
> > > evts |= KOMEDA_EVENT_VSYNC;
> > > + if (raw_status & DOU_IRQ_PL1)
> > > + evts |= KOMEDA_EVENT_CRCDONE;
> > > if (raw_status & DOU_IRQ_UND)
> > > evts |= KOMEDA_EVENT_URUN;
> > >
> > > @@ -149,7 +151,7 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
> > >
> > > static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> > > {
> > > - u32 evts = 0ULL;
> > > + u64 evts = 0ULL;
> > >
> > > if (gcu_status & (GLB_IRQ_STATUS_LPU0 | GLB_IRQ_STATUS_LPU1))
> > > evts |= get_lpu_event(d71_pipeline);
> > > @@ -163,6 +165,26 @@ static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> > > return evts;
> > > }
> > >
> > > +static void get_frame_crcs(struct d71_pipeline *d71_pipeline, u32 pipe,
> > > + struct komeda_events *evts)
> > > +{
> > > + if (evts->pipes[pipe] & KOMEDA_EVENT_CRCDONE) {
> > > + struct komeda_component *c;
> > > +
> > > + c = komeda_pipeline_get_component(&d71_pipeline->base,
> > > + KOMEDA_COMPONENT_TIMING_CTRLR);
> > > + if (!c)
> > > + return;
> > > +
> > > + evts->crcs[pipe][0] = malidp_read32(c->reg, BS_CRC0_LOW);
> > > + evts->crcs[pipe][1] = malidp_read32(c->reg, BS_CRC0_HIGH);
> > > + if (d71_pipeline->base.dual_link) {
> > > + evts->crcs[pipe][2] = malidp_read32(c->reg, BS_CRC1_LOW);
> > > + evts->crcs[pipe][3] = malidp_read32(c->reg, BS_CRC1_HIGH);
> > > + }
> > > + }
> > > +}
> > > +
> > > static irqreturn_t
> > > d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > {
> > > @@ -195,6 +217,9 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > if (gcu_status & GLB_IRQ_STATUS_PIPE1)
> > > evts->pipes[1] |= get_pipeline_event(d71->pipes[1], gcu_status);
> > >
> > > + get_frame_crcs(d71->pipes[0], 0, evts);
> > > + get_frame_crcs(d71->pipes[1], 1, evts);
> > > +
> > > return gcu_status ? IRQ_HANDLED : IRQ_NONE;
> > > }
> > >
> > > @@ -202,7 +227,7 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > GCU_IRQ_MODE | GCU_IRQ_ERR)
> > > #define ENABLED_LPU_IRQS (LPU_IRQ_IBSY | LPU_IRQ_ERR | LPU_IRQ_EOW)
> > > #define ENABLED_CU_IRQS (CU_IRQ_OVR | CU_IRQ_ERR)
> > > -#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR)
> > > +#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR | DOU_IRQ_PL1)
> > >
> > > static int d71_enable_irq(struct komeda_dev *mdev)
> > > {
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > index fa9a4593bb375..4b9f5d33e999d 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > @@ -207,10 +207,13 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> > > drm_crtc_send_vblank_event(crtc, event);
> > > } else {
> > > DRM_WARN("CRTC[%d]: FLIP happen but no pending commit.\n",
> > > - drm_crtc_index(&kcrtc->base));
> > > + drm_crtc_index(crtc));
> > > }
> > > spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > > }
> > > +
> > > + if ((kcrtc->crc_enabled) && (events & KOMEDA_EVENT_CRCDONE))
> > > + drm_crtc_add_crc_entry(crtc, false, 0, evts->crcs[kcrtc->master->id]);
> > > }
> > >
> > > static void
> > > @@ -487,6 +490,59 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc)
> > > mdev->funcs->on_off_vblank(mdev, kcrtc->master->id, false);
> > > }
> > >
> > > +static const char * const komeda_pipe_crc_sources[] = {"auto"};
> > > +
> > > +static const char *const *komeda_crtc_get_crc_sources(struct drm_crtc *crtc,
> > > + size_t *count)
> > > +{
> > > + *count = ARRAY_SIZE(komeda_pipe_crc_sources);
> > > + return komeda_pipe_crc_sources;
> > > +}
> > > +
> > > +static int komeda_crtc_parse_crc_source(const char *source)
> > > +{
> > > + if (!source)
> > > + return 0;
> > > + if (strcmp(source, "auto") == 0)
> > > + return 1;
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static int komeda_crtc_verify_crc_source(struct drm_crtc *crtc,
> > > + const char *source_name,
> > > + size_t *values_count)
> > > +{
> > > + struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> > > + int source = komeda_crtc_parse_crc_source(source_name);
> > > +
> > > + if (source < 0) {
> > > + DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
> > > + drm_crtc_index(crtc));
> > > + return -EINVAL;
> > > + }
> > > +
> > > + *values_count = kcrtc->master->dual_link ? 4 : 2;
> >
> > Can CRC generation continue across a modeset? If so I think we could
> > end up with a case where dual_link changes while CRC is active. Maybe
> > we should just always return 4 values, but set the third and fourth
> > values to 0 in the event handler, if dual-link isn't active.
>
> Modeset is allowed to destry the crc setup. Maybe not the smartest
> decision (it prevents us from making sure the first frame is perfect),
> but pretty deeply in-grained into tests by now. If we want to move
> this I think first step would be to add new basic testcases to igt to
> validate crc generation against modesets (maybe start with dpms
> off/on, then suspend/resume, then full modesets). Really not sure
> there's a need, and I've seen too many cases like this where crc
> generation changes depending upon modeset (bpp, routing, ...) to
> assume we can just make it work.
>
> I'd not bother and leave things as-is.

Daniel, should I take this also as a Reviewed-by you?

Best regards,
Liviu

> -Daniel
> >
> > Cheers,
> > -Brian
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int komeda_crtc_set_crc_source(struct drm_crtc *crtc, const char *source)
> > > +{
> > > + struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> > > + int src = komeda_crtc_parse_crc_source(source);
> > > +
> > > + if (src < 0) {
> > > + DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
> > > + drm_crtc_index(crtc));
> > > + return -EINVAL;
> > > + }
> > > +
> > > + kcrtc->crc_enabled = src & 1;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static const struct drm_crtc_funcs komeda_crtc_funcs = {
> > > .gamma_set = drm_atomic_helper_legacy_gamma_set,
> > > .destroy = drm_crtc_cleanup,
> > > @@ -497,6 +553,9 @@ static const struct drm_crtc_funcs komeda_crtc_funcs = {
> > > .atomic_destroy_state = komeda_crtc_atomic_destroy_state,
> > > .enable_vblank = komeda_crtc_vblank_enable,
> > > .disable_vblank = komeda_crtc_vblank_disable,
> > > + .set_crc_source = komeda_crtc_set_crc_source,
> > > + .verify_crc_source = komeda_crtc_verify_crc_source,
> > > + .get_crc_sources = komeda_crtc_get_crc_sources,
> > > };
> > >
> > > int komeda_kms_setup_crtcs(struct komeda_kms_dev *kms,
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > index d1c86b6174c80..244227b945f63 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > @@ -20,6 +20,7 @@
> > > #define KOMEDA_EVENT_OVR BIT_ULL(4)
> > > #define KOMEDA_EVENT_EOW BIT_ULL(5)
> > > #define KOMEDA_EVENT_MODE BIT_ULL(6)
> > > +#define KOMEDA_EVENT_CRCDONE BIT_ULL(7)
> > >
> > > #define KOMEDA_ERR_TETO BIT_ULL(14)
> > > #define KOMEDA_ERR_TEMR BIT_ULL(15)
> > > @@ -69,6 +70,7 @@ struct komeda_dev;
> > > struct komeda_events {
> > > u64 global;
> > > u64 pipes[KOMEDA_MAX_PIPELINES];
> > > + u32 crcs[KOMEDA_MAX_PIPELINES][KOMEDA_MAX_CRCS];
> > > };
> > >
> > > /**
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > > index 45c498e15e7ae..de7c93b2d0a11 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > > @@ -84,6 +84,9 @@ struct komeda_crtc {
> > >
> > > /** @disable_done: this flip_done is for tracing the disable */
> > > struct completion *disable_done;
> > > +
> > > + /** @crc_enabled: true if per-frame generation of CRC is enabled */
> > > + bool crc_enabled;
> > > };
> > >
> > > /**
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > > index a7a84e66549d6..dfe2482c6274b 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > > @@ -16,6 +16,7 @@
> > > #define KOMEDA_PIPELINE_MAX_LAYERS 4
> > > #define KOMEDA_PIPELINE_MAX_SCALERS 2
> > > #define KOMEDA_COMPONENT_N_INPUTS 5
> > > +#define KOMEDA_MAX_CRCS 4
> > >
> > > /* pipeline component IDs */
> > > enum {
> > > --
> > > 2.22.0
> > >
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯

2019-08-03 14:23:48

by Daniel Vetter

Subject: Re: drm/komeda: Add support for generation of CRC data per frame.

On Fri, Aug 2, 2019 at 4:50 PM Liviu Dudau <[email protected]> wrote:
>
> On Fri, Aug 02, 2019 at 11:52:11AM +0200, Daniel Vetter wrote:
> > On Fri, Aug 2, 2019 at 11:39 AM Brian Starkey <[email protected]> wrote:
> > >
> > > Hi Liviu,
> > >
> > > On Thu, Aug 01, 2019 at 11:42:31AM +0100, Liviu Dudau wrote:
> > > > Komeda has support to generate per-frame CRC values in the DOU
> > > > backend subsystem. Implement necessary hooks to expose the CRC
> > > > "control" and "data" file over debugfs and program the DOUx_BS
> > > > accordingly.
> > > >
> > > > This patch makes use of PL1 (programmable line 1) interrupt to
> > > > know when the CRC generation has finished.
> > > >
> > > > Patch is also dependent on the series that adds dual-link support
> > > > for komeda: https://patchwork.freedesktop.org/series/62280/
> > > >
> > > > Cc: "james qian wang (Arm Technology China)" <[email protected]>
> > > > Signed-off-by: Liviu Dudau <[email protected]>
> > > > ---
> > > > .../arm/display/komeda/d71/d71_component.c | 2 +-
> > > > .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 29 ++++++++-
> > > > .../gpu/drm/arm/display/komeda/komeda_crtc.c | 61 ++++++++++++++++++-
> > > > .../gpu/drm/arm/display/komeda/komeda_dev.h | 2 +
> > > > .../gpu/drm/arm/display/komeda/komeda_kms.h | 3 +
> > > > .../drm/arm/display/komeda/komeda_pipeline.h | 1 +
> > > > 6 files changed, 94 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > index 55a8cc94808a1..3c45468848ee4 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > @@ -1061,7 +1061,7 @@ static void d71_timing_ctrlr_update(struct komeda_component *c,
> > > > malidp_write32(reg, BS_PREFETCH_LINE, D71_DEFAULT_PREPRETCH_LINE);
> > > >
> > > > /* configure bs control register */
> > > > - value = BS_CTRL_EN | BS_CTRL_VM;
> > > > + value = BS_CTRL_EN | BS_CTRL_VM | BS_CTRL_CRC;
> > > > if (c->pipeline->dual_link) {
> > > > malidp_write32(reg, BS_DRIFT_TO, hfront_porch + 16);
> > > > value |= BS_CTRL_DL;
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > index d567ab7ed314e..05bfd9891c540 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > @@ -115,6 +115,8 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
> > > > raw_status = malidp_read32(reg, BLK_IRQ_RAW_STATUS);
> > > > if (raw_status & DOU_IRQ_PL0)
> > > > evts |= KOMEDA_EVENT_VSYNC;
> > > > + if (raw_status & DOU_IRQ_PL1)
> > > > + evts |= KOMEDA_EVENT_CRCDONE;
> > > > if (raw_status & DOU_IRQ_UND)
> > > > evts |= KOMEDA_EVENT_URUN;
> > > >
> > > > @@ -149,7 +151,7 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
> > > >
> > > > static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> > > > {
> > > > - u32 evts = 0ULL;
> > > > + u64 evts = 0ULL;
> > > >
> > > > if (gcu_status & (GLB_IRQ_STATUS_LPU0 | GLB_IRQ_STATUS_LPU1))
> > > > evts |= get_lpu_event(d71_pipeline);
> > > > @@ -163,6 +165,26 @@ static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> > > > return evts;
> > > > }
> > > >
> > > > +static void get_frame_crcs(struct d71_pipeline *d71_pipeline, u32 pipe,
> > > > + struct komeda_events *evts)
> > > > +{
> > > > + if (evts->pipes[pipe] & KOMEDA_EVENT_CRCDONE) {
> > > > + struct komeda_component *c;
> > > > +
> > > > + c = komeda_pipeline_get_component(&d71_pipeline->base,
> > > > + KOMEDA_COMPONENT_TIMING_CTRLR);
> > > > + if (!c)
> > > > + return;
> > > > +
> > > > + evts->crcs[pipe][0] = malidp_read32(c->reg, BS_CRC0_LOW);
> > > > + evts->crcs[pipe][1] = malidp_read32(c->reg, BS_CRC0_HIGH);
> > > > + if (d71_pipeline->base.dual_link) {
> > > > + evts->crcs[pipe][2] = malidp_read32(c->reg, BS_CRC1_LOW);
> > > > + evts->crcs[pipe][3] = malidp_read32(c->reg, BS_CRC1_HIGH);
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > static irqreturn_t
> > > > d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > > {
> > > > @@ -195,6 +217,9 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > > if (gcu_status & GLB_IRQ_STATUS_PIPE1)
> > > > evts->pipes[1] |= get_pipeline_event(d71->pipes[1], gcu_status);
> > > >
> > > > + get_frame_crcs(d71->pipes[0], 0, evts);
> > > > + get_frame_crcs(d71->pipes[1], 1, evts);
> > > > +
> > > > return gcu_status ? IRQ_HANDLED : IRQ_NONE;
> > > > }
> > > >
> > > > @@ -202,7 +227,7 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > > GCU_IRQ_MODE | GCU_IRQ_ERR)
> > > > #define ENABLED_LPU_IRQS (LPU_IRQ_IBSY | LPU_IRQ_ERR | LPU_IRQ_EOW)
> > > > #define ENABLED_CU_IRQS (CU_IRQ_OVR | CU_IRQ_ERR)
> > > > -#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR)
> > > > +#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR | DOU_IRQ_PL1)
> > > >
> > > > static int d71_enable_irq(struct komeda_dev *mdev)
> > > > {
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > index fa9a4593bb375..4b9f5d33e999d 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > @@ -207,10 +207,13 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> > > > drm_crtc_send_vblank_event(crtc, event);
> > > > } else {
> > > > DRM_WARN("CRTC[%d]: FLIP happen but no pending commit.\n",
> > > > - drm_crtc_index(&kcrtc->base));
> > > > + drm_crtc_index(crtc));
> > > > }
> > > > spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > > > }
> > > > +
> > > > + if ((kcrtc->crc_enabled) && (events & KOMEDA_EVENT_CRCDONE))
> > > > + drm_crtc_add_crc_entry(crtc, false, 0, evts->crcs[kcrtc->master->id]);
> > > > }
> > > >
> > > > static void
> > > > @@ -487,6 +490,59 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc)
> > > > mdev->funcs->on_off_vblank(mdev, kcrtc->master->id, false);
> > > > }
> > > >
> > > > +static const char * const komeda_pipe_crc_sources[] = {"auto"};
> > > > +
> > > > +static const char *const *komeda_crtc_get_crc_sources(struct drm_crtc *crtc,
> > > > + size_t *count)
> > > > +{
> > > > + *count = ARRAY_SIZE(komeda_pipe_crc_sources);
> > > > + return komeda_pipe_crc_sources;
> > > > +}
> > > > +
> > > > +static int komeda_crtc_parse_crc_source(const char *source)
> > > > +{
> > > > + if (!source)
> > > > + return 0;
> > > > + if (strcmp(source, "auto") == 0)
> > > > + return 1;
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +static int komeda_crtc_verify_crc_source(struct drm_crtc *crtc,
> > > > + const char *source_name,
> > > > + size_t *values_count)
> > > > +{
> > > > + struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> > > > + int source = komeda_crtc_parse_crc_source(source_name);
> > > > +
> > > > + if (source < 0) {
> > > > + DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
> > > > + drm_crtc_index(crtc));
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + *values_count = kcrtc->master->dual_link ? 4 : 2;
> > >
> > > Can CRC generation continue across a modeset? If so I think we could
> > > end up with a case where dual_link changes while CRC is active. Maybe
> > > we should just always return 4 values, but set the third and fourth
> > > values to 0 in the event handler, if dual-link isn't active.
> >
> > Modeset is allowed to destry the crc setup. Maybe not the smartest
> > decision (it prevents us from making sure the first frame is perfect),
> > but pretty deeply in-grained into tests by now. If we want to move
> > this I think first step would be to add new basic testcases to igt to
> > validate crc generation against modesets (maybe start with dpms
> > off/on, then suspend/resume, then full modesets). Really not sure
> > there's a need, and I've seen too many cases like this where crc
> > generation changes depending upon modeset (bpp, routing, ...) to
> > assume we can just make it work.
> >
> > I'd not bother and leave things as-is.
>
> Daniel, should I take this also as a Reviewed-by you?

Maybe ack, I have no idea how you're hw works and I didn't really look
at the code.
-Daniel

>
> Best regards,
> Liviu
>
> > -Daniel
> > >
> > > Cheers,
> > > -Brian
> > >
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int komeda_crtc_set_crc_source(struct drm_crtc *crtc, const char *source)
> > > > +{
> > > > + struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> > > > + int src = komeda_crtc_parse_crc_source(source);
> > > > +
> > > > + if (src < 0) {
> > > > + DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
> > > > + drm_crtc_index(crtc));
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + kcrtc->crc_enabled = src & 1;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > static const struct drm_crtc_funcs komeda_crtc_funcs = {
> > > > .gamma_set = drm_atomic_helper_legacy_gamma_set,
> > > > .destroy = drm_crtc_cleanup,
> > > > @@ -497,6 +553,9 @@ static const struct drm_crtc_funcs komeda_crtc_funcs = {
> > > > .atomic_destroy_state = komeda_crtc_atomic_destroy_state,
> > > > .enable_vblank = komeda_crtc_vblank_enable,
> > > > .disable_vblank = komeda_crtc_vblank_disable,
> > > > + .set_crc_source = komeda_crtc_set_crc_source,
> > > > + .verify_crc_source = komeda_crtc_verify_crc_source,
> > > > + .get_crc_sources = komeda_crtc_get_crc_sources,
> > > > };
> > > >
> > > > int komeda_kms_setup_crtcs(struct komeda_kms_dev *kms,
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > > index d1c86b6174c80..244227b945f63 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > > @@ -20,6 +20,7 @@
> > > > #define KOMEDA_EVENT_OVR BIT_ULL(4)
> > > > #define KOMEDA_EVENT_EOW BIT_ULL(5)
> > > > #define KOMEDA_EVENT_MODE BIT_ULL(6)
> > > > +#define KOMEDA_EVENT_CRCDONE BIT_ULL(7)
> > > >
> > > > #define KOMEDA_ERR_TETO BIT_ULL(14)
> > > > #define KOMEDA_ERR_TEMR BIT_ULL(15)
> > > > @@ -69,6 +70,7 @@ struct komeda_dev;
> > > > struct komeda_events {
> > > > u64 global;
> > > > u64 pipes[KOMEDA_MAX_PIPELINES];
> > > > + u32 crcs[KOMEDA_MAX_PIPELINES][KOMEDA_MAX_CRCS];
> > > > };
> > > >
> > > > /**
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > > > index 45c498e15e7ae..de7c93b2d0a11 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> > > > @@ -84,6 +84,9 @@ struct komeda_crtc {
> > > >
> > > > /** @disable_done: this flip_done is for tracing the disable */
> > > > struct completion *disable_done;
> > > > +
> > > > + /** @crc_enabled: true if per-frame generation of CRC is enabled */
> > > > + bool crc_enabled;
> > > > };
> > > >
> > > > /**
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > > > index a7a84e66549d6..dfe2482c6274b 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> > > > @@ -16,6 +16,7 @@
> > > > #define KOMEDA_PIPELINE_MAX_LAYERS 4
> > > > #define KOMEDA_PIPELINE_MAX_SCALERS 2
> > > > #define KOMEDA_COMPONENT_N_INPUTS 5
> > > > +#define KOMEDA_MAX_CRCS 4
> > > >
> > > > /* pipeline component IDs */
> > > > enum {
> > > > --
> > > > 2.22.0
> > > >
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> --
> ====================
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ¯\_(ツ)_/¯



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

2019-08-03 18:06:43

by Daniel Vetter

Subject: Re: drm/komeda: Add support for generation of CRC data per frame.

On Fri, Aug 02, 2019 at 10:13:03AM +0000, Brian Starkey wrote:
> Hi Daniel,
>
> On Fri, Aug 02, 2019 at 11:52:11AM +0200, Daniel Vetter wrote:
> > On Fri, Aug 2, 2019 at 11:39 AM Brian Starkey <[email protected]> wrote:
> > >
> > > Hi Liviu,
> > >
> > > On Thu, Aug 01, 2019 at 11:42:31AM +0100, Liviu Dudau wrote:
> > > > Komeda has support to generate per-frame CRC values in the DOU
> > > > backend subsystem. Implement necessary hooks to expose the CRC
> > > > "control" and "data" file over debugfs and program the DOUx_BS
> > > > accordingly.
> > > >
> > > > This patch makes use of PL1 (programmable line 1) interrupt to
> > > > know when the CRC generation has finished.
> > > >
> > > > Patch is also dependent on the series that adds dual-link support
> > > > for komeda: https://patchwork.freedesktop.org/series/62280/
> > > >
> > > > Cc: "james qian wang (Arm Technology China)" <[email protected]>
> > > > Signed-off-by: Liviu Dudau <[email protected]>
> > > > ---
> > > > .../arm/display/komeda/d71/d71_component.c | 2 +-
> > > > .../gpu/drm/arm/display/komeda/d71/d71_dev.c | 29 ++++++++-
> > > > .../gpu/drm/arm/display/komeda/komeda_crtc.c | 61 ++++++++++++++++++-
> > > > .../gpu/drm/arm/display/komeda/komeda_dev.h | 2 +
> > > > .../gpu/drm/arm/display/komeda/komeda_kms.h | 3 +
> > > > .../drm/arm/display/komeda/komeda_pipeline.h | 1 +
> > > > 6 files changed, 94 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > index 55a8cc94808a1..3c45468848ee4 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c
> > > > @@ -1061,7 +1061,7 @@ static void d71_timing_ctrlr_update(struct komeda_component *c,
> > > > malidp_write32(reg, BS_PREFETCH_LINE, D71_DEFAULT_PREPRETCH_LINE);
> > > >
> > > > /* configure bs control register */
> > > > - value = BS_CTRL_EN | BS_CTRL_VM;
> > > > + value = BS_CTRL_EN | BS_CTRL_VM | BS_CTRL_CRC;
> > > > if (c->pipeline->dual_link) {
> > > > malidp_write32(reg, BS_DRIFT_TO, hfront_porch + 16);
> > > > value |= BS_CTRL_DL;
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > index d567ab7ed314e..05bfd9891c540 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> > > > @@ -115,6 +115,8 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
> > > > raw_status = malidp_read32(reg, BLK_IRQ_RAW_STATUS);
> > > > if (raw_status & DOU_IRQ_PL0)
> > > > evts |= KOMEDA_EVENT_VSYNC;
> > > > + if (raw_status & DOU_IRQ_PL1)
> > > > + evts |= KOMEDA_EVENT_CRCDONE;
> > > > if (raw_status & DOU_IRQ_UND)
> > > > evts |= KOMEDA_EVENT_URUN;
> > > >
> > > > @@ -149,7 +151,7 @@ static u64 get_dou_event(struct d71_pipeline *d71_pipeline)
> > > >
> > > > static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> > > > {
> > > > - u32 evts = 0ULL;
> > > > + u64 evts = 0ULL;
> > > >
> > > > if (gcu_status & (GLB_IRQ_STATUS_LPU0 | GLB_IRQ_STATUS_LPU1))
> > > > evts |= get_lpu_event(d71_pipeline);
> > > > @@ -163,6 +165,26 @@ static u64 get_pipeline_event(struct d71_pipeline *d71_pipeline, u32 gcu_status)
> > > > return evts;
> > > > }
> > > >
> > > > +static void get_frame_crcs(struct d71_pipeline *d71_pipeline, u32 pipe,
> > > > + struct komeda_events *evts)
> > > > +{
> > > > + if (evts->pipes[pipe] & KOMEDA_EVENT_CRCDONE) {
> > > > + struct komeda_component *c;
> > > > +
> > > > + c = komeda_pipeline_get_component(&d71_pipeline->base,
> > > > + KOMEDA_COMPONENT_TIMING_CTRLR);
> > > > + if (!c)
> > > > + return;
> > > > +
> > > > + evts->crcs[pipe][0] = malidp_read32(c->reg, BS_CRC0_LOW);
> > > > + evts->crcs[pipe][1] = malidp_read32(c->reg, BS_CRC0_HIGH);
> > > > + if (d71_pipeline->base.dual_link) {
> > > > + evts->crcs[pipe][2] = malidp_read32(c->reg, BS_CRC1_LOW);
> > > > + evts->crcs[pipe][3] = malidp_read32(c->reg, BS_CRC1_HIGH);
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > static irqreturn_t
> > > > d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > > {
> > > > @@ -195,6 +217,9 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > > if (gcu_status & GLB_IRQ_STATUS_PIPE1)
> > > > evts->pipes[1] |= get_pipeline_event(d71->pipes[1], gcu_status);
> > > >
> > > > + get_frame_crcs(d71->pipes[0], 0, evts);
> > > > + get_frame_crcs(d71->pipes[1], 1, evts);
> > > > +
> > > > return gcu_status ? IRQ_HANDLED : IRQ_NONE;
> > > > }
> > > >
> > > > @@ -202,7 +227,7 @@ d71_irq_handler(struct komeda_dev *mdev, struct komeda_events *evts)
> > > > GCU_IRQ_MODE | GCU_IRQ_ERR)
> > > > #define ENABLED_LPU_IRQS (LPU_IRQ_IBSY | LPU_IRQ_ERR | LPU_IRQ_EOW)
> > > > #define ENABLED_CU_IRQS (CU_IRQ_OVR | CU_IRQ_ERR)
> > > > -#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR)
> > > > +#define ENABLED_DOU_IRQS (DOU_IRQ_UND | DOU_IRQ_ERR | DOU_IRQ_PL1)
> > > >
> > > > static int d71_enable_irq(struct komeda_dev *mdev)
> > > > {
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > index fa9a4593bb375..4b9f5d33e999d 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > @@ -207,10 +207,13 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> > > > drm_crtc_send_vblank_event(crtc, event);
> > > > } else {
> > > > DRM_WARN("CRTC[%d]: FLIP happen but no pending commit.\n",
> > > > - drm_crtc_index(&kcrtc->base));
> > > > + drm_crtc_index(crtc));
> > > > }
> > > > spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > > > }
> > > > +
> > > > + if ((kcrtc->crc_enabled) && (events & KOMEDA_EVENT_CRCDONE))
> > > > + drm_crtc_add_crc_entry(crtc, false, 0, evts->crcs[kcrtc->master->id]);
> > > > }
> > > >
> > > > static void
> > > > @@ -487,6 +490,59 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc)
> > > > mdev->funcs->on_off_vblank(mdev, kcrtc->master->id, false);
> > > > }
> > > >
> > > > +static const char * const komeda_pipe_crc_sources[] = {"auto"};
> > > > +
> > > > +static const char *const *komeda_crtc_get_crc_sources(struct drm_crtc *crtc,
> > > > + size_t *count)
> > > > +{
> > > > + *count = ARRAY_SIZE(komeda_pipe_crc_sources);
> > > > + return komeda_pipe_crc_sources;
> > > > +}
> > > > +
> > > > +static int komeda_crtc_parse_crc_source(const char *source)
> > > > +{
> > > > + if (!source)
> > > > + return 0;
> > > > + if (strcmp(source, "auto") == 0)
> > > > + return 1;
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +static int komeda_crtc_verify_crc_source(struct drm_crtc *crtc,
> > > > + const char *source_name,
> > > > + size_t *values_count)
> > > > +{
> > > > + struct komeda_crtc *kcrtc = to_kcrtc(crtc);
> > > > + int source = komeda_crtc_parse_crc_source(source_name);
> > > > +
> > > > + if (source < 0) {
> > > > + DRM_DEBUG_DRIVER("Unknown or invalid CRC source for CRTC%d\n",
> > > > + drm_crtc_index(crtc));
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + *values_count = kcrtc->master->dual_link ? 4 : 2;
> > >
> > > Can CRC generation continue across a modeset? If so I think we could
> > > end up with a case where dual_link changes while CRC is active. Maybe
> > > we should just always return 4 values, but set the third and fourth
> > > values to 0 in the event handler, if dual-link isn't active.
> >
> > Modeset is allowed to destry the crc setup. Maybe not the smartest
> > decision (it prevents us from making sure the first frame is perfect),
> > but pretty deeply in-grained into tests by now. If we want to move
> > this I think first step would be to add new basic testcases to igt to
> > validate crc generation against modesets (maybe start with dpms
> > off/on, then suspend/resume, then full modesets). Really not sure
> > there's a need, and I've seen too many cases like this where crc
> > generation changes depending upon modeset (bpp, routing, ...) to
> > assume we can just make it work.
> >
> > I'd not bother and leave things as-is.
>
> OK good, glad we're not alone. Do we need to do anything specific to
> stop/reconfigure generation over modeset, or we just rely on userspace
> to reconfigure?

Expectation is:
- userspace configures mode
- userspace selects crc source and enables it (this might internally do a
full modeset to get the pipeline back into a shape were we can capture
crcs - some of the low power modes in i915 bypass the crc block ...
yay).

So nothing to do for you. I think the rough rule is that crc generation
should continue working as long as you don't do anything which requires an
ALLOW_MODESET commit.

Care to type a kerneldoc patch to clarify this, since I think current docs
are lacking?
-Daniel

>
> Thanks,
> -Brian
>
> > -Daniel
> > >
> > > Cheers,
> > > -Brian
> > >

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

2019-08-05 15:14:25

by Brian Starkey

Subject: [PATCH] drm/crc-debugfs: Add notes about CRC<->commit interactions

CRC generation can be impacted by commits coming from userspace, and
enabling CRC generation may itself trigger a commit. Add notes about
this to the kerneldoc.

Signed-off-by: Brian Starkey <[email protected]>
---

I might have got the wrong end of the stick, but this is what I
understood from what you said.

Cheers,
-Brian

drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++----
include/drm/drm_crtc.h | 3 +++
2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 7ca486d750e9..1dff956bcc74 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -65,10 +65,17 @@
* it submits. In this general case, the maximum userspace can do is to compare
* the reported CRCs of frames that should have the same contents.
*
- * On the driver side the implementation effort is minimal, drivers only need to
- * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically
- * set up if that vfunc is set. CRC samples need to be captured in the driver by
- * calling drm_crtc_add_crc_entry().
+ * On the driver side the implementation effort is minimal, drivers only need
+ * to implement &drm_crtc_funcs.set_crc_source. The debugfs files are
+ * automatically set up if that vfunc is set. CRC samples need to be captured
+ * in the driver by calling drm_crtc_add_crc_entry(). Depending on the driver
+ * and HW requirements, &drm_crtc_funcs.set_crc_source may result in a commit
+ * (even a full modeset).
+ *
+ * It's also possible that a "normal" commit via DRM_IOCTL_MODE_ATOMIC or the
+ * legacy paths may interfere with CRC generation. So, in the general case,
+ * userspace can't rely on the values in crtc-N/crc/data being valid
+ * across a commit.
*/

static int crc_control_show(struct seq_file *m, void *data)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 128d8b210621..0f7ea094a900 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -756,6 +756,8 @@ struct drm_crtc_funcs {
* provided from the configured source. Drivers must accept an "auto"
* source name that will select a default source for this CRTC.
*
+ * This may trigger a commit if necessary, to enable CRC generation.
+ *
* Note that "auto" can depend upon the current modeset configuration,
* e.g. it could pick an encoder or output specific CRC sampling point.
*
@@ -767,6 +769,7 @@ struct drm_crtc_funcs {
* 0 on success or a negative error code on failure.
*/
int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
+
/**
* @verify_crc_source:
*
--
2.17.1

2019-08-05 16:25:46

by Daniel Vetter

Subject: Re: [PATCH] drm/crc-debugfs: Add notes about CRC<->commit interactions

On Mon, Aug 05, 2019 at 04:11:43PM +0100, Brian Starkey wrote:
> CRC generation can be impacted by commits coming from userspace, and
> enabling CRC generation may itself trigger a commit. Add notes about
> this to the kerneldoc.
>
> Signed-off-by: Brian Starkey <[email protected]>
> ---
>
> I might have got the wrong end of the stick, but this is what I
> understood from what you said.
>
> Cheers,
> -Brian
>
> drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++----
> include/drm/drm_crtc.h | 3 +++
> 2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 7ca486d750e9..1dff956bcc74 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -65,10 +65,17 @@
> * it submits. In this general case, the maximum userspace can do is to compare
> * the reported CRCs of frames that should have the same contents.
> *
> - * On the driver side the implementation effort is minimal, drivers only need to
> - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically
> - * set up if that vfunc is set. CRC samples need to be captured in the driver by
> - * calling drm_crtc_add_crc_entry().
> + * On the driver side the implementation effort is minimal, drivers only need
> + * to implement &drm_crtc_funcs.set_crc_source. The debugfs files are
> + * automatically set up if that vfunc is set. CRC samples need to be captured
> + * in the driver by calling drm_crtc_add_crc_entry(). Depending on the driver
> + * and HW requirements, &drm_crtc_funcs.set_crc_source may result in a commit
> + * (even a full modeset).
> + *
> + * It's also possible that a "normal" commit via DRM_IOCTL_MODE_ATOMIC or the
> + * legacy paths may interfere with CRC generation. So, in the general case,
> + * userspace can't rely on the values in crtc-N/crc/data being valid
> + * across a commit.

It's not just the values, but the generation itself might get disabled
(e.g. on i915 if you select "auto" on some chips you get the DP port
sampling point, but for HDMI mode you get a different sampling ploint, and
if you get the wrong one there won't be any crc for you). Also it's not
just any atomic commit, only the ones with ALLOW_MODESET.

Maybe something like the below text:

"Please note that an atomic modeset commit with the
DRM_MODE_ATOMIC_ALLOW_MODESET, or a call to the legacy SETCRTR ioctl
(which amounts to the same), can destry the CRC setup due to hardware
requirements. This results in inconsistent CRC values or not even any CRC
values generated. Generic userspace therefore needs to re-setup the CRC
after each such modeset."

>
> static int crc_control_show(struct seq_file *m, void *data)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 128d8b210621..0f7ea094a900 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -756,6 +756,8 @@ struct drm_crtc_funcs {
> * provided from the configured source. Drivers must accept an "auto"
> * source name that will select a default source for this CRTC.
> *
> + * This may trigger a commit if necessary, to enable CRC generation.

I'd clarify this as "atomic modeset commit" just to be sure.

With these two comments addressed somehow:

Reviewed-by: Daniel Vetter <[email protected]>


> + *
> * Note that "auto" can depend upon the current modeset configuration,
> * e.g. it could pick an encoder or output specific CRC sampling point.
> *
> @@ -767,6 +769,7 @@ struct drm_crtc_funcs {
> * 0 on success or a negative error code on failure.
> */
> int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
> +
> /**
> * @verify_crc_source:
> *
> --
> 2.17.1
>

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

2019-08-05 16:56:27

by Brian Starkey

Subject: Re: [PATCH] drm/crc-debugfs: Add notes about CRC<->commit interactions

On Mon, Aug 05, 2019 at 06:24:17PM +0200, Daniel Vetter wrote:
> On Mon, Aug 05, 2019 at 04:11:43PM +0100, Brian Starkey wrote:
> > CRC generation can be impacted by commits coming from userspace, and
> > enabling CRC generation may itself trigger a commit. Add notes about
> > this to the kerneldoc.
> >
> > Signed-off-by: Brian Starkey <[email protected]>
> > ---
> >
> > I might have got the wrong end of the stick, but this is what I
> > understood from what you said.
> >
> > Cheers,
> > -Brian
> >
> > drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++----
> > include/drm/drm_crtc.h | 3 +++
> > 2 files changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> > index 7ca486d750e9..1dff956bcc74 100644
> > --- a/drivers/gpu/drm/drm_debugfs_crc.c
> > +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> > @@ -65,10 +65,17 @@
> > * it submits. In this general case, the maximum userspace can do is to compare
> > * the reported CRCs of frames that should have the same contents.
> > *
> > - * On the driver side the implementation effort is minimal, drivers only need to
> > - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically
> > - * set up if that vfunc is set. CRC samples need to be captured in the driver by
> > - * calling drm_crtc_add_crc_entry().
> > + * On the driver side the implementation effort is minimal, drivers only need
> > + * to implement &drm_crtc_funcs.set_crc_source. The debugfs files are
> > + * automatically set up if that vfunc is set. CRC samples need to be captured
> > + * in the driver by calling drm_crtc_add_crc_entry(). Depending on the driver
> > + * and HW requirements, &drm_crtc_funcs.set_crc_source may result in a commit
> > + * (even a full modeset).
> > + *
> > + * It's also possible that a "normal" commit via DRM_IOCTL_MODE_ATOMIC or the
> > + * legacy paths may interfere with CRC generation. So, in the general case,
> > + * userspace can't rely on the values in crtc-N/crc/data being valid
> > + * across a commit.
>
> It's not just the values, but the generation itself might get disabled
> (e.g. on i915 if you select "auto" on some chips you get the DP port
> sampling point, but for HDMI mode you get a different sampling ploint, and
> if you get the wrong one there won't be any crc for you). Also it's not
> just any atomic commit, only the ones with ALLOW_MODESET.

Is the meaning of ALLOW_MODESET actually defined somewhere then? I
thought it was broadly speaking "anything that would cause a flicker
on the output" - but that needn't be the same set of things that break
CRC generation.

>
> Maybe something like the below text:
>
> "Please note that an atomic modeset commit with the
> DRM_MODE_ATOMIC_ALLOW_MODESET, or a call to the legacy SETCRTR ioctl
> (which amounts to the same), can destry the CRC setup due to hardware
> requirements. This results in inconsistent CRC values or not even any CRC
> values generated. Generic userspace therefore needs to re-setup the CRC
> after each such modeset."
>
> >
> > static int crc_control_show(struct seq_file *m, void *data)
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 128d8b210621..0f7ea094a900 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -756,6 +756,8 @@ struct drm_crtc_funcs {
> > * provided from the configured source. Drivers must accept an "auto"
> > * source name that will select a default source for this CRTC.
> > *
> > + * This may trigger a commit if necessary, to enable CRC generation.
>
> I'd clarify this as "atomic modeset commit" just to be sure.

Ack.

Thanks,
-Brian

>
> With these two comments addressed somehow:
>
> Reviewed-by: Daniel Vetter <[email protected]>
>
>
> > + *
> > * Note that "auto" can depend upon the current modeset configuration,
> > * e.g. it could pick an encoder or output specific CRC sampling point.
> > *
> > @@ -767,6 +769,7 @@ struct drm_crtc_funcs {
> > * 0 on success or a negative error code on failure.
> > */
> > int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
> > +
> > /**
> > * @verify_crc_source:
> > *
> > --
> > 2.17.1
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

2019-08-06 09:13:37

by Daniel Vetter

Subject: Re: [PATCH] drm/crc-debugfs: Add notes about CRC<->commit interactions

On Mon, Aug 05, 2019 at 04:54:15PM +0000, Brian Starkey wrote:
> On Mon, Aug 05, 2019 at 06:24:17PM +0200, Daniel Vetter wrote:
> > On Mon, Aug 05, 2019 at 04:11:43PM +0100, Brian Starkey wrote:
> > > CRC generation can be impacted by commits coming from userspace, and
> > > enabling CRC generation may itself trigger a commit. Add notes about
> > > this to the kerneldoc.
> > >
> > > Signed-off-by: Brian Starkey <[email protected]>
> > > ---
> > >
> > > I might have got the wrong end of the stick, but this is what I
> > > understood from what you said.
> > >
> > > Cheers,
> > > -Brian
> > >
> > > drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++----
> > > include/drm/drm_crtc.h | 3 +++
> > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> > > index 7ca486d750e9..1dff956bcc74 100644
> > > --- a/drivers/gpu/drm/drm_debugfs_crc.c
> > > +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> > > @@ -65,10 +65,17 @@
> > > * it submits. In this general case, the maximum userspace can do is to compare
> > > * the reported CRCs of frames that should have the same contents.
> > > *
> > > - * On the driver side the implementation effort is minimal, drivers only need to
> > > - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically
> > > - * set up if that vfunc is set. CRC samples need to be captured in the driver by
> > > - * calling drm_crtc_add_crc_entry().
> > > + * On the driver side the implementation effort is minimal, drivers only need
> > > + * to implement &drm_crtc_funcs.set_crc_source. The debugfs files are
> > > + * automatically set up if that vfunc is set. CRC samples need to be captured
> > > + * in the driver by calling drm_crtc_add_crc_entry(). Depending on the driver
> > > + * and HW requirements, &drm_crtc_funcs.set_crc_source may result in a commit
> > > + * (even a full modeset).
> > > + *
> > > + * It's also possible that a "normal" commit via DRM_IOCTL_MODE_ATOMIC or the
> > > + * legacy paths may interfere with CRC generation. So, in the general case,
> > > + * userspace can't rely on the values in crtc-N/crc/data being valid
> > > + * across a commit.
> >
> > It's not just the values, but the generation itself might get disabled
> > (e.g. on i915 if you select "auto" on some chips you get the DP port
> > sampling point, but for HDMI mode you get a different sampling ploint, and
> > if you get the wrong one there won't be any crc for you). Also it's not
> > just any atomic commit, only the ones with ALLOW_MODESET.
>
> Is the meaning of ALLOW_MODESET actually defined somewhere then? I
> thought it was broadly speaking "anything that would cause a flicker
> on the output" - but that needn't be the same set of things that break
> CRC generation.

It's the inverse, I think we should require that crc keeps working for
non-ALLOW_MODESET. Otherwise crc are essentially useless for validating
stuff. And yeah allow_modeset is "could flicker and/or take enormous
amounts of time".
-Daniel

> > Maybe something like the below text:
> >
> > "Please note that an atomic modeset commit with the
> > DRM_MODE_ATOMIC_ALLOW_MODESET, or a call to the legacy SETCRTR ioctl
> > (which amounts to the same), can destry the CRC setup due to hardware
> > requirements. This results in inconsistent CRC values or not even any CRC
> > values generated. Generic userspace therefore needs to re-setup the CRC
> > after each such modeset."
> >
> > >
> > > static int crc_control_show(struct seq_file *m, void *data)
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > index 128d8b210621..0f7ea094a900 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -756,6 +756,8 @@ struct drm_crtc_funcs {
> > > * provided from the configured source. Drivers must accept an "auto"
> > > * source name that will select a default source for this CRTC.
> > > *
> > > + * This may trigger a commit if necessary, to enable CRC generation.
> >
> > I'd clarify this as "atomic modeset commit" just to be sure.
>
> Ack.
>
> Thanks,
> -Brian
>
> >
> > With these two comments addressed somehow:
> >
> > Reviewed-by: Daniel Vetter <[email protected]>
> >
> >
> > > + *
> > > * Note that "auto" can depend upon the current modeset configuration,
> > > * e.g. it could pick an encoder or output specific CRC sampling point.
> > > *
> > > @@ -767,6 +769,7 @@ struct drm_crtc_funcs {
> > > * 0 on success or a negative error code on failure.
> > > */
> > > int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
> > > +
> > > /**
> > > * @verify_crc_source:
> > > *
> > > --
> > > 2.17.1
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

2019-08-06 10:49:38

by Brian Starkey

Subject: [PATCH v2] drm/crc-debugfs: Add notes about CRC<->commit interactions

CRC generation can be impacted by commits coming from userspace, and
enabling CRC generation may itself trigger a commit. Add notes about
this to the kerneldoc.

Signed-off-by: Brian Starkey <[email protected]>
---
drivers/gpu/drm/drm_debugfs_crc.c | 17 +++++++++++++----
include/drm/drm_crtc.h | 4 ++++
2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 7ca486d750e9..77159b6e77c3 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -65,10 +65,19 @@
* it submits. In this general case, the maximum userspace can do is to compare
* the reported CRCs of frames that should have the same contents.
*
- * On the driver side the implementation effort is minimal, drivers only need to
- * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically
- * set up if that vfunc is set. CRC samples need to be captured in the driver by
- * calling drm_crtc_add_crc_entry().
+ * On the driver side the implementation effort is minimal, drivers only need
+ * to implement &drm_crtc_funcs.set_crc_source. The debugfs files are
+ * automatically set up if that vfunc is set. CRC samples need to be captured
+ * in the driver by calling drm_crtc_add_crc_entry(). Depending on the driver
+ * and HW requirements, &drm_crtc_funcs.set_crc_source may result in a commit
+ * (even a full modeset).
+ *
+ * CRC results must be reliable across non-full-modeset atomic commits, so if a
+ * commit via DRM_IOCTL_MODE_ATOMIC would disable or otherwise interfere with
+ * CRC generation, then the driver must mark that commit as a full modeset
+ * (drm_atomic_crtc_needs_modeset() should return true). As a result, to ensure
+ * consistent results, generic userspace must re-setup CRC generation after a
+ * legacy SETCRTC or an atomic commit with DRM_MODE_ATOMIC_ALLOW_MODESET.
*/

static int crc_control_show(struct seq_file *m, void *data)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 128d8b210621..7d14c11bdc0a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -756,6 +756,9 @@ struct drm_crtc_funcs {
* provided from the configured source. Drivers must accept an "auto"
* source name that will select a default source for this CRTC.
*
+ * This may trigger an atomic modeset commit if necessary, to enable CRC
+ * generation.
+ *
* Note that "auto" can depend upon the current modeset configuration,
* e.g. it could pick an encoder or output specific CRC sampling point.
*
@@ -767,6 +770,7 @@ struct drm_crtc_funcs {
* 0 on success or a negative error code on failure.
*/
int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
+
/**
* @verify_crc_source:
*
--
2.17.1

2019-08-06 11:47:04

by Daniel Vetter

Subject: Re: [PATCH v2] drm/crc-debugfs: Add notes about CRC<->commit interactions

On Tue, Aug 6, 2019 at 12:48 PM Brian Starkey <[email protected]> wrote:
>
> CRC generation can be impacted by commits coming from userspace, and
> enabling CRC generation may itself trigger a commit. Add notes about
> this to the kerneldoc.
>
> Signed-off-by: Brian Starkey <[email protected]>

lgtm, my earlier r-b holds. Well maybe should have a v2 here somewhere
with what you changed.
-Daniel
> ---
> drivers/gpu/drm/drm_debugfs_crc.c | 17 +++++++++++++----
> include/drm/drm_crtc.h | 4 ++++
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 7ca486d750e9..77159b6e77c3 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -65,10 +65,19 @@
> * it submits. In this general case, the maximum userspace can do is to compare
> * the reported CRCs of frames that should have the same contents.
> *
> - * On the driver side the implementation effort is minimal, drivers only need to
> - * implement &drm_crtc_funcs.set_crc_source. The debugfs files are automatically
> - * set up if that vfunc is set. CRC samples need to be captured in the driver by
> - * calling drm_crtc_add_crc_entry().
> + * On the driver side the implementation effort is minimal, drivers only need
> + * to implement &drm_crtc_funcs.set_crc_source. The debugfs files are
> + * automatically set up if that vfunc is set. CRC samples need to be captured
> + * in the driver by calling drm_crtc_add_crc_entry(). Depending on the driver
> + * and HW requirements, &drm_crtc_funcs.set_crc_source may result in a commit
> + * (even a full modeset).
> + *
> + * CRC results must be reliable across non-full-modeset atomic commits, so if a
> + * commit via DRM_IOCTL_MODE_ATOMIC would disable or otherwise interfere with
> + * CRC generation, then the driver must mark that commit as a full modeset
> + * (drm_atomic_crtc_needs_modeset() should return true). As a result, to ensure
> + * consistent results, generic userspace must re-setup CRC generation after a
> + * legacy SETCRTC or an atomic commit with DRM_MODE_ATOMIC_ALLOW_MODESET.
> */
>
> static int crc_control_show(struct seq_file *m, void *data)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 128d8b210621..7d14c11bdc0a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -756,6 +756,9 @@ struct drm_crtc_funcs {
> * provided from the configured source. Drivers must accept an "auto"
> * source name that will select a default source for this CRTC.
> *
> + * This may trigger an atomic modeset commit if necessary, to enable CRC
> + * generation.
> + *
> * Note that "auto" can depend upon the current modeset configuration,
> * e.g. it could pick an encoder or output specific CRC sampling point.
> *
> @@ -767,6 +770,7 @@ struct drm_crtc_funcs {
> * 0 on success or a negative error code on failure.
> */
> int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
> +
> /**
> * @verify_crc_source:
> *
> --
> 2.17.1
>


--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch