[PATCHv2 1/5] drm/i915/display: Add support for histogram

[Kulkarni, Vandita]

> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy at intel.com>
> Sent: Thursday, September 12, 2024 2:39 PM
> To: Kulkarni, Vandita <vandita.kulkarni at intel.com>; intel-
> gfx at lists.freedesktop.org
> 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;
> > > +	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
> --------------------