Hi Daniel,
> On Thu, 21 Nov 2024 at 12:35, Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> wrote:
> > The corresponding mutter changes to enable/disable histogram, read the
> > histogram data, communicate with the library and write the enhanced
> > data back to the KMD is also pushed for review at
> > https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873
>
> Again, the Mutter MR which is linked does _not_ write anything back to the
> KMS driver. It would be much more clear to explain in your cover letters that it
> currently reads the histogram values and does nothing with them, and that an
> expected next step is to make it write the processed values back to the KMS
> driver. Even though I'm sure your intention is not to mislead anyone, what you
> are writing here is not actually true.
>
I am sorry for that.
I totally agree that without writing back the enhanced data back to the KMD, the feature is incomplete. Will update the mutter changes so as to include the KMD write part in a day or two.
Thanks and Regards,
Arun R Murthy
--------------------
--- Begin Message ---
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Arun R
> Murthy
> Sent: Thursday, November 21, 2024 5:56 PM
> To: intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> Subject: [PATCHv5 2/8] 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.
> 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)
> 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)
>
I think some comments were missed need to address them but nothing major
> Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> .../drm/i915/display/intel_display_types.h | 2 +
> .../gpu/drm/i915/display/intel_histogram.c | 190 ++++++++++++++++++
> .../gpu/drm/i915/display/intel_histogram.h | 35 ++++
> 4 files changed, 228 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 e465828d748f..97141b4def3b 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -267,6 +267,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 339e4b0f7698..351441efd10a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1414,6 +1414,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..b2dda88a8093
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_histogram.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 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
> +
> +struct intel_histogram {
> + struct intel_crtc *crtc;
> + struct delayed_work work;
> + bool enable;
> + bool can_enable;
> + u32 bin_data[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 int intel_histogram_enable(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;
> + u64 res;
> + u32 gbandthreshold;
> +
> + if (!histogram)
> + return -EINVAL;
> +
> + if (!histogram->can_enable)
> + return -EINVAL;
Nit: the above two conditions can be combined with &&
> +
> + 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,
> + DPST_CTL_BIN_REG_FUNC_TC | 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: To be moved to modeset
> path */
I wanted this to be something like this
/* 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_DELA
> Y) |
> + 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, 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 intel_display *display = to_intel_display(intel_crtc);
> + int pipe = intel_crtc->pipe;
> + int i = 0;
> +
> + if (!histogram)
> + return -EINVAL;
> +
> + if (!histogram->enable) {
> + drm_err(display->drm, "histogram not enabled");
> + return -EINVAL;
> + }
> +
> + if (!data) {
> + drm_err(display->drm, "enhancement LUT data is NULL");
> + return -EINVAL;
> + }
> +
> + /* Set DPST_CTL Bin Reg function select to IE & wait for a vblabk */
> + intel_de_rmw(display, DPST_CTL(pipe),
> + DPST_CTL_BIN_REG_FUNC_SEL,
> DPST_CTL_BIN_REG_FUNC_IE);
> +
> + drm_crtc_wait_one_vblank(&intel_crtc->base);
> +
> + /* Set DPST_CTL Bin Register Index to 0 */
> + intel_de_rmw(display, DPST_CTL(pipe),
> + DPST_CTL_BIN_REG_MASK, DPST_CTL_BIN_REG_CLEAR);
> +
> + for (i = 0; i < HISTOGRAM_IET_LENGTH; i++) {
> + intel_de_rmw(display, DPST_BIN(pipe),
> + DPST_BIN_DATA_MASK, data[i]);
> + drm_dbg_atomic(display->drm, "iet_lut[%d]=%x\n", i,
> data[i]);
> + }
> +
> + 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 *intel_crtc) {
> + struct intel_histogram *histogram;
> +
> + /* Allocate histogram internal struct */
> + histogram = kzalloc(sizeof(*histogram), GFP_KERNEL);
> + if (!histogram) {
> + return -ENOMEM;
> + }
You don’t need the {} brackets here for if
With the above fixed LGTM
Reviewed-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> +
> + intel_crtc->histogram = histogram;
> + histogram->crtc = intel_crtc;
> + histogram->can_enable = false;
> +
> + 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..9d66724f9c53
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_histogram.h
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef __INTEL_HISTOGRAM_H__
> +#define __INTEL_HISTOGRAM_H__
> +
> +#include <linux/types.h>
> +
> +struct intel_crtc;
> +
> +#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_finish(struct intel_crtc *intel_crtc);
> +
> +#endif /* __INTEL_HISTOGRAM_H__ */
> --
> 2.25.1
--- End Message ---
