On Fri, 05 Jul 2024, Arun R Murthy <arun.r.murthy@xxxxxxxxx> wrote:
> 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
>
> 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 */
Does the comment add value?
> + 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>
Blank line here.
> +#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 */
> + histogram->can_enable = true;
A function named _can_enable() sounds like a predicate function that
does not have side effects.
> +
> + 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) {
intel_crtc->histogram may be NULL. Ditto everywhere.
> + drm_err(&i915->drm,
> + "Histogram not supported, compute config failed\n");
> + return -EINVAL;
Seems iffy to log that as an error.
> + }
> +
> + 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_DELAY) |
> + 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;
What do you need this for?
Compute config should be used to decide what to do.
Old and new state should be used to check whether it's already enabled.
> +
> + 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;
Ditto.
> +}
> +
> +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)) {
Please never use likely/unlikely unless you can show it has a meaningful
perf impact.
> + drm_err(&i915->drm,
> + "Failed to allocate HISTOGRAM event\n");
Please never log allocation failures.
> + kfree(histogram);
Allocation of histogram just failed...?
> + 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"
Please use forward declarations instead of includes in headers whenever
possible. You don't need *any* of those includes.
> +#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
Register macros belong in their own files. We never shove them in the
regular headers like this.
> +
> +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,
> +};
> +
> +struct intel_histogram {
> + struct drm_i915_private *i915;
> + bool enable;
> + bool can_enable;
> + enum pipe pipe;
> + u32 bindata[HISTOGRAM_BIN_COUNT];
> +};
These should probably be internal to intel_histogram.c.
> +
> +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 \
--
Jani Nikula, Intel
