> On Tue, 19 Nov 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.
> >
> > v2: Added intel_crtc backpointer to intel_histogram struct (Jani)
> > Removed histogram_wq and instead use dev_priv->unodered_eq (Jani)
> > v3: Replaced drm_i915_private with intel_display (Suraj)
> > Refactored the histogram read code (Jani)
> > v4: Rebased after addressing comments on patch 1
> >
> > 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 f0d3bdb5fc60..cb60c9db4418 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > @@ -20,6 +20,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"
> > @@ -1179,6 +1180,9 @@ void gen8_de_irq_handler(struct
> drm_i915_private *dev_priv, u32 master_ctl)
> > if (iir & GEN8_PIPE_FIFO_UNDERRUN)
> > intel_cpu_fifo_underrun_irq_handler(dev_priv, pipe);
> >
> > + if (iir & GEN9_PIPE_HISTOGRAM_EVENT)
> > + intel_histogram_irq_handler(display, pipe);
> > +
> > fault_errors = iir & gen8_de_pipe_fault_mask(dev_priv);
> > if (fault_errors)
> > drm_err_ratelimited(&dev_priv->drm,
> > @@ -1764,7 +1768,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 e751977fc6f7..16f29923ef10 100644
> > --- a/drivers/gpu/drm/i915/display/intel_histogram.c
> > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> > @@ -18,6 +18,8 @@
> > #define HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT 300 // 3.0% of
> the pipe's current pixel count.
> > #define HISTOGRAM_GUARDBAND_PRECISION_FACTOR 10000 // Precision
> factor for threshold guardband.
> > #define HISTOGRAM_DEFAULT_GUARDBAND_DELAY 0x04
> > +#define HISTOGRAM_BIN_READ_RETRY_COUNT 5 #define
> > +HISTOGRAM_BIN_READ_DELAY 2
>
> The above are only used once each. Please just put the values inline. Here, you
> don't even know what "2" means without looking up the code.
>
> >
> > struct intel_histogram {
> > struct intel_crtc *crtc;
> > @@ -27,6 +29,92 @@ struct intel_histogram {
> > u32 bin_data[HISTOGRAM_BIN_COUNT];
> > };
> >
> > +static bool intel_histogram_get_data(struct intel_crtc *intel_crtc) {
> > + struct intel_display *display = to_intel_display(intel_crtc);
> > + struct intel_histogram *histogram = intel_crtc->histogram;
> > + u8 index, retry_count;
>
> These should be ints.
>
> > + u32 dpstbin;
> > +
> > + index = 0;
> > + retry_count = 0;
> > +
> > + while (index < HISTOGRAM_BIN_COUNT) {
>
> This should be a for loop
>
> for (index = 0; index < ARRAY_SIZE(histogram->bin_data); i++)
>
> Any retries should be handled in a separate loop instead of abusing the top
> level loop for two things.
>
> Based on how this is now, looks like the retry loop should be outside of this
> function altogether? I.e. a single busy response leads to retry from start?
>
> > + dpstbin = intel_de_read(display, DPST_BIN(intel_crtc->pipe));
> > + if (!(dpstbin & DPST_BIN_BUSY)) {
> > + histogram->bin_data[index] = dpstbin &
> DPST_BIN_DATA_MASK;
> > + index++;
> > + } else {
> > + /*
> > + * If DPST_BIN busy bit is set, then set the
> > + * DPST_CTL bin reg index to 0 and proceed
> > + * from beginning.
> > + */
>
> That's just saying the same thing as the code in English. What's the point?
>
> > + retry_count++;
> > + if (retry_count >
> HISTOGRAM_BIN_READ_RETRY_COUNT) {
> > + drm_err(display->drm, "Histogram bin read
> failed with max retry\n");
> > + return false;
> > + }
> > + /* Add a delay before retrying */
>
> The comment says the same thing as the code.
>
> > + fsleep(HISTOGRAM_BIN_READ_DELAY);
> > + index = 0;
> > + intel_de_rmw(display, DPST_CTL(intel_crtc->pipe),
> > + DPST_CTL_BIN_REG_FUNC_SEL |
> > + DPST_CTL_BIN_REG_MASK, 0);
>
> This is duplicated; see the retry loop placement.
>
> > + }
> > + }
> > + return true;
> > +}
> > +
> > +static void intel_histogram_handle_int_work(struct work_struct *work)
> > +{
> > + struct intel_histogram *histogram = container_of(work,
> > + struct intel_histogram, work.work);
> > + struct intel_crtc *intel_crtc = histogram->crtc;
> > + struct intel_display *display = to_intel_display(intel_crtc);
> > + char *histogram_event[] = {"HISTOGRAM=1", NULL};
>
> Not sure it's great that this is a global uevent instead of having more detailed
> information. Maybe it should have the CRTC details?
>
Done!
> > +
> > + /*
> > + * 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(display, DPST_CTL(intel_crtc->pipe),
> > + DPST_CTL_BIN_REG_FUNC_SEL |
> DPST_CTL_BIN_REG_MASK, 0);
> > + if (intel_histogram_get_data(intel_crtc)) {
> > + /* Notify user for Histogram rediness */
> > + if (kobject_uevent_env(&display->drm->primary->kdev->kobj,
> > + KOBJ_CHANGE, histogram_event))
> > + drm_err(display->drm,
> > + "sending HISTOGRAM event failed\n");
> > + }
>
> So you probably want to add the retry loop here. This avoids duplicating the
> DPST_CTL rmw.
>
Done!
But moved this retry change to 8th patch in this series where changes are LNL are done.
Thanks and Regards,
Arun R Murthy
--------------------
