diff mbox series

[1/7] drm/i915/histogram: Define registers for histogram

Message ID 20240925150754.1876893-2-arun.r.murthy@intel.com (mailing list archive)
State New, archived
Headers show
Series Display Global Histogram | expand

Commit Message

Arun R Murthy Sept. 25, 2024, 3:07 p.m. UTC 
Add the register/bit definitions for global histogram.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 .../drm/i915/display/intel_histogram_reg.h    | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/display/intel_histogram_reg.h

Comments

Jani Nikula Sept. 26, 2024, 10:13 a.m. UTC | #1 
On Wed, 25 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> Add the register/bit definitions for global histogram.
>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  .../drm/i915/display/intel_histogram_reg.h    | 54 +++++++++++++++++++

We have 36 files named *_regs.h under display/, and 0 files named
*_reg.h. We should follow the pattern.

>  1 file changed, 54 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_histogram_reg.h
>
> diff --git a/drivers/gpu/drm/i915/display/intel_histogram_reg.h b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> new file mode 100644
> index 000000000000..ed8f22aa8e75
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef __INTEL_HISTOGRAM_REG_H__
> +#define __INTEL_HISTOGRAM_REG_H__
> +
> +#include <linux/types.h>

I don't see this used.

But it's probably prudent to #include "intel_display_reg_defs.h" for
_MMIO_PIPE() etc. like almost all the other _regs.h files do.

> +
> +/* 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)

We've tried to establish a uniform style for defining register macros
since 2017. Just so they're easier for everyone to read. It's documented
in i915_reg.h. Please indent the register *content* macros so they are
easier to distinguish from the actual register.

Ditto below.

> +
> +#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)

This can't be right, but it's unused so wasn't caught.

BR,
Jani.


> +
> +#endif /* __INTEL_HISTOGRAM_REG_H__ */
Arun R Murthy Oct. 23, 2024, 5:55 a.m. UTC | #2 
> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Thursday, September 26, 2024 3:44 PM
> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-xe@lists.freedesktop.org;
> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: Re: [PATCH 1/7] drm/i915/histogram: Define registers for histogram
> 
> On Wed, 25 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> > Add the register/bit definitions for global histogram.
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  .../drm/i915/display/intel_histogram_reg.h    | 54 +++++++++++++++++++
> 
> We have 36 files named *_regs.h under display/, and 0 files named *_reg.h. We
> should follow the pattern.
> 
> >  1 file changed, 54 insertions(+)
> >  create mode 100644 drivers/gpu/drm/i915/display/intel_histogram_reg.h
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> > b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> > new file mode 100644
> > index 000000000000..ed8f22aa8e75
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
> > @@ -0,0 +1,54 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2024 Intel Corporation  */
> > +
> > +#ifndef __INTEL_HISTOGRAM_REG_H__
> > +#define __INTEL_HISTOGRAM_REG_H__
> > +
> > +#include <linux/types.h>
> 
> I don't see this used.
> 
> But it's probably prudent to #include "intel_display_reg_defs.h" for
> _MMIO_PIPE() etc. like almost all the other _regs.h files do.
> 
> > +
> > +/* 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)
> 
> We've tried to establish a uniform style for defining register macros since 2017.
> Just so they're easier for everyone to read. It's documented in i915_reg.h.
> Please indent the register *content* macros so they are easier to distinguish
> from the actual register.
> 
> Ditto below.
> 
> > +
> > +#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)
> 
> This can't be right, but it's unused so wasn't caught.
> 
> BR,
> Jani.
> 
> 
> > +
> > +#endif /* __INTEL_HISTOGRAM_REG_H__ */
> 
Addressed the above comments, if there any other comment on the series or can I float the new version?

Thanks and Regards,
Arun R Murthy
--------------------
> --
> Jani Nikula, Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_histogram_reg.h b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
new file mode 100644
index 000000000000..ed8f22aa8e75
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_histogram_reg.h
@@ -0,0 +1,54 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#ifndef __INTEL_HISTOGRAM_REG_H__
+#define __INTEL_HISTOGRAM_REG_H__
+
+#include <linux/types.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)
+
+#endif /* __INTEL_HISTOGRAM_REG_H__ */