> -----Original Message-----
> From: Kandpal, Suraj
> Sent: Friday, February 14, 2025 3:54 PM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; Murthy, Arun R
> <arun.r.murthy@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: dmitry.baryshkov@xxxxxxxxxx
> Subject: RE: [PATCH v8 06/14] drm/i915/histogram: Add support for histogram
>
>
>
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> > Kandpal, Suraj
> > Sent: Friday, February 14, 2025 3:32 PM
> > To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>;
> > intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: dmitry.baryshkov@xxxxxxxxxx
> > Subject: RE: [PATCH v8 06/14] drm/i915/histogram: Add support for
> > histogram
> >
> >
> >
> > > -----Original Message-----
> > > From: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> > > Sent: Tuesday, January 28, 2025 9:21 PM
> > > To: intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > > dri- devel@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>;
> > > dmitry.baryshkov@xxxxxxxxxx; Murthy, Arun R
> > > <arun.r.murthy@xxxxxxxxx>
> > > Subject: [PATCH v8 06/14] drm/i915/histogram: Add support for
> > > histogram
> > >
> > > Statistics is generated from the image frame that is coming to
> > > display and an event is sent to user after reading this histogram data.
> > >
> > > 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)
> > > v5: IET LUT pgm follow the seq in spec and removed change to TC at end
> > > (Suraj)
> > > v8: Retained only the Histogram part and move IET LUT to a different
> > > patch.
> > >
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
>
> LGTM,
> Reviewed-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
I meant to send this to another patch in this series but sent it here this patch already has some comments of mine that need to be addressed
Don't consider this Rb
Regards,
Suraj Kandpal
>
> > > ---
> > > drivers/gpu/drm/i915/Makefile | 1 +
> > > drivers/gpu/drm/i915/display/intel_display_types.h | 2 +
> > > drivers/gpu/drm/i915/display/intel_histogram.c | 157
> > > +++++++++++++++++++++
> > > drivers/gpu/drm/i915/display/intel_histogram.h | 48 +++++++
> > > 4 files changed, 208 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/Makefile
> > > b/drivers/gpu/drm/i915/Makefile index
> > >
> >
> 4caa8e30bc98387d45212fbc7cea8b38687bd0d5..f993b19174ba79c0bcc349946
> > > 19937be7d2797ed 100644
> > > --- a/drivers/gpu/drm/i915/Makefile
> > > +++ b/drivers/gpu/drm/i915/Makefile
> > > @@ -270,6 +270,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
> > >
> >
> cb51b7936f9338caaf14b1c6f7bbcc4327da4ef1..761fefed9376439c0ee5d346e8
> 1
> > > 10a219ad0a586 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1434,6 +1434,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
> > >
> >
> 0000000000000000000000000000000000000000..26eae8f40d0bf642546d5835
> > > 46782e22d5cefa9c
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> > > @@ -0,0 +1,157 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright © 2025 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_de.h"
> > > +#include "intel_display.h"
> > > +#include "intel_display_types.h"
> > > +#include "intel_histogram.h"
> > > +#include "intel_histogram_regs.h"
> > > +
> > > +/* 3.0% of the pipe's current pixel count, hw does x4 */ #define
> > > +HISTOGRAM_GUARDBAND_THRESHOLD_DEFAULT 300
> > > +/* Precision factor for threshold guardband */ #define
> > > +HISTOGRAM_GUARDBAND_PRECISION_FACTOR 10000 #define
> > > +HISTOGRAM_DEFAULT_GUARDBAND_DELAY 0x04
> > > +
> > > +int intel_histogram_atomic_check(struct intel_crtc *intel_crtc) {
> > > + struct intel_histogram *histogram = intel_crtc->histogram;
> > > +
> > > + /* TODO: Restrictions for enabling histogram */
> > > + histogram->can_enable = true;
> >
> > We don’t change anything in the intel_crtc structure during compute or
> > atomic check phase We need to consider moving this to the crtc_state.
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int intel_histogram_enable(struct intel_crtc *intel_crtc, u8
> > > +mode) {
> > > + 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 || !histogram->can_enable)
> > > + return -EINVAL;
> > > +
> > > + if (histogram->enable)
> > > + return 0;
> > > +
> > > + /* enable histogram, clear DPST_CTL bin reg func select to TC */
> > > + 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_ENHANCEMENT_MODE_MASK |
> > > DPST_CTL_IE_MODI_TABLE_EN,
> > > + ((mode == DRM_MODE_HISTOGRAM_HSV_MAX_RGB) ?
> > > + DPST_CTL_BIN_REG_FUNC_TC : 0) | DPST_CTL_IE_HIST_EN
> > > |
> > > + DPST_CTL_HIST_MODE_HSV |
> > > + DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC |
> > > + DPST_CTL_EN_MULTIPLICATIVE |
> > > DPST_CTL_IE_MODI_TABLE_EN);
> > > +
> > > + /* Re-Visit: check if wait for one vblank is required */
> > > + drm_crtc_wait_one_vblank(&intel_crtc->base);
> > > +
> > > + /* TODO: Program GuardBand Threshold needs to be moved to
> > > modeset path */
> > > + 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;
> > > +
> > > + /* 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_DELAY
> )
> > > |
> > > + 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,
> > > + struct drm_histogram_config *config) {
> > > + struct intel_display *display = to_intel_display(intel_crtc);
> > > +
> > > + if (config->enable) {
> > > + if (config->hist_mode !=
> > > DRM_MODE_HISTOGRAM_HSV_MAX_RGB) {
> > > + drm_err(display->drm,
> > > + "Only max(RGB) mode is supported for
> > > histogram\n");
> > > + return -EINVAL;
> > > + }
> > > + return intel_histogram_enable(intel_crtc, config->hist_mode);
> > > + }
> > > +
> > > + intel_histogram_disable(intel_crtc);
> > > + 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 *crtc) {
> > > + struct intel_histogram *histogram;
> > > + struct drm_histogram_caps *histogram_caps;
> > > +
> > > + /* Allocate histogram internal struct */
> > > + histogram = kzalloc(sizeof(*histogram), GFP_KERNEL);
> > > + if (!histogram)
> > > + return -ENOMEM;
> >
> > New line here
> >
> > > + histogram_caps = kzalloc(sizeof(*histogram_caps), GFP_KERNEL);
> > > + if (!histogram_caps)
> > > + return -ENOMEM;
> > > +
> > > + histogram_caps->histogram_mode =
> > > DRM_MODE_HISTOGRAM_HSV_MAX_RGB;
> > > + histogram_caps->bins_count = HISTOGRAM_BIN_COUNT;
> > > +
> >
> > Remove extra line
> >
> > Regards,
> > Suraj Kandpal
> > > + crtc->histogram = histogram;
> > > + histogram->crtc = crtc;
> > > + histogram->can_enable = false;
> > > + histogram->caps = histogram_caps;
> > > +
> > > + return 0;
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.h
> > > b/drivers/gpu/drm/i915/display/intel_histogram.h
> > > new file mode 100644
> > > index
> > >
> >
> 0000000000000000000000000000000000000000..5ea19ef2d3ecadf1ac159a78
> > > 4f51278fdde593de
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/display/intel_histogram.h
> > > @@ -0,0 +1,48 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright © 2025 Intel Corporation */
> > > +
> > > +#ifndef __INTEL_HISTOGRAM_H__
> > > +#define __INTEL_HISTOGRAM_H__
> > > +
> > > +#include <linux/types.h>
> > > +#include <linux/workqueue.h>
> > > +
> > > +struct delayed_work;
> > > +struct drm_property_blob;
> > > +struct drm_histogram_config;
> > > +struct drm_histogram_caps;
> > > +struct intel_crtc;
> > > +
> > > +#define HISTOGRAM_BIN_COUNT 32
> > > +
> > > +struct intel_histogram {
> > > + struct drm_histogram_caps *caps;
> > > + struct intel_crtc *crtc;
> > > + struct delayed_work work;
> > > + bool enable;
> > > + bool can_enable;
> > > + u32 bin_data[HISTOGRAM_BIN_COUNT]; };
> > > +
> > > +enum intel_global_hist_status {
> > > + INTEL_HISTOGRAM_ENABLE,
> > > + INTEL_HISTOGRAM_DISABLE,
> > > +};
> > > +
> > > +enum intel_global_histogram {
> > > + INTEL_HISTOGRAM,
> > > +};
> > > +
> > > +enum intel_global_hist_lut {
> > > + INTEL_HISTOGRAM_PIXEL_FACTOR,
> > > +};
> > > +
> > > +int intel_histogram_atomic_check(struct intel_crtc *intel_crtc);
> > > +int intel_histogram_update(struct intel_crtc *intel_crtc,
> > > + struct drm_histogram_config *config); int
> > > +intel_histogram_init(struct intel_crtc *intel_crtc); void
> > > +intel_histogram_finish(struct intel_crtc *intel_crtc);
> > > +
> > > +#endif /* __INTEL_HISTOGRAM_H__ */
> > >
> > > --
> > > 2.25.1
