diff mbox series

[1/2] drm/i915: Add Darkscreen registers and timer handler.

Message ID 20231025102734.2783492-2-nemesa.garg@intel.com (mailing list archive)
State New, archived
Headers show
Series Enable Darkscreen Feature | expand

Commit Message

Garg, Nemesa Oct. 25, 2023, 10:27 a.m. UTC 
Add new registers for Darkscreen
The logic checks for any black screen at pipe level and upon such detection prints error.
Darkscreen compares the pixels with the compare value(0x00 - black) for the detection purpose.
This feature can be enable/disable through debugfs.

Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
---
 drivers/gpu/drm/i915/Makefile                 |  1 +
 .../gpu/drm/i915/display/intel_darkscreen.c   | 69 +++++++++++++++++++
 .../gpu/drm/i915/display/intel_darkscreen.h   | 40 +++++++++++
 .../drm/i915/display/intel_display_types.h    |  3 +
 drivers/gpu/drm/i915/i915_reg.h               |  9 +++
 5 files changed, 122 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/display/intel_darkscreen.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_darkscreen.h

Comments

Jani Nikula Oct. 25, 2023, 11:18 a.m. UTC | #1 
On Wed, 25 Oct 2023, Nemesa Garg <nemesa.garg@intel.com> wrote:
> Add new registers for Darkscreen
> The logic checks for any black screen at pipe level and upon such detection prints error.
> Darkscreen compares the pixels with the compare value(0x00 - black) for the detection purpose.
> This feature can be enable/disable through debugfs.

The most important question a commit message should answer is *why*.

Why do we need this, what is it for?

Please find a number of nitpicks below.

First, the commit subject does not describe the patch. Registers and
timer handling? Huh?

The commit message should be wrapped. This is all explained in
Documentation/process/submitting-patches.rst; please read it.

>
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile                 |  1 +
>  .../gpu/drm/i915/display/intel_darkscreen.c   | 69 +++++++++++++++++++
>  .../gpu/drm/i915/display/intel_darkscreen.h   | 40 +++++++++++
>  .../drm/i915/display/intel_display_types.h    |  3 +
>  drivers/gpu/drm/i915/i915_reg.h               |  9 +++
>  5 files changed, 122 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/display/intel_darkscreen.c
>  create mode 100644 drivers/gpu/drm/i915/display/intel_darkscreen.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 88b2bb005014..538d5a42f9e3 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -254,6 +254,7 @@ i915-y += \
>  	display/intel_crtc.o \
>  	display/intel_crtc_state_dump.o \
>  	display/intel_cursor.o \
> +	display/intel_darkscreen.o \
>  	display/intel_display.o \
>  	display/intel_display_driver.o \
>  	display/intel_display_irq.o \
> diff --git a/drivers/gpu/drm/i915/display/intel_darkscreen.c b/drivers/gpu/drm/i915/display/intel_darkscreen.c
> new file mode 100644
> index 000000000000..ef68dbc7a296
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_darkscreen.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2018 Intel Corporation

It's 2023 now.

> + *
> + * Author: Nemesa Garg <nemesa.garg@intel.com>

Please don't add Author: lines in source code.

The git history says who has done what, and is always up-to-date. Adding
authors in source presents a problem: when do you amend it? Some people
update them with a typo fix, some people almost never, even when they
rewrite the entire file. Then it's just stale information.

I, for example, have modified almost every file in the entire i915
driver. Should I add my name to all of them? What would be the point?

Finally, I think author lines give a false sense of ownership in a
fundamentally shared project.

Yes, we have existing author lines, but that's just because of another
problem with author lines: they're actually incredibly hard to remove.

> + */

Blank line here.

> +#include "i915_reg.h"
> +#include "intel_display_types.h"
> +#include "intel_de.h"

Please sort includes.

> +
> +/*
> + * Dark screen detection check if all pixels
> + * in a frame are less than or equal to compare
> + * value.Check the color format and compute the
> + * compare value based on bpc.

Rewrap, space after '.'.

> + */
> +void dark_screen_enable(struct intel_crtc_state *crtc_state)

Non-static functions should have prefix that matches the file name.

E.g. intel_foo.[ch] -> intel_foo_bar().

> +{
> +	u32 comparevalue;

Usually the longer declarations go first.

> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> +	if (!crtc->dark_screen.enable)
> +		return;
> +
> +	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
> +		return;

Blank line here.

> +	drm_err(&dev_priv->drm,
> +		"RGB format is not present%c\n",

Is this a useful log line:

RGB format is not presentB

Is it worth an *error* in the dmesg?

> +		pipe_name(crtc->pipe));

The whole thing fits on two  lines.

> +
> +	switch (crtc_state->pipe_bpp / 3) {
> +	case DD_COLOR_DEPTH_6BPC:
> +		comparevalue = DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_6_BPC;
> +		break;
> +	case DD_COLOR_DEPTH_8BPC:
> +		comparevalue = DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_8_BPC;
> +		break;
> +	case DD_COLOR_DEPTH_10BPC:
> +		comparevalue = DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_10_BPC;
> +		break;
> +	case DD_COLOR_DEPTH_12BPC:
> +		comparevalue = DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_12_BPC;
> +		break;
> +	default:

You'll get a warning about uninitialized comparevalue on this code path.

> +		break;
> +	}
> +
> +	comparevalue = comparevalue <<
> +		(DARKSCREEN_PROGRAMMED_COMPARE_VALUE_CALCULATION_FACTOR - crtc->dark_screen.bpc);

That macro name is silly long.

> +
> +	intel_de_write(dev_priv, DARK_SCREEN(cpu_transcoder),
> +		       DARK_SCREEN_ENABLE | DARK_SCREEN_DETECT |
> +		       DARK_SCREEN_DONE | DARK_SCREEN_COMPARE_VAL(comparevalue));
> +
> +	intel_de_wait_for_set(dev_priv,
> +			      DARK_SCREEN(crtc->config->cpu_transcoder), DARK_SCREEN_DONE, 1);
> +
> +	if (intel_de_read(dev_priv, DARK_SCREEN(crtc->config->cpu_transcoder)) &
> +			  DARK_SCREEN_DETECT) {
> +		drm_err(&dev_priv->drm,
> +			"Dark Screen detected %c\n",
> +			pipe_name(crtc->pipe));

Error for detected? What is this?

> +	}
> +
> +	intel_de_rmw(dev_priv, DARK_SCREEN(crtc->config->cpu_transcoder), 0, DARK_SCREEN_DETECT |
> +		       DARK_SCREEN_DONE);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_darkscreen.h b/drivers/gpu/drm/i915/display/intel_darkscreen.h
> new file mode 100644
> index 000000000000..69da1ea56829
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_darkscreen.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Author: Nemesa Garg <nemesa.garg@intel.com>

Same things about copyright year and author lines.

> + */
> +
> +#ifndef __INTEL_DARKSCREEN_H__
> +#define __INTEL_DARKSCREEN_H__
> +
> +#include <drm/drm_device.h>

You don't need that. Use minimal includes in headers.

> +
> +#define DD_COLOR_DEPTH_6BPC 6
> +#define DD_COLOR_DEPTH_8BPC 8
> +#define DD_COLOR_DEPTH_10BPC 10
> +#define DD_COLOR_DEPTH_12BPC 12
> +
> +// HW Darkscreen Detection Macros

Please use /* */ comments.

> +#define DARKSCREEN_PROGRAMMED_COMPARE_VALUE_CALCULATION_FACTOR 12
> +
> +// Compare Value = 16*(2 ^ (bpc-8))

Ditto.

> +#define DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_6_BPC 4
> +#define DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_8_BPC 16
> +#define DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_10_BPC 64
> +#define DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_12_BPC 256

All of the above could be in intel_darkscreen.c.

> +
> +struct intel_crtc_state;
> +struct intel_crtc;
> +
> +struct intel_darkscreen {
> +	bool enable;
> +	u64 timer_value;
> +	u8 bpc;
> +	struct hrtimer timer;
> +};
> +
> +void dark_screen_enable(struct intel_crtc_state *crtc_state);
> +void intel_darkscreen_crtc_debugfs_add(struct intel_crtc *crtc);
> +
> +#endif /* __INTEL_DARKSCREEN_H_ */

The comment does not match the #ifdef.

> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 65ea37fe8cff..289ecda2e032 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -54,6 +54,7 @@
>  #include "intel_display_power.h"
>  #include "intel_dpll_mgr.h"
>  #include "intel_wm_types.h"
> +#include "intel_darkscreen.h"

Please keep includes sorted.

>  
>  struct drm_printer;
>  struct __intel_global_objs_state;
> @@ -1515,6 +1516,8 @@ struct intel_crtc {
>  	/* for loading single buffered registers during vblank */
>  	struct pm_qos_request vblank_pm_qos;
>  
> +	struct intel_darkscreen dark_screen;
> +

If it's debugfs only (is it?) you could add this below the #ifdef.

>  #ifdef CONFIG_DEBUG_FS
>  	struct intel_pipe_crc pipe_crc;
>  #endif
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 135e8d8dbdf0..01d9de14f79c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2097,6 +2097,15 @@
>  #define   TRANS_PUSH_EN			REG_BIT(31)
>  #define   TRANS_PUSH_SEND		REG_BIT(30)
>  
> +/* Dark SCREEN */

It's pretty obvious from the macro names. The comment adds no value.

> +#define _DARK_SCREEN_PIPE_A             0x60120
> +#define DARK_SCREEN(trans)              _MMIO_TRANS2(trans, _DARK_SCREEN_PIPE_A)
> +#define   DARK_SCREEN_ENABLE            REG_BIT(31)
> +#define   DARK_SCREEN_DETECT            REG_BIT(29)
> +#define   DARK_SCREEN_DONE		REG_BIT(28)
> +#define DARK_SCREEN_COMPARE_MASK        REG_GENMASK(9, 0)
> +#define DARK_SCREEN_COMPARE_VAL(x)	REG_FIELD_PREP(DARK_SCREEN_COMPARE_MASK, (x))

Please use tabs for indenting the values.

> +
>  /* VGA port control */
>  #define ADPA			_MMIO(0x61100)
>  #define PCH_ADPA                _MMIO(0xe1100)
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 88b2bb005014..538d5a42f9e3 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -254,6 +254,7 @@  i915-y += \
 	display/intel_crtc.o \
 	display/intel_crtc_state_dump.o \
 	display/intel_cursor.o \
+	display/intel_darkscreen.o \
 	display/intel_display.o \
 	display/intel_display_driver.o \
 	display/intel_display_irq.o \
diff --git a/drivers/gpu/drm/i915/display/intel_darkscreen.c b/drivers/gpu/drm/i915/display/intel_darkscreen.c
new file mode 100644
index 000000000000..ef68dbc7a296
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_darkscreen.c
@@ -0,0 +1,69 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2018 Intel Corporation
+ *
+ * Author: Nemesa Garg <nemesa.garg@intel.com>
+ */
+#include "i915_reg.h"
+#include "intel_display_types.h"
+#include "intel_de.h"
+
+/*
+ * Dark screen detection check if all pixels
+ * in a frame are less than or equal to compare
+ * value.Check the color format and compute the
+ * compare value based on bpc.
+ */
+void dark_screen_enable(struct intel_crtc_state *crtc_state)
+{
+	u32 comparevalue;
+	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+
+	if (!crtc->dark_screen.enable)
+		return;
+
+	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB)
+		return;
+	drm_err(&dev_priv->drm,
+		"RGB format is not present%c\n",
+		pipe_name(crtc->pipe));
+
+	switch (crtc_state->pipe_bpp / 3) {
+	case DD_COLOR_DEPTH_6BPC:
+		comparevalue = DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_6_BPC;
+		break;
+	case DD_COLOR_DEPTH_8BPC:
+		comparevalue = DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_8_BPC;
+		break;
+	case DD_COLOR_DEPTH_10BPC:
+		comparevalue = DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_10_BPC;
+		break;
+	case DD_COLOR_DEPTH_12BPC:
+		comparevalue = DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_12_BPC;
+		break;
+	default:
+		break;
+	}
+
+	comparevalue = comparevalue <<
+		(DARKSCREEN_PROGRAMMED_COMPARE_VALUE_CALCULATION_FACTOR - crtc->dark_screen.bpc);
+
+	intel_de_write(dev_priv, DARK_SCREEN(cpu_transcoder),
+		       DARK_SCREEN_ENABLE | DARK_SCREEN_DETECT |
+		       DARK_SCREEN_DONE | DARK_SCREEN_COMPARE_VAL(comparevalue));
+
+	intel_de_wait_for_set(dev_priv,
+			      DARK_SCREEN(crtc->config->cpu_transcoder), DARK_SCREEN_DONE, 1);
+
+	if (intel_de_read(dev_priv, DARK_SCREEN(crtc->config->cpu_transcoder)) &
+			  DARK_SCREEN_DETECT) {
+		drm_err(&dev_priv->drm,
+			"Dark Screen detected %c\n",
+			pipe_name(crtc->pipe));
+	}
+
+	intel_de_rmw(dev_priv, DARK_SCREEN(crtc->config->cpu_transcoder), 0, DARK_SCREEN_DETECT |
+		       DARK_SCREEN_DONE);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_darkscreen.h b/drivers/gpu/drm/i915/display/intel_darkscreen.h
new file mode 100644
index 000000000000..69da1ea56829
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_darkscreen.h
@@ -0,0 +1,40 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2018 Intel Corporation
+ *
+ * Author: Nemesa Garg <nemesa.garg@intel.com>
+ */
+
+#ifndef __INTEL_DARKSCREEN_H__
+#define __INTEL_DARKSCREEN_H__
+
+#include <drm/drm_device.h>
+
+#define DD_COLOR_DEPTH_6BPC 6
+#define DD_COLOR_DEPTH_8BPC 8
+#define DD_COLOR_DEPTH_10BPC 10
+#define DD_COLOR_DEPTH_12BPC 12
+
+// HW Darkscreen Detection Macros
+#define DARKSCREEN_PROGRAMMED_COMPARE_VALUE_CALCULATION_FACTOR 12
+
+// Compare Value = 16*(2 ^ (bpc-8))
+#define DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_6_BPC 4
+#define DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_8_BPC 16
+#define DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_10_BPC 64
+#define DARKSCREEN_COMPARE_VALUE_LIMITED_RANGE_12_BPC 256
+
+struct intel_crtc_state;
+struct intel_crtc;
+
+struct intel_darkscreen {
+	bool enable;
+	u64 timer_value;
+	u8 bpc;
+	struct hrtimer timer;
+};
+
+void dark_screen_enable(struct intel_crtc_state *crtc_state);
+void intel_darkscreen_crtc_debugfs_add(struct intel_crtc *crtc);
+
+#endif /* __INTEL_DARKSCREEN_H_ */
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 65ea37fe8cff..289ecda2e032 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -54,6 +54,7 @@ 
 #include "intel_display_power.h"
 #include "intel_dpll_mgr.h"
 #include "intel_wm_types.h"
+#include "intel_darkscreen.h"
 
 struct drm_printer;
 struct __intel_global_objs_state;
@@ -1515,6 +1516,8 @@  struct intel_crtc {
 	/* for loading single buffered registers during vblank */
 	struct pm_qos_request vblank_pm_qos;
 
+	struct intel_darkscreen dark_screen;
+
 #ifdef CONFIG_DEBUG_FS
 	struct intel_pipe_crc pipe_crc;
 #endif
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 135e8d8dbdf0..01d9de14f79c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2097,6 +2097,15 @@ 
 #define   TRANS_PUSH_EN			REG_BIT(31)
 #define   TRANS_PUSH_SEND		REG_BIT(30)
 
+/* Dark SCREEN */
+#define _DARK_SCREEN_PIPE_A             0x60120
+#define DARK_SCREEN(trans)              _MMIO_TRANS2(trans, _DARK_SCREEN_PIPE_A)
+#define   DARK_SCREEN_ENABLE            REG_BIT(31)
+#define   DARK_SCREEN_DETECT            REG_BIT(29)
+#define   DARK_SCREEN_DONE		REG_BIT(28)
+#define DARK_SCREEN_COMPARE_MASK        REG_GENMASK(9, 0)
+#define DARK_SCREEN_COMPARE_VAL(x)	REG_FIELD_PREP(DARK_SCREEN_COMPARE_MASK, (x))
+
 /* VGA port control */
 #define ADPA			_MMIO(0x61100)
 #define PCH_ADPA                _MMIO(0xe1100)