> -----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.
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
> -------------------
