> -----Original Message-----
> From: Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>
> Sent: Thursday, August 22, 2024 4:24 PM
> To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH 1/5] drm/i915/display: Add support for histogram
>
> > -----Original Message-----
> > From: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> > Sent: Thursday, August 22, 2024 12:07 PM
> > To: Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>; intel-
> > gfx@xxxxxxxxxxxxxxxxxxxxx
> > Subject: RE: [PATCH 1/5] drm/i915/display: Add support for histogram
> >
> >
> > > -----Original Message-----
> > > From: Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>
> > > Sent: Monday, August 5, 2024 12:16 PM
> > > To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>;
> > > intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> > > Subject: RE: [PATCH 1/5] drm/i915/display: Add support for histogram
> > >
> > > > -----Original Message-----
> > > > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On
> > > > Behalf Of Arun R Murthy
> > > > Sent: Friday, July 5, 2024 3:26 PM
> > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > Cc: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> > > > Subject: [PATCH 1/5] drm/i915/display: 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.
> > > > 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.
> > > >
> > > > Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/Makefile | 1 +
> > > > .../drm/i915/display/intel_display_types.h | 3 +
> > > > .../gpu/drm/i915/display/intel_histogram.c | 187
> > ++++++++++++++++++
> > > > .../gpu/drm/i915/display/intel_histogram.h | 91 +++++++++
> > > > drivers/gpu/drm/xe/Makefile | 1 +
> > > > 5 files changed, 283 insertions(+) create mode 100644
> > > > drivers/gpu/drm/i915/display/intel_histogram.c
> > > > create mode 100644 drivers/gpu/drm/i915/display/intel_histogram.h
> > > >
> > > I see that this patch is doing 3 things, histogram enable, setting
> > > lut and also enable dithering.
> > > See if they can be split into sub patches.
> > >
> > > Also on a generic note, there are a lot of TODO comments, are they
> > > waiting on something or you have newer Version of these patches.
> > > Otherwise it will be better to make a note in the comments what will
> > > be the corner cases that would be hit with these TODO comments not
> > fixed.
> >
> > TODO has been added in places where we don’t need them for now but
> > required. As of now we are enabling just the basic histogram, going
> > forward we will need to have this TODO implemented as the driver
> > enhances. So just to keep an eye and we don’t loose track on this TODO
> > has been added in the code itself.
> >
> > >
> > > > diff --git a/drivers/gpu/drm/i915/Makefile
> > > > b/drivers/gpu/drm/i915/Makefile index c63fa2133ccb..03caf3a24966
> > > > 100644
> > > > --- a/drivers/gpu/drm/i915/Makefile
> > > > +++ b/drivers/gpu/drm/i915/Makefile
> > > > @@ -264,6 +264,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 8713835e2307..e0a9c6d8c9b2 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -1537,6 +1537,9 @@ struct intel_crtc {
> > > > /* for loading single buffered registers during vblank */
> > > > struct pm_qos_request vblank_pm_qos;
> > > >
> > > > + /* histogram data */
> > > > + 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..1a603445fc29
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> > > > @@ -0,0 +1,187 @@
> > > > +// SPDX-License-Identifier: MIT
> > > > +/*
> > > > + * Copyright © 2024 Intel Corporation */
> > > > +
> > > > +#include <drm/drm_device.h>
> > > > +#include <drm/drm_file.h>
> > > > +#include "i915_reg.h"
> > > > +#include "i915_drv.h"
> > > > +#include "intel_display_types.h"
> > > > +#include "intel_de.h"
> > > > +#include "intel_histogram.h"
> > > > +
> > > > +int intel_histogram_can_enable(struct intel_crtc *intel_crtc) {
> > > > + struct intel_histogram *histogram = intel_crtc->histogram;
> > > > +
> > > > + /* TODO: Restrictions for enabling histogram */
> > > More info on what kind of restrictions we are talking about here,
> > > will be helpful.
> > >
> > These restrictions can be more confined to the algorithm that we use.
> > We have the basic GHE algorithm now and going forward some
> > restrictions will come.
>
> I see that from the IGT patch that has been testing this series, the tests are
> skipped on non eDP panels.
> https://patchwork.freedesktop.org/patch/602381/?series=135789&rev=1
> (picked from cover letter) if (connector->connector_type !=
> DRM_MODE_CONNECTOR_eDP)
> + continue;
> Is this a miss from the IGT , if not
> is the behavior known, on the how part if non eDP panels are tested with this
> feature? Or is this something that we would want to restrict from the driver,
> considering the gains from this feature?
> I am not sure if I missed but I do not see this part documented or addressed in
> the patches.
>
Histogram is as such generic and can be used on any connector.
Thanks and Regards,
Arun R Murthy
--------------------
> Thanks,
> Vandita
>
> >
> > > > + histogram->can_enable = true;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void intel_histogram_enable_dithering(struct
> > > > +drm_i915_private
> > > > *dev_priv,
> > > > + enum pipe pipe)
> > > > +{
> > > > + intel_de_rmw(dev_priv, PIPE_MISC(pipe),
> > > > PIPE_MISC_DITHER_ENABLE,
> > > > + PIPE_MISC_DITHER_ENABLE);
> > > > +}
> > > > +
> > > > +static int intel_histogram_enable(struct intel_crtc *intel_crtc) {
> > > > + struct drm_i915_private *i915 = to_i915(intel_crtc->base.dev);
> > > > + struct intel_histogram *histogram = intel_crtc->histogram;
> > > > + int pipe = intel_crtc->pipe;
> > > > + u64 res;
> > > > + u32 gbandthreshold;
> > > > +
> > > > + if (!histogram->can_enable) {
> > > > + drm_err(&i915->drm,
> > > > + "Histogram not supported, compute config
> > > > failed\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (histogram->enable)
> > > > + return 0;
> > > > +
> > > > + /* Pipe Dithering should be enabled with GLOBAL_HIST */
> > > > + intel_histogram_enable_dithering(i915, pipe);
> > > > +
> > > > + /*
> > > > + * enable DPST_CTL Histogram mode
> > > > + * Clear DPST_CTL Bin Reg function select to TC
> > > > + */
> > > > + intel_de_rmw(i915, 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);
> > > > +
> > > > + /* TODO: one time programming: Program GuardBand Threshold */
> > > > + 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(i915, 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_DE
> > L
> > > > AY) |
> > > > + DPST_GUARD_HIST_INT_EN);
> > > > +
> > > > + /* Clear pending interrupts has to be done on separate write */
> > > > + intel_de_rmw(i915, 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 drm_i915_private *i915 = to_i915(intel_crtc->base.dev);
> > > > + struct intel_histogram *histogram = intel_crtc->histogram;
> > > > + int pipe = intel_crtc->pipe;
> > > > +
> > > > + /* Pipe Dithering should be enabled with GLOBAL_HIST */
> > > > + intel_histogram_enable_dithering(i915, pipe);
> > > > +
> > > > + /* Clear pending interrupts and disable interrupts */
> > > > + intel_de_rmw(i915, DPST_GUARD(pipe),
> > > > + DPST_GUARD_HIST_INT_EN |
> > > > DPST_GUARD_HIST_EVENT_STATUS, 0);
> > > > +
> > > > + /* disable DPST_CTL Histogram mode */
> > > > + intel_de_rmw(i915, 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 drm_i915_private *i915 = to_i915(intel_crtc->base.dev);
> > > > + int pipe = intel_crtc->pipe;
> > > > + int i = 0;
> > > > +
> > > > + if (!histogram->enable) {
> > > > + drm_err(&i915->drm, "histogram not enabled");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (!data) {
> > > > + drm_err(&i915->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(i915, 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(i915, DPST_BIN(pipe),
> > > > + DPST_BIN_DATA_MASK, data[i]);
> > > > + drm_dbg_atomic(&i915->drm, "iet_lut[%d]=%x\n", i,
> > data[i]);
> > > > + }
> > > > +
> > > > + intel_de_rmw(i915, DPST_CTL(pipe),
> > > > + DPST_CTL_ENHANCEMENT_MODE_MASK |
> > > > DPST_CTL_IE_MODI_TABLE_EN,
> > > > + DPST_CTL_EN_MULTIPLICATIVE |
> > > > DPST_CTL_IE_MODI_TABLE_EN);
> > > > +
> > > > + /* Once IE is applied, change DPST CTL to TC */
> > > > + intel_de_rmw(i915, DPST_CTL(pipe),
> > > > + DPST_CTL_BIN_REG_FUNC_SEL,
> > > > DPST_CTL_BIN_REG_FUNC_TC);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +void intel_histogram_deinit(struct intel_crtc *intel_crtc) {
> > > > + struct intel_histogram *histogram = intel_crtc->histogram;
> > > > +
> > > > + kfree(histogram);
> > > > +}
> > > > +
> > > > +int intel_histogram_init(struct intel_crtc *intel_crtc) {
> > > > + struct drm_i915_private *i915 = to_i915(intel_crtc->base.dev);
> > > > + struct intel_histogram *histogram;
> > > > +
> > > > + /* Allocate histogram internal struct */
> > > > + histogram = kzalloc(sizeof(*histogram), GFP_KERNEL);
> > > > + if (unlikely(!histogram)) {
> > > > + drm_err(&i915->drm,
> > > > + "Failed to allocate HISTOGRAM event\n");
> > > > + kfree(histogram);
> > > This Kfree is not needed, as it is already inside a NULL check.
> > Done!
> >
> > >
> > > > + return -ENOMEM;
> > > > + }
> > > > +
> > > > + intel_crtc->histogram = histogram;
> > > > + histogram->pipe = intel_crtc->pipe;
> > > > + histogram->can_enable = false;
> > > > +
> > > > + 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
> > > > new file mode 100644
> > > > index 000000000000..632116c52882
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/i915/display/intel_histogram.h
> > > > @@ -0,0 +1,91 @@
> > > > +// SPDX-License-Identifier: MIT
> > > > +/*
> > > > + * Copyright © 2024 Intel Corporation */
> > > > +
> > > > +#ifndef __INTEL_HISTOGRAM_H__
> > > > +#define __INTEL_HISTOGRAM_H__
> > > > +
> > > > +#include <drm/drm_device.h>
> > > > +#include <drm/drm_file.h>
> > > > +#include "intel_display.h"
> > > > +#include "i915_reg.h"
> > > > +
> > > > +/* GLOBAL_HIST related registers */
> > > > +#define _DPST_CTL_A 0x490C0
> > > > +#define _DPST_CTL_B 0x491C0
> > > > +#define DPST_CTL(pipe)
> > > > _MMIO_PIPE(pipe, _DPST_CTL_A, _DPST_CTL_B)
> > > > +#define DPST_CTL_IE_HIST_EN REG_BIT(31)
> > > > +#define DPST_CTL_RESTORE REG_BIT(28)
> > > > +#define DPST_CTL_IE_MODI_TABLE_EN REG_BIT(27)
> > > > +#define DPST_CTL_HIST_MODE REG_BIT(24)
> > > > +#define DPST_CTL_ENHANCEMENT_MODE_MASK
> > > > REG_GENMASK(14, 13)
> > > > +#define DPST_CTL_EN_MULTIPLICATIVE
> > > > REG_FIELD_PREP(DPST_CTL_ENHANCEMENT_MODE_MASK, 2)
> > > > +#define DPST_CTL_IE_TABLE_VALUE_FORMAT
> > > > REG_BIT(15)
> > > > +#define DPST_CTL_BIN_REG_FUNC_SEL REG_BIT(11)
> > > > +#define DPST_CTL_BIN_REG_FUNC_TC
> > > > REG_FIELD_PREP(DPST_CTL_BIN_REG_FUNC_SEL, 0)
> > > > +#define DPST_CTL_BIN_REG_FUNC_IE
> > > > REG_FIELD_PREP(DPST_CTL_BIN_REG_FUNC_SEL, 1)
> > > > +#define DPST_CTL_BIN_REG_MASK
> > > > REG_GENMASK(6, 0)
> > > > +#define DPST_CTL_BIN_REG_CLEAR
> > > > REG_FIELD_PREP(DPST_CTL_BIN_REG_MASK, 0)
> > > > +#define DPST_CTL_IE_TABLE_VALUE_FORMAT_2INT_8FRAC
> > > > REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 1)
> > > > +#define DPST_CTL_IE_TABLE_VALUE_FORMAT_1INT_9FRAC
> > > > REG_FIELD_PREP(DPST_CTL_IE_TABLE_VALUE_FORMAT, 0)
> > > > +#define DPST_CTL_HIST_MODE_YUV
> > > > REG_FIELD_PREP(DPST_CTL_HIST_MODE, 0)
> > > > +#define DPST_CTL_HIST_MODE_HSV
> > > > REG_FIELD_PREP(DPST_CTL_HIST_MODE, 1)
> > > > +
> > > > +#define _DPST_GUARD_A
> > 0x490C8
> > > > +#define _DPST_GUARD_B 0x491C8
> > > > +#define DPST_GUARD(pipe)
> > _MMIO_PIPE(pipe,
> > > > _DPST_GUARD_A, _DPST_GUARD_B)
> > > > +#define DPST_GUARD_HIST_INT_EN
> > REG_BIT(31)
> > > > +#define DPST_GUARD_HIST_EVENT_STATUS
> > REG_BIT(30)
> > > > +#define DPST_GUARD_INTERRUPT_DELAY_MASK
> > > > REG_GENMASK(29, 22)
> > > > +#define DPST_GUARD_INTERRUPT_DELAY(val)
> > > > REG_FIELD_PREP(DPST_GUARD_INTERRUPT_DELAY_MASK, val)
> > > > +#define DPST_GUARD_THRESHOLD_GB_MASK
> > > > REG_GENMASK(21, 0)
> > > > +#define DPST_GUARD_THRESHOLD_GB(val)
> > > > REG_FIELD_PREP(DPST_GUARD_THRESHOLD_GB_MASK, val)
> > > > +
> > > > +#define _DPST_BIN_A 0x490C4
> > > > +#define _DPST_BIN_B 0x491C4
> > > > +#define DPST_BIN(pipe)
> > > > _MMIO_PIPE(pipe, _DPST_BIN_A, _DPST_BIN_B)
> > > > +#define DPST_BIN_DATA_MASK
> > > > REG_GENMASK(23, 0)
> > > > +#define DPST_BIN_BUSY REG_BIT(31)
> > > > +
> > > > +#define INTEL_HISTOGRAM_PIPEA 0x90000000
> > > > +#define INTEL_HISTOGRAM_PIPEB 0x90000002
> > > > +#define INTEL_HISTOGRAM_EVENT(pipe) PIPE(pipe, \
> > > > + INTEL_HISTOGRAM_PIPEA,
> > > > \
> > > > + INTEL_HISTOGRAM_PIPEB)
> > > > +
> > > > +#define HISTOGRAM_BIN_COUNT 32
> > > > +#define HISTOGRAM_IET_LENGTH 33
> > > > +
> > > > +#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
> > > > +
> > > > +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,
> > > > +};
> > > > +
> > > Description on this new structure and its new members with comments
> > > will be better.
> > This is internal to only this file and not exposed and the names are
> > self explanatory. Please let me know if the headers are still required
> > will add them in the next series.
> >
> > >
> > > > +struct intel_histogram {
> > > > + struct drm_i915_private *i915;
> > > > + bool enable;
> > > > + bool can_enable;
> > > > + enum pipe pipe;
> > > > + u32 bindata[HISTOGRAM_BIN_COUNT]; };
> > > > +
> > > > +int intel_histogram_can_enable(struct intel_crtc *intel_crtc);
> > > > +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); void intel_histogram_deinit(struct
> > > > +intel_crtc *intel_crtc);
> > > > +
> > > > +#endif /* __INTEL_HISTOGRAM_H__ */
> > > > diff --git a/drivers/gpu/drm/xe/Makefile
> > > > b/drivers/gpu/drm/xe/Makefile index 628c245c4822..1881e3e1f4a9
> > > > 100644
> > > > --- a/drivers/gpu/drm/xe/Makefile
> > > > +++ b/drivers/gpu/drm/xe/Makefile
> > > > @@ -260,6 +260,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
> > > > i915-display/intel_hdcp.o \
> > > > i915-display/intel_hdcp_gsc_message.o \
> > > > i915-display/intel_hdmi.o \
> > > > + i915-display/intel_histogram.o \
> > > > i915-display/intel_hotplug.o \
> > > > i915-display/intel_hotplug_irq.o \
> > > > i915-display/intel_hti.o \
> > > > --
> > > > 2.25.1
> >
> > Thanks and Regards,
> > Arun R Murthy
> > -------------------
