> > Statistics is generated from the image frame that is coming to display
> > and an event is sent to user after reading this histogram data.
> > This statistics/histogram is then shared with the user upon getting a
> > request from user. User can then use this histogram and generate an
> > enhancement factor. This enhancement factor can be multiplied/added
> > with the incoming pixel data frame.
> >
> > v2: forward declaration in header file along with error handling
> > (Jani)
> > v3: Replaced i915 with intel_display (Suraj)
> > v4: Removed dithering enable/disable (Vandita)
> > New patch for histogram register definitions (Suraj)
> >
>
> Mostly looks okay to me some minor comments and questions below
>
> > Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/Makefile | 1 +
> > .../drm/i915/display/intel_display_types.h | 2 +
> > .../gpu/drm/i915/display/intel_histogram.c | 195 ++++++++++++++++++
> > .../gpu/drm/i915/display/intel_histogram.h | 35 ++++
> > 4 files changed, 233 insertions(+)
> > create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.c
> > create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.h
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile
> > b/drivers/gpu/drm/i915/Makefile index e465828d748f..97141b4def3b
> > 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -267,6 +267,7 @@ i915-y += \
> > display/intel_hdcp.o \
> > display/intel_hdcp_gsc.o \
> > display/intel_hdcp_gsc_message.o \
> > + display/intel_histogram.o \
> > display/intel_hotplug.o \
> > display/intel_hotplug_irq.o \
> > display/intel_hti.o \
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 339e4b0f7698..351441efd10a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1414,6 +1414,8 @@ struct intel_crtc {
> > /* for loading single buffered registers during vblank */
> > struct pm_qos_request vblank_pm_qos;
> >
> > + struct intel_histogram *histogram;
> > +
> > #ifdef CONFIG_DEBUG_FS
> > struct intel_pipe_crc pipe_crc;
> > #endif
> > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c
> > b/drivers/gpu/drm/i915/display/intel_histogram.c
> > new file mode 100644
> > index 000000000000..e751977fc6f7
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> > @@ -0,0 +1,195 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2024 Intel Corporation */
> > +
> > +#include <drm/drm_device.h>
> > +#include <drm/drm_file.h>
> > +#include <drm/drm_vblank.h>
> > +
> > +#include "i915_reg.h"
> > +#include "i915_drv.h"
> > +#include "intel_display.h"
> > +#include "intel_histogram_regs.h"
> > +#include "intel_histogram.h"
> > +#include "intel_display_types.h"
> > +#include "intel_de.h"
>
> Rearrange the above definitions in alphabetical order so intel_de goes above
> intel display and Display types comes under intel_display
>
Done
> > +
> > +#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.
>
> Shouldn’t we be using /* xxxxx */ to make comments
>
Done
> > +#define HISTOGRAM_DEFAULT_GUARDBAND_DELAY 0x04
> > +
> > +struct intel_histogram {
> > + struct intel_crtc *crtc;
> > + struct delayed_work work;
> > + bool enable;
> > + bool can_enable;
> > + u32 bin_data[HISTOGRAM_BIN_COUNT];
> > +};
> > +
> > +int intel_histogram_atomic_check(struct intel_crtc *intel_crtc) {
> > + struct intel_histogram *histogram = intel_crtc->histogram;
> > +
> > + /* TODO: Restrictions for enabling histogram */
>
> Should we at least add the restriction for the minimum display version required
> to run DPST?
>
We are not considering DPST over here. Its only the global histogram.
Remaining will be taken up in the upcoming patches.
> > + histogram->can_enable = true;
> > +
> > + return 0;
> > +}
> > +
> > +static int intel_histogram_enable(struct intel_crtc *intel_crtc) {
> > + struct intel_display *display = to_intel_display(intel_crtc);
> > + struct intel_histogram *histogram = intel_crtc->histogram;
> > + int pipe = intel_crtc->pipe;
> > + u64 res;
> > + u32 gbandthreshold;
> > +
> > + if (!histogram)
> > + return -EINVAL;
> > +
> > + if (!histogram->can_enable)
> > + return -EINVAL;
> > +
> > + if (histogram->enable)
> > + return 0;
> > +
> > + /* enable histogram, clear DPST_BIN reg and select TC function */
>
> I think you mean "Clear DPST_CTL bin reg func select to TC"
> Because that’s what I see on Bspec and also I don’t see you clearing DPST_BIN
> register
>
Done
> > + intel_de_rmw(display, DPST_CTL(pipe),
> > + DPST_CTL_BIN_REG_FUNC_SEL | DPST_CTL_IE_HIST_EN |
> > + DPST_CTL_HIST_MODE |
> > DPST_CTL_IE_TABLE_VALUE_FORMAT,
> > + DPST_CTL_BIN_REG_FUNC_TC | DPST_CTL_IE_HIST_EN |
> > + DPST_CTL_HIST_MODE_HSV |
> > + DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC);
> > +
> > + /* Re-Visit: check if wait for one vblank is required */
> > + drm_crtc_wait_one_vblank(&intel_crtc->base);
>
> Confused here so histogram will not be enabled for at least 2 vblanks and 3
> vblanks for display 12-14 after writing enable When enabled outside the active
> region so why just the single vblank wait ?
>
Histogram is the statistics of 'n' number of frames. Generation of histogram event would take 2-3 vblanks as said. But here we are waiting for a vblank so as to enable histogram then program the guardband values.
> > +
> > + /*
> > + * TODO: one time programming: Program GuardBand Threshold.
> > + * To be moved to modeset path
> > + */
>
> The two : back to back seem dirty after the todo state in a single sentence what
> needs to be done
>
Done
> > + res = (intel_crtc->config->hw.adjusted_mode.vtotal *
> > + intel_crtc->config->hw.adjusted_mode.htotal);
> > +
> > + gbandthreshold = (res *
> > HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT) /
> > + HISTOGRAM_GUARDBAND_PRECISION_FACTOR;
>
> According to bspec this value i.e gbandthreshold "This value is shifted left 2 bits
> (multiplied by 4) for use with the 24 bit bin values."
> I don’t see that happening here is that on purpose?
> Also I don’t see any spec stating how you calculate this threshold can you point
> me to where this is written.
>
Guardband threshold for 24bit bin data is chosen to be 12. But the value used in calculation is 3% and hw does x4 as per the register description.
The calculation is from the HLD.
> > +
> > + /* Enable histogram interrupt mode */
> > + intel_de_rmw(display, DPST_GUARD(pipe),
> > + DPST_GUARD_THRESHOLD_GB_MASK |
> > + DPST_GUARD_INTERRUPT_DELAY_MASK |
> > DPST_GUARD_HIST_INT_EN,
> > + DPST_GUARD_THRESHOLD_GB(gbandthreshold) |
> > +
> > DPST_GUARD_INTERRUPT_DELAY(HISTOGRAM_DEFAULT_GUARDBAND_DELA
> > Y) |
>
> Where do we get the HISTOGRAM_DEFAULT_GUARDBAND_DELAY from ?
>
We define it.
> > + DPST_GUARD_HIST_INT_EN);
> > +
> > + /* Clear pending interrupts has to be done on separate write */
> > + intel_de_rmw(display, DPST_GUARD(pipe),
> > + DPST_GUARD_HIST_EVENT_STATUS, 1);
> > +
> > + histogram->enable = true;
> > +
> > + return 0;
> > +}
> > +
> > +static void intel_histogram_disable(struct intel_crtc *intel_crtc) {
> > + struct intel_display *display = to_intel_display(intel_crtc);
> > + struct intel_histogram *histogram = intel_crtc->histogram;
> > + int pipe = intel_crtc->pipe;
> > +
> > + if (!histogram)
> > + return;
> > +
> > + /* If already disabled return */
> > + if (histogram->enable)
> > + return;
> > +
> > + /* Clear pending interrupts and disable interrupts */
> > + intel_de_rmw(display, DPST_GUARD(pipe),
> > + DPST_GUARD_HIST_INT_EN |
> > DPST_GUARD_HIST_EVENT_STATUS, 0);
> > +
> > + /* disable DPST_CTL Histogram mode */
> > + intel_de_rmw(display, DPST_CTL(pipe),
> > + DPST_CTL_IE_HIST_EN, 0);
> > +
> > + histogram->enable = false;
> > +}
> > +
> > +int intel_histogram_update(struct intel_crtc *intel_crtc, bool
> > +enable) {
> > + if (enable)
> > + return intel_histogram_enable(intel_crtc);
> > +
> > + intel_histogram_disable(intel_crtc);
> > + return 0;
> > +}
> > +
> > +int intel_histogram_set_iet_lut(struct intel_crtc *intel_crtc, u32
> > +*data) {
> > + struct intel_histogram *histogram = intel_crtc->histogram;
> > + struct intel_display *display = to_intel_display(intel_crtc);
> > + int pipe = intel_crtc->pipe;
> > + int i = 0;
> > +
> > + if (!histogram)
> > + return -EINVAL;
> > +
> > + if (!histogram->enable) {
> > + drm_err(display->drm, "histogram not enabled");
> > + return -EINVAL;
> > + }
> > +
> > + if (!data) {
> > + drm_err(display->drm, "enhancement LUT data is NULL");
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * Set DPST_CTL Bin Reg function select to IE
> > + * Set DPST_CTL Bin Register Index to 0
> > + */
> > + intel_de_rmw(display, DPST_CTL(pipe),
> > + DPST_CTL_BIN_REG_FUNC_SEL |
> > DPST_CTL_BIN_REG_MASK,
> > + DPST_CTL_BIN_REG_FUNC_IE |
> > DPST_CTL_BIN_REG_CLEAR);
> > +
> > + for (i = 0; i < HISTOGRAM_IET_LENGTH; i++) {
> > + intel_de_rmw(display, DPST_BIN(pipe),
> > + DPST_BIN_DATA_MASK, data[i]);
> > + drm_dbg_atomic(display->drm, "iet_lut[%d]=%x\n", i,
> > data[i]);
> > + }
> > +
> > + intel_de_rmw(display, DPST_CTL(pipe),
> > + DPST_CTL_ENHANCEMENT_MODE_MASK |
> > DPST_CTL_IE_MODI_TABLE_EN,
> > + DPST_CTL_EN_MULTIPLICATIVE |
> > DPST_CTL_IE_MODI_TABLE_EN);
>
> According to Bspec we follow the below steps "
> 1)Set DPST_CTL Bin Register Function Select to IE 2)Wait for vertical blank for
> switch to IE mode, can skip if step 1 was done more than 1 vblank previously
> 3)Set DPST_CTL Bin Register Index to 0 4)Write enhancement factor to
> DPST_BIN Data 4)Go to step 4 until all 33 entries are written "
> The sequence in code seems reversed.
> Also I don’t see the vblank wait that need to be done after switching to IE
> mode.
>
>
Done
> > +
> > + /* Once IE is applied, change DPST CTL to TC */
> > + intel_de_rmw(display, DPST_CTL(pipe),
> > + DPST_CTL_BIN_REG_FUNC_SEL,
> > DPST_CTL_BIN_REG_FUNC_TC);
> > +
> > + return 0;
> > +}
> > +
> > +void intel_histogram_finish(struct intel_crtc *intel_crtc) {
> > + struct intel_histogram *histogram = intel_crtc->histogram;
> > +
> > + kfree(histogram);
> > +}
> > +
> > +int intel_histogram_init(struct intel_crtc *intel_crtc) {
> > + struct intel_histogram *histogram;
> > +
> > + /* Allocate histogram internal struct */
> > + histogram = kzalloc(sizeof(*histogram), GFP_KERNEL);
> > + if (!histogram) {
> > + return -ENOMEM;
> > + }
>
> No need for extra brackets
>
Done
Thanks and Regards,
Arun R Murthy
--------------------
