| Message ID | 20240821102349.3961986-3-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Display Global histogram | expand |
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun > R Murthy > Sent: Wednesday, August 21, 2024 3:54 PM > To: intel-gfx@lists.freedesktop.org > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > Subject: [PATCHv2 2/5] drm/i915/display: histogram interrupt handling > > Upon enabling histogram an interrupt is trigerred after the generation of the > statistics. This patch registers the histogram interrupt and handles the > interrupt. > > v2: Added intel_crtc backpointer to intel_histogram struct (Jani) > Removed histogram_wq and instead use dev_priv->unodered_eq (Jani) > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > .../gpu/drm/i915/display/intel_display_irq.c | 6 +- > .../gpu/drm/i915/display/intel_histogram.c | 80 ++++++++++++++++++- > .../gpu/drm/i915/display/intel_histogram.h | 3 + > drivers/gpu/drm/i915/i915_reg.h | 5 +- > 4 files changed, 89 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c > b/drivers/gpu/drm/i915/display/intel_display_irq.c > index afcd2af82942..0178595102bb 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" > @@ -1170,6 +1171,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, > @@ -1701,7 +1705,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 45e968e00af6..83ba826a7a89 100644 > --- a/drivers/gpu/drm/i915/display/intel_histogram.c > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c > @@ -19,12 +19,83 @@ > > struct intel_histogram { > struct drm_i915_private *i915; > + struct intel_crtc *crtc; > + struct delayed_work handle_histogram_int_work; > bool enable; > bool can_enable; > - enum pipe pipe; > u32 bindata[HISTOGRAM_BIN_COUNT]; > }; > > +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 = histogram->crtc; > + char *histogram_event[] = {"HISTOGRAM=1", NULL}; > + u32 dpstbin; > + int i, try = 0; > + If we have PSR enabled looks like this code might straight away break, and PSR being enabled is a common scenario Can we have some checks on enabling this feature if no PSR until we handle this scenario? > + /* > + * 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 > + */ > +retry: > + intel_de_rmw(i915, DPST_CTL(intel_crtc->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(intel_crtc->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. > + */ > + if (try++ >= 5) { > + drm_err(&i915->drm, > + "Histogram block is busy, failed to > read\n"); > + intel_de_rmw(i915, DPST_GUARD(intel_crtc- > >pipe), > + > DPST_GUARD_HIST_EVENT_STATUS, 1); > + return; > + } > + goto retry; > + } > + 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(intel_crtc->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(intel_crtc->pipe), > + DPST_GUARD_HIST_EVENT_STATUS, 1); } > + > +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(i915->unordered_wq, > + &histogram->handle_histogram_int_work, 0); } > + > int intel_histogram_atomic_check(struct intel_crtc *intel_crtc) { > struct intel_histogram *histogram = intel_crtc->histogram; @@ - > 120,6 +191,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; > } > > @@ -181,6 +253,7 @@ void intel_histogram_deinit(struct intel_crtc > *intel_crtc) { > struct intel_histogram *histogram = intel_crtc->histogram; > > + cancel_delayed_work_sync(&histogram- > >handle_histogram_int_work); > kfree(histogram); > } > > @@ -196,9 +269,12 @@ int intel_histogram_init(struct intel_crtc *intel_crtc) > } > > intel_crtc->histogram = histogram; > - histogram->pipe = intel_crtc->pipe; > + histogram->crtc = intel_crtc; > histogram->can_enable = false; > > + INIT_DEFERRABLE_WORK(&histogram->handle_histogram_int_work, > + intel_histogram_handle_int_work); > + > histogram->i915 = i915; > > return 0; > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.h > b/drivers/gpu/drm/i915/display/intel_histogram.h > index b25091732274..f35ea76719d8 100644 > --- a/drivers/gpu/drm/i915/display/intel_histogram.h > +++ b/drivers/gpu/drm/i915/display/intel_histogram.h > @@ -9,6 +9,8 @@ > #include <linux/types.h> > > struct intel_crtc; > +struct drm_i915_private; > +enum pipe; > > /* GLOBAL_HIST related registers */ > #define _DPST_CTL_A 0x490C0 > @@ -70,6 +72,7 @@ enum intel_global_hist_lut { }; > > int intel_histogram_atomic_check(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 569b461022c5..f7b974691381 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) > @@ -2516,6 +2516,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 */ > -- > 2.25.1
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun > R Murthy > Sent: Wednesday, August 21, 2024 3:54 PM > To: intel-gfx@lists.freedesktop.org > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > Subject: [PATCHv2 2/5] drm/i915/display: histogram interrupt handling > > Upon enabling histogram an interrupt is trigerred after the generation of the > statistics. This patch registers the histogram interrupt and handles the > interrupt. > > v2: Added intel_crtc backpointer to intel_histogram struct (Jani) > Removed histogram_wq and instead use dev_priv->unodered_eq (Jani) Typo here "wq" > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > .../gpu/drm/i915/display/intel_display_irq.c | 6 +- > .../gpu/drm/i915/display/intel_histogram.c | 80 ++++++++++++++++++- > .../gpu/drm/i915/display/intel_histogram.h | 3 + > drivers/gpu/drm/i915/i915_reg.h | 5 +- > 4 files changed, 89 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c > b/drivers/gpu/drm/i915/display/intel_display_irq.c > index afcd2af82942..0178595102bb 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" > @@ -1170,6 +1171,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, > @@ -1701,7 +1705,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 45e968e00af6..83ba826a7a89 100644 > --- a/drivers/gpu/drm/i915/display/intel_histogram.c > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c > @@ -19,12 +19,83 @@ > > struct intel_histogram { > struct drm_i915_private *i915; > + struct intel_crtc *crtc; > + struct delayed_work handle_histogram_int_work; Too long of a name delayed_work should be enough since it will be called as Histogram->delayed_work which is self explanatory > bool enable; > bool can_enable; > - enum pipe pipe; > u32 bindata[HISTOGRAM_BIN_COUNT]; > }; > > +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; Lets not use it like this here we have intel_crtc use this to get intel_display by to_intel_display(crtc) and I don't see the use for i915 since display will do the job > + struct intel_crtc *intel_crtc = histogram->crtc; > + 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 > + */ You may have to think on how you will be able to get the intel_dp structure here You may need to add it in you histogram structure to get intel_psr structure Then check if psr->enabled And suspend and resume psr based on that Something to think about > +retry: > + intel_de_rmw(i915, DPST_CTL(intel_crtc->pipe), I915 can be replaced with intel_display . can be also replaced in all other Occurrences of i915 in this patch where we don't need the unordered_wq > + 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(intel_crtc->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. > + */ > + if (try++ >= 5) { > + drm_err(&i915->drm, > + "Histogram block is busy, failed to > read\n"); > + intel_de_rmw(i915, DPST_GUARD(intel_crtc- > >pipe), > + > DPST_GUARD_HIST_EVENT_STATUS, 1); > + return; > + } > + goto retry; > + } > + 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 */ Typo "readiness" > + 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(intel_crtc->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(intel_crtc->pipe), > + DPST_GUARD_HIST_EVENT_STATUS, 1); } > + > +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(i915->unordered_wq, > + &histogram->handle_histogram_int_work, 0); } > + > int intel_histogram_atomic_check(struct intel_crtc *intel_crtc) { > struct intel_histogram *histogram = intel_crtc->histogram; @@ - > 120,6 +191,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; > } > > @@ -181,6 +253,7 @@ void intel_histogram_deinit(struct intel_crtc > *intel_crtc) { > struct intel_histogram *histogram = intel_crtc->histogram; > > + cancel_delayed_work_sync(&histogram- > >handle_histogram_int_work); > kfree(histogram); > } > > @@ -196,9 +269,12 @@ int intel_histogram_init(struct intel_crtc *intel_crtc) > } > > intel_crtc->histogram = histogram; > - histogram->pipe = intel_crtc->pipe; > + histogram->crtc = intel_crtc; Why even add it in patch 1 to delete it patch 2 lets not add it to begin with, Also we don't need the intel_pipe variable in intel_histogram since it can be derived from Intel_crtc. Regards, Suraj Kandpal > histogram->can_enable = false; > > + INIT_DEFERRABLE_WORK(&histogram->handle_histogram_int_work, > + intel_histogram_handle_int_work); > + > histogram->i915 = i915; > > return 0; > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.h > b/drivers/gpu/drm/i915/display/intel_histogram.h > index b25091732274..f35ea76719d8 100644 > --- a/drivers/gpu/drm/i915/display/intel_histogram.h > +++ b/drivers/gpu/drm/i915/display/intel_histogram.h > @@ -9,6 +9,8 @@ > #include <linux/types.h> > > struct intel_crtc; > +struct drm_i915_private; > +enum pipe; > > /* GLOBAL_HIST related registers */ > #define _DPST_CTL_A 0x490C0 > @@ -70,6 +72,7 @@ enum intel_global_hist_lut { }; > > int intel_histogram_atomic_check(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 569b461022c5..f7b974691381 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) > @@ -2516,6 +2516,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 */ > -- > 2.25.1
> > +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 = histogram->crtc; > > + char *histogram_event[] = {"HISTOGRAM=1", NULL}; > > + u32 dpstbin; > > + int i, try = 0; > > + > If we have PSR enabled looks like this code might straight away break, and PSR > being enabled is a common scenario Can we have some checks on enabling this > feature if no PSR until we handle this scenario? With PSR enabled histogram event will not be generated as there wont be any statistics. This should have no impact to user and user is not time bound with the histogram event. This TODO is to handle the histogram generation even in case of PSR enabled with some additional settings. Thanks and Regards, Arun R Murthy -------------------
On Wed, 21 Aug 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote: > Upon enabling histogram an interrupt is trigerred after the generation > of the statistics. This patch registers the histogram interrupt and > handles the interrupt. > > v2: Added intel_crtc backpointer to intel_histogram struct (Jani) > Removed histogram_wq and instead use dev_priv->unodered_eq (Jani) > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > .../gpu/drm/i915/display/intel_display_irq.c | 6 +- > .../gpu/drm/i915/display/intel_histogram.c | 80 ++++++++++++++++++- > .../gpu/drm/i915/display/intel_histogram.h | 3 + > drivers/gpu/drm/i915/i915_reg.h | 5 +- > 4 files changed, 89 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c > index afcd2af82942..0178595102bb 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" > @@ -1170,6 +1171,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, > @@ -1701,7 +1705,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 45e968e00af6..83ba826a7a89 100644 > --- a/drivers/gpu/drm/i915/display/intel_histogram.c > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c > @@ -19,12 +19,83 @@ > > struct intel_histogram { > struct drm_i915_private *i915; Don't need this if you have crtc pointer. > + struct intel_crtc *crtc; > + struct delayed_work handle_histogram_int_work; > bool enable; > bool can_enable; > - enum pipe pipe; Why not add crtc to begin with in patch 1? > u32 bindata[HISTOGRAM_BIN_COUNT]; > }; > > +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_display everywhere in the series. > + struct intel_crtc *intel_crtc = histogram->crtc; > + 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 > + */ > +retry: > + intel_de_rmw(i915, DPST_CTL(intel_crtc->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(intel_crtc->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. > + */ > + if (try++ >= 5) { > + drm_err(&i915->drm, > + "Histogram block is busy, failed to read\n"); > + intel_de_rmw(i915, DPST_GUARD(intel_crtc->pipe), > + DPST_GUARD_HIST_EVENT_STATUS, 1); > + return; > + } > + goto retry; > + } The retry logic seems pretty weird here with the goto. Maybe abstract a separate function to avoid it. > + histogram->bindata[i] = dpstbin & DPST_BIN_DATA_MASK; > + drm_dbg_atomic(&i915->drm, "Histogram[%d]=%x\n", > + i, histogram->bindata[i]); What's atomic about this? Also please consider the amount of log spew this generates. If you need to log it, do it in a single hex dump after everything is read. > + } > + > + /* 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(intel_crtc->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(intel_crtc->pipe), > + DPST_GUARD_HIST_EVENT_STATUS, 1); > +} > + > +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(i915->unordered_wq, > + &histogram->handle_histogram_int_work, 0); > +} > + > int intel_histogram_atomic_check(struct intel_crtc *intel_crtc) > { > struct intel_histogram *histogram = intel_crtc->histogram; > @@ -120,6 +191,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; > } > > @@ -181,6 +253,7 @@ void intel_histogram_deinit(struct intel_crtc *intel_crtc) > { > struct intel_histogram *histogram = intel_crtc->histogram; > > + cancel_delayed_work_sync(&histogram->handle_histogram_int_work); > kfree(histogram); > } > > @@ -196,9 +269,12 @@ int intel_histogram_init(struct intel_crtc *intel_crtc) > } > > intel_crtc->histogram = histogram; > - histogram->pipe = intel_crtc->pipe; > + histogram->crtc = intel_crtc; > histogram->can_enable = false; > > + INIT_DEFERRABLE_WORK(&histogram->handle_histogram_int_work, > + intel_histogram_handle_int_work); > + > histogram->i915 = i915; > > return 0; > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.h b/drivers/gpu/drm/i915/display/intel_histogram.h > index b25091732274..f35ea76719d8 100644 > --- a/drivers/gpu/drm/i915/display/intel_histogram.h > +++ b/drivers/gpu/drm/i915/display/intel_histogram.h > @@ -9,6 +9,8 @@ > #include <linux/types.h> > > struct intel_crtc; > +struct drm_i915_private; > +enum pipe; > > /* GLOBAL_HIST related registers */ > #define _DPST_CTL_A 0x490C0 > @@ -70,6 +72,7 @@ enum intel_global_hist_lut { > }; > > int intel_histogram_atomic_check(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 569b461022c5..f7b974691381 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) > @@ -2516,6 +2516,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 */
> -----Original Message----- > From: Murthy, Arun R <arun.r.murthy@intel.com> > Sent: Thursday, September 12, 2024 3:22 PM > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: RE: [PATCHv2 2/5] drm/i915/display: histogram interrupt handling > > > > +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 = histogram->crtc; > > > + char *histogram_event[] = {"HISTOGRAM=1", NULL}; > > > + u32 dpstbin; > > > + int i, try = 0; > > > + > > If we have PSR enabled looks like this code might straight away break, > > and PSR being enabled is a common scenario Can we have some checks on > > enabling this feature if no PSR until we handle this scenario? > > With PSR enabled histogram event will not be generated as there wont be > any statistics. > This should have no impact to user and user is not time bound with the > histogram event. Just to get some clarity, does this mean the tests for histogram wont fail on PSR enabled panel with this series ? Thanks, Vandita > > This TODO is to handle the histogram generation even in case of PSR enabled > with some additional settings. > > Thanks and Regards, > Arun R Murthy > -------------------
> > Upon enabling histogram an interrupt is trigerred after the generation > > of the statistics. This patch registers the histogram interrupt and > > handles the interrupt. > > > > v2: Added intel_crtc backpointer to intel_histogram struct (Jani) > > Removed histogram_wq and instead use dev_priv->unodered_eq (Jani) > > Typo here "wq" Done > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > --- > > .../gpu/drm/i915/display/intel_display_irq.c | 6 +- > > .../gpu/drm/i915/display/intel_histogram.c | 80 ++++++++++++++++++- > > .../gpu/drm/i915/display/intel_histogram.h | 3 + > > drivers/gpu/drm/i915/i915_reg.h | 5 +- > > 4 files changed, 89 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c > > b/drivers/gpu/drm/i915/display/intel_display_irq.c > > index afcd2af82942..0178595102bb 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" > > @@ -1170,6 +1171,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, > > @@ -1701,7 +1705,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 45e968e00af6..83ba826a7a89 100644 > > --- a/drivers/gpu/drm/i915/display/intel_histogram.c > > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c > > @@ -19,12 +19,83 @@ > > > > struct intel_histogram { > > struct drm_i915_private *i915; > > + struct intel_crtc *crtc; > > + struct delayed_work handle_histogram_int_work; > > Too long of a name delayed_work should be enough since it will be called as > Histogram->delayed_work which is self explanatory > Done > > bool enable; > > bool can_enable; > > - enum pipe pipe; > > u32 bindata[HISTOGRAM_BIN_COUNT]; > > }; > > > > +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; > > Lets not use it like this here we have intel_crtc use this to get intel_display by > to_intel_display(crtc) and I don't see the use for i915 since display will do the > job > Done > > + struct intel_crtc *intel_crtc = histogram->crtc; > > + 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 > > + */ > > You may have to think on how you will be able to get the intel_dp structure > here You may need to add it in you histogram structure to get intel_psr > structure Then check if psr->enabled And suspend and resume psr based on > that Something to think about > Yes will take this up later. > > +retry: > > + intel_de_rmw(i915, DPST_CTL(intel_crtc->pipe), > > I915 can be replaced with intel_display . can be also replaced in all other > Occurrences of i915 in this patch where we don't need the unordered_wq > Done > > > + 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(intel_crtc->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. > > + */ > > + if (try++ >= 5) { > > + drm_err(&i915->drm, > > + "Histogram block is busy, failed to > > read\n"); > > + intel_de_rmw(i915, DPST_GUARD(intel_crtc- > > >pipe), > > + > > DPST_GUARD_HIST_EVENT_STATUS, 1); > > + return; > > + } > > + goto retry; > > + } > > + 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 */ > > Typo "readiness" > > > + 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(intel_crtc->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(intel_crtc->pipe), > > + DPST_GUARD_HIST_EVENT_STATUS, 1); } > > + > > +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(i915->unordered_wq, > > + &histogram->handle_histogram_int_work, 0); } > > + > > int intel_histogram_atomic_check(struct intel_crtc *intel_crtc) { > > struct intel_histogram *histogram = intel_crtc->histogram; @@ - > > 120,6 +191,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; > > } > > > > @@ -181,6 +253,7 @@ void intel_histogram_deinit(struct intel_crtc > > *intel_crtc) { > > struct intel_histogram *histogram = intel_crtc->histogram; > > > > + cancel_delayed_work_sync(&histogram- > > >handle_histogram_int_work); > > kfree(histogram); > > } > > > > @@ -196,9 +269,12 @@ int intel_histogram_init(struct intel_crtc *intel_crtc) > > } > > > > intel_crtc->histogram = histogram; > > - histogram->pipe = intel_crtc->pipe; > > + histogram->crtc = intel_crtc; > > Why even add it in patch 1 to delete it patch 2 lets not add it to begin with, > Also we don't need the intel_pipe variable in intel_histogram since it can be > derived from Intel_crtc. > Done Thanks and Regards, Arun R Murthy ---------------------
> > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c > > b/drivers/gpu/drm/i915/display/intel_histogram.c > > index 45e968e00af6..83ba826a7a89 100644 > > --- a/drivers/gpu/drm/i915/display/intel_histogram.c > > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c > > @@ -19,12 +19,83 @@ > > > > struct intel_histogram { > > struct drm_i915_private *i915; > > Don't need this if you have crtc pointer. > As part of replacing drm_i915_private with intel_display, this has been taken care of(Comment from Suraj). > > + struct intel_crtc *crtc; > > + struct delayed_work handle_histogram_int_work; > > bool enable; > > bool can_enable; > > - enum pipe pipe; > > Why not add crtc to begin with in patch 1? > Done as part of replacing drm_i915_private to intel_dispay(comment from Suraj) > > u32 bindata[HISTOGRAM_BIN_COUNT]; > > }; > > > > +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_display everywhere in the series. > Done! As part of comments from Suraj. > > + struct intel_crtc *intel_crtc = histogram->crtc; > > + 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 > > + */ > > +retry: > > + intel_de_rmw(i915, DPST_CTL(intel_crtc->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(intel_crtc->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. > > + */ > > + if (try++ >= 5) { > > + drm_err(&i915->drm, > > + "Histogram block is busy, failed to > read\n"); > > + intel_de_rmw(i915, DPST_GUARD(intel_crtc- > >pipe), > > + DPST_GUARD_HIST_EVENT_STATUS, > 1); > > + return; > > + } > > + goto retry; > > + } > > The retry logic seems pretty weird here with the goto. Maybe abstract a > separate function to avoid it. > > Done > > + histogram->bindata[i] = dpstbin & DPST_BIN_DATA_MASK; > > + drm_dbg_atomic(&i915->drm, "Histogram[%d]=%x\n", > > + i, histogram->bindata[i]); > > What's atomic about this? Also please consider the amount of log spew this > generates. If you need to log it, do it in a single hex dump after everything is > read. > Will remove this dbg print, may not be required for normal debugging. Thanks and Regards, Arun R Murthy --------------------
diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c index afcd2af82942..0178595102bb 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" @@ -1170,6 +1171,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, @@ -1701,7 +1705,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 45e968e00af6..83ba826a7a89 100644 --- a/drivers/gpu/drm/i915/display/intel_histogram.c +++ b/drivers/gpu/drm/i915/display/intel_histogram.c @@ -19,12 +19,83 @@ struct intel_histogram { struct drm_i915_private *i915; + struct intel_crtc *crtc; + struct delayed_work handle_histogram_int_work; bool enable; bool can_enable; - enum pipe pipe; u32 bindata[HISTOGRAM_BIN_COUNT]; }; +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 = histogram->crtc; + 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 + */ +retry: + intel_de_rmw(i915, DPST_CTL(intel_crtc->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(intel_crtc->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. + */ + if (try++ >= 5) { + drm_err(&i915->drm, + "Histogram block is busy, failed to read\n"); + intel_de_rmw(i915, DPST_GUARD(intel_crtc->pipe), + DPST_GUARD_HIST_EVENT_STATUS, 1); + return; + } + goto retry; + } + 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(intel_crtc->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(intel_crtc->pipe), + DPST_GUARD_HIST_EVENT_STATUS, 1); +} + +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(i915->unordered_wq, + &histogram->handle_histogram_int_work, 0); +} + int intel_histogram_atomic_check(struct intel_crtc *intel_crtc) { struct intel_histogram *histogram = intel_crtc->histogram; @@ -120,6 +191,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; } @@ -181,6 +253,7 @@ void intel_histogram_deinit(struct intel_crtc *intel_crtc) { struct intel_histogram *histogram = intel_crtc->histogram; + cancel_delayed_work_sync(&histogram->handle_histogram_int_work); kfree(histogram); } @@ -196,9 +269,12 @@ int intel_histogram_init(struct intel_crtc *intel_crtc) } intel_crtc->histogram = histogram; - histogram->pipe = intel_crtc->pipe; + histogram->crtc = intel_crtc; histogram->can_enable = false; + INIT_DEFERRABLE_WORK(&histogram->handle_histogram_int_work, + intel_histogram_handle_int_work); + histogram->i915 = i915; return 0; diff --git a/drivers/gpu/drm/i915/display/intel_histogram.h b/drivers/gpu/drm/i915/display/intel_histogram.h index b25091732274..f35ea76719d8 100644 --- a/drivers/gpu/drm/i915/display/intel_histogram.h +++ b/drivers/gpu/drm/i915/display/intel_histogram.h @@ -9,6 +9,8 @@ #include <linux/types.h> struct intel_crtc; +struct drm_i915_private; +enum pipe; /* GLOBAL_HIST related registers */ #define _DPST_CTL_A 0x490C0 @@ -70,6 +72,7 @@ enum intel_global_hist_lut { }; int intel_histogram_atomic_check(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 569b461022c5..f7b974691381 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) @@ -2516,6 +2516,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 */
Upon enabling histogram an interrupt is trigerred after the generation of the statistics. This patch registers the histogram interrupt and handles the interrupt. v2: Added intel_crtc backpointer to intel_histogram struct (Jani) Removed histogram_wq and instead use dev_priv->unodered_eq (Jani) Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- .../gpu/drm/i915/display/intel_display_irq.c | 6 +- .../gpu/drm/i915/display/intel_histogram.c | 80 ++++++++++++++++++- .../gpu/drm/i915/display/intel_histogram.h | 3 + drivers/gpu/drm/i915/i915_reg.h | 5 +- 4 files changed, 89 insertions(+), 5 deletions(-)