On Fri, 05 Jul 2024, Arun R Murthy <arun.r.murthy@xxxxxxxxx> wrote:
> Upon enabling histogram an interrupt is trigerred after the generation
> of the statistics. This patch registers the histogram interrupt and
> handles the interrupt.
>
> Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> ---
> .../gpu/drm/i915/display/intel_display_irq.c | 6 +-
> .../gpu/drm/i915/display/intel_histogram.c | 93 +++++++++++++++++++
> .../gpu/drm/i915/display/intel_histogram.h | 3 +
> drivers/gpu/drm/i915/i915_reg.h | 5 +-
> 4 files changed, 104 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index 5219ba295c74..42fa2226b69e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> @@ -17,6 +17,7 @@
> #include "intel_fdi_regs.h"
> #include "intel_fifo_underrun.h"
> #include "intel_gmbus.h"
> +#include "intel_histogram.h"
> #include "intel_hotplug_irq.h"
> #include "intel_pipe_crc_regs.h"
> #include "intel_pmdemand.h"
> @@ -1155,6 +1156,9 @@ void gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> if (iir & gen8_de_pipe_underrun_mask(dev_priv))
> intel_cpu_fifo_underrun_irq_handler(dev_priv, pipe);
>
> + if (iir & GEN9_PIPE_HISTOGRAM_EVENT)
> + intel_histogram_irq_handler(dev_priv, pipe);
> +
> fault_errors = iir & gen8_de_pipe_fault_mask(dev_priv);
> if (fault_errors)
> drm_err_ratelimited(&dev_priv->drm,
> @@ -1683,7 +1687,7 @@ void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> struct intel_uncore *uncore = &dev_priv->uncore;
>
> u32 de_pipe_masked = gen8_de_pipe_fault_mask(dev_priv) |
> - GEN8_PIPE_CDCLK_CRC_DONE;
> + GEN8_PIPE_CDCLK_CRC_DONE | GEN9_PIPE_HISTOGRAM_EVENT;
> u32 de_pipe_enables;
> u32 de_port_masked = gen8_de_port_aux_mask(dev_priv);
> u32 de_port_enables;
> diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c
> index 1a603445fc29..8fa3bc74e52b 100644
> --- a/drivers/gpu/drm/i915/display/intel_histogram.c
> +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> @@ -11,6 +11,85 @@
> #include "intel_de.h"
> #include "intel_histogram.h"
>
> +static void intel_histogram_handle_int_work(struct work_struct *work)
> +{
> + struct intel_histogram *histogram = container_of(work,
> + struct intel_histogram, handle_histogram_int_work.work);
> + struct drm_i915_private *i915 = histogram->i915;
> + struct intel_crtc *intel_crtc =
> + to_intel_crtc(drm_crtc_from_index(&i915->drm, histogram->pipe));
You could just store the intel_crtc backpointer to histogram instead.
> + char *histogram_event[] = {"HISTOGRAM=1", NULL};
> + u32 dpstbin;
> + int i, try = 0;
> +
> + /*
> + * TODO: PSR to be exited while reading the Histogram data
> + * Set DPST_CTL Bin Reg function select to TC
> + * Set DPST_CTL Bin Register Index to 0
> + */
> + intel_de_rmw(i915, DPST_CTL(histogram->pipe),
> + DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_BIN_REG_MASK, 0);
> +
> + for (i = 0; i < HISTOGRAM_BIN_COUNT; i++) {
> + dpstbin = intel_de_read(i915, DPST_BIN(histogram->pipe));
> + if (dpstbin & DPST_BIN_BUSY) {
> + /*
> + * If DPST_BIN busy bit is set, then set the
> + * DPST_CTL bin reg index to 0 and proceed
> + * from beginning.
> + */
> + intel_de_rmw(i915, DPST_CTL(histogram->pipe),
> + DPST_CTL_BIN_REG_MASK, 0);
> + i = 0;
Please don't mess with the loop variables within the loop. It makes hard
to see what's really going on.
In this case, this becomes an infinite loop that you only break out by
getting DPST_BIN_BUSY six times, and you only get out of here by
returning.
> + if (try++ == 5) {
> + drm_err(&i915->drm,
> + "Histogram block is busy, failed to read\n");
> + intel_de_rmw(i915, DPST_GUARD(histogram->pipe),
> + DPST_GUARD_HIST_EVENT_STATUS, 1);
> + return;
> + }
> + }
> + histogram->bindata[i] = dpstbin & DPST_BIN_DATA_MASK;
> + drm_dbg_atomic(&i915->drm, "Histogram[%d]=%x\n",
> + i, histogram->bindata[i]);
> + }
> +
> + /* Notify user for Histogram rediness */
> + if (kobject_uevent_env(&i915->drm.primary->kdev->kobj, KOBJ_CHANGE,
> + histogram_event))
> + drm_err(&i915->drm, "sending HISTOGRAM event failed\n");
> +
> + /* Enable histogram interrupt */
> + intel_de_rmw(i915, DPST_GUARD(histogram->pipe), DPST_GUARD_HIST_INT_EN,
> + DPST_GUARD_HIST_INT_EN);
> +
> + /* Clear histogram interrupt by setting histogram interrupt status bit*/
> + intel_de_rmw(i915, DPST_GUARD(histogram->pipe),
> + DPST_GUARD_HIST_EVENT_STATUS, 1);
> +
> + drm_property_replace_global_blob(&i915->drm,
> + &intel_crtc->config->histogram,
> + sizeof(histogram->bindata),
> + histogram->bindata, &intel_crtc->base.base,
> + intel_crtc->histogram_property);
So you first notify and then replace the property, creating a race?
> +}
> +
> +void intel_histogram_irq_handler(struct drm_i915_private *i915, enum pipe pipe)
> +{
> + struct intel_crtc *intel_crtc =
> + to_intel_crtc(drm_crtc_from_index(&i915->drm, pipe));
> + struct intel_histogram *histogram = intel_crtc->histogram;
> +
> + if (!histogram->enable) {
> + drm_err(&i915->drm,
> + "spurious interrupt, histogram not enabled\n");
> + return;
> + }
> +
> + queue_delayed_work(histogram->wq,
> + &histogram->handle_histogram_int_work, 0);
> +}
> +
> int intel_histogram_can_enable(struct intel_crtc *intel_crtc)
> {
> struct intel_histogram *histogram = intel_crtc->histogram;
> @@ -102,6 +181,7 @@ static void intel_histogram_disable(struct intel_crtc *intel_crtc)
> intel_de_rmw(i915, DPST_CTL(pipe),
> DPST_CTL_IE_HIST_EN, 0);
>
> + cancel_delayed_work(&histogram->handle_histogram_int_work);
> histogram->enable = false;
> }
>
> @@ -160,6 +240,8 @@ void intel_histogram_deinit(struct intel_crtc *intel_crtc)
> {
> struct intel_histogram *histogram = intel_crtc->histogram;
>
> + cancel_delayed_work(&histogram->handle_histogram_int_work);
Probably want sync.
> + destroy_workqueue(histogram->wq);
> kfree(histogram);
> }
>
> @@ -180,6 +262,17 @@ int intel_histogram_init(struct intel_crtc *intel_crtc)
> intel_crtc->histogram = histogram;
> histogram->pipe = intel_crtc->pipe;
> histogram->can_enable = false;
> + histogram->wq = alloc_ordered_workqueue("histogram_wq",
> + WQ_MEM_RECLAIM);
Please don't add new workqueues, we have too many.
> + if (!histogram->wq) {
> + drm_err(&i915->drm,
> + "failed to create work queue\n");
> + kfree(histogram);
> + return -ENOMEM;
> + }
> +
> + INIT_DEFERRABLE_WORK(&histogram->handle_histogram_int_work,
> + intel_histogram_handle_int_work);
>
> histogram->i915 = i915;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_histogram.h b/drivers/gpu/drm/i915/display/intel_histogram.h
> index 632116c52882..f07683dde005 100644
> --- a/drivers/gpu/drm/i915/display/intel_histogram.h
> +++ b/drivers/gpu/drm/i915/display/intel_histogram.h
> @@ -76,6 +76,8 @@ enum intel_global_hist_lut {
>
> struct intel_histogram {
> struct drm_i915_private *i915;
> + struct workqueue_struct *wq;
> + struct delayed_work handle_histogram_int_work;
> bool enable;
> bool can_enable;
> enum pipe pipe;
> @@ -83,6 +85,7 @@ struct intel_histogram {
> };
>
> int intel_histogram_can_enable(struct intel_crtc *intel_crtc);
> +void intel_histogram_irq_handler(struct drm_i915_private *i915, enum pipe pipe);
> int intel_histogram_update(struct intel_crtc *intel_crtc, bool enable);
> int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc, u32 *data);
> int intel_histogram_init(struct intel_crtc *intel_crtc);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0e3d79227e3c..185d4b66cbb6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1655,7 +1655,7 @@
> #define PIPE_HOTPLUG_INTERRUPT_ENABLE (1UL << 26)
> #define PIPE_VSYNC_INTERRUPT_ENABLE (1UL << 25)
> #define PIPE_DISPLAY_LINE_COMPARE_ENABLE (1UL << 24)
> -#define PIPE_DPST_EVENT_ENABLE (1UL << 23)
> +#define PIPE_HISTOGRAM_EVENT_ENABLE (1UL << 23)
> #define SPRITE0_FLIP_DONE_INT_EN_VLV (1UL << 22)
> #define PIPE_LEGACY_BLC_EVENT_ENABLE (1UL << 22)
> #define PIPE_ODD_FIELD_INTERRUPT_ENABLE (1UL << 21)
> @@ -1678,7 +1678,7 @@
> #define PIPE_HOTPLUG_INTERRUPT_STATUS (1UL << 10)
> #define PIPE_VSYNC_INTERRUPT_STATUS (1UL << 9)
> #define PIPE_DISPLAY_LINE_COMPARE_STATUS (1UL << 8)
> -#define PIPE_DPST_EVENT_STATUS (1UL << 7)
> +#define PIPE_HISTOGRAM_EVENT_STATUS (1UL << 7)
> #define PIPE_A_PSR_STATUS_VLV (1UL << 6)
> #define PIPE_LEGACY_BLC_EVENT_STATUS (1UL << 6)
> #define PIPE_ODD_FIELD_INTERRUPT_STATUS (1UL << 5)
> @@ -2515,6 +2515,7 @@
> #define GEN11_PIPE_PLANE7_FLIP_DONE REG_BIT(18) /* icl/tgl */
> #define GEN11_PIPE_PLANE6_FLIP_DONE REG_BIT(17) /* icl/tgl */
> #define GEN11_PIPE_PLANE5_FLIP_DONE REG_BIT(16) /* icl+ */
> +#define GEN9_PIPE_HISTOGRAM_EVENT REG_BIT(12) /* skl+ */
> #define GEN9_PIPE_CURSOR_FAULT REG_BIT(11) /* skl+ */
> #define GEN9_PIPE_PLANE4_FAULT REG_BIT(10) /* skl+ */
> #define GEN8_PIPE_CURSOR_FAULT REG_BIT(10) /* bdw */
--
Jani Nikula, Intel
