[PATCHv2 1/5] drm/i915/display: Add support for histogram
Kandpal, Suraj
suraj.kandpal at intel.com
Wed Sep 11 09:45:45 UTC 2024
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of
> Kandpal, Suraj
> Sent: Wednesday, September 11, 2024 11:07 AM
> To: Murthy, Arun R <arun.r.murthy at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy at intel.com>
> Subject: RE: [PATCHv2 1/5] drm/i915/display: Add support for histogram
>
>
>
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of
> > Arun R Murthy
> > Sent: Wednesday, August 21, 2024 3:54 PM
> > To: intel-gfx at lists.freedesktop.org
> > Cc: Murthy, Arun R <arun.r.murthy at intel.com>
> > Subject: [PATCHv2 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.
> >
> > v2: forward declaration in header file along with error handling
> > (Jani)
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy at intel.com>
> > ---
> > drivers/gpu/drm/i915/Makefile | 1 +
> > .../drm/i915/display/intel_display_types.h | 2 +
> > .../gpu/drm/i915/display/intel_histogram.c | 205 ++++++++++++++++++
> > .../gpu/drm/i915/display/intel_histogram.h | 78 +++++++
> > drivers/gpu/drm/xe/Makefile | 1 +
> > 5 files changed, 287 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 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 bd290536a1b7..79d34d6d537d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1537,6 +1537,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..45e968e00af6
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> > @@ -0,0 +1,205 @@
> > +// 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.h"
> > +#include "intel_histogram.h"
> > +#include "intel_display_types.h"
> > +#include "intel_de.h"
> > +
> > +#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
> > +
> > +struct intel_histogram {
> > + struct drm_i915_private *i915;
>
> Lets use intel_display here instead of i915 as I can see this is mostly being
> used for reg read/writes Read/write/rmw also work with intel_display well.
>
On another look I believe intel_display also is not required here I see intel_crtc is available
Almost everywhere hence you can derive it with to_intel_display(crtc)
And derive drm_i915_private wherever we cant avoid it with to_i915(display->drm)
Regards,
Suraj Kandpal
> > + bool enable;
> > + bool can_enable;
> > + enum pipe pipe;
> > + u32 bindata[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 */
> > + histogram->can_enable = true;
> > +
> > + return 0;
> > +}
> > +
> > +static void intel_histogram_enable_dithering(struct drm_i915_private
> > *dev_priv,
>
> Use intel_display here
> > + enum pipe pipe)
> > +{
> > + intel_de_rmw(dev_priv, PIPE_MISC(pipe),
> PIPE_MISC_DITHER_ENABLE,
> > + PIPE_MISC_DITHER_ENABLE);
>
> So every where below drm_i915_private can be replaced with intel_display
> Ditto.
>
> > +}
> > +
> > +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)
> > + return -EINVAL;
> > +
> > + if (!histogram->can_enable) {
> > + return -EINVAL;
> > + }
>
> No need for brackets here now atleast
>
> > +
> > + 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
> > + */
>
> Nit: maybe make it Enable DPST... and If we are mentioning the steps lets
> add some number or points(-)
>
> > + 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_DELA
> Y)
> > |
> > + 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;
> > +
> > + if (!histogram)
> > + return;
> > +
> > + /* 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)
> > + return -EINVAL;
> > +
> > + 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 (!histogram) {
> > + 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..b25091732274
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_histogram.h
> > @@ -0,0 +1,78 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2024 Intel Corporation */
> > +
> > +#ifndef __INTEL_HISTOGRAM_H__
> > +#define __INTEL_HISTOGRAM_H__
> > +
> > +#include <linux/types.h>
> > +
> > +struct intel_crtc;
> > +
> > +/* 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)
>
> I don't think this is the right way. Lets have a file for intel_histogram_regs.h
> and intel_histogram.h That will look a lot cleaner.
>
> > +
> > +#define HISTOGRAM_BIN_COUNT 32
> > +#define HISTOGRAM_IET_LENGTH 33
> > +
> > +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, 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 b9670ae09a9e..424ea43016dd 100644
> > --- a/drivers/gpu/drm/xe/Makefile
> > +++ b/drivers/gpu/drm/xe/Makefile
> > @@ -238,6 +238,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 \
>
> Lets try to separate xe and i915 changes into different patches as well as we
> can I know its tough to decouple some Changes but the ones that can be
> done should be.
>
> Regards,
> Suraj Kandpal
>
> > --
> > 2.25.1
More information about the Intel-gfx
mailing list