> -----Original Message----- > From: Murthy, Arun R <arun.r.murthy@xxxxxxxxx> > Sent: Thursday, September 12, 2024 2:39 PM > To: Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: RE: [PATCHv2 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: Wednesday, August 21, 2024 3:54 PM > > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > Cc: Murthy, Arun R <arun.r.murthy@xxxxxxxxx> > > > 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@xxxxxxxxx> > > > --- > > > 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; > > > + 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; > > > +} > > > + > > Looks like we are totally bypassing crtc_state->dither. > > Also I see some comments on dither not being enabled on anything which > > is not 6bpc. Is that constraint resolved now? > > > > Crtc_state->dither is used for enabling dithering during the crtc_enable and > at this point we are far ahead of that. > That restriction is for older platforms(ironlake) and we don't have any such > for ADLP+ My first point was why do you need to enable it again separately, if it has been already taken care by crtc_state->dither And second point was, can you please share the bsepc link where we have this requirement of enabling it again, even it it was enabled. Thanks Vandita > > > > +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) > > > + return -EINVAL; > > > + > > > + if (!histogram->can_enable) { > > > + return -EINVAL; > > > + } > > > + > > > + if (histogram->enable) > > > + return 0; > > > + > > I don't see in the spec that dither should be enabled, any quick bspec > > references? > Dithering should be enabled by default for ADL+ for DPST. This is an > enhancement to avoid artifacts. > > Thanks and Regards, > Arun R Murthy > --------------------