[Intel-gfx] [PATCH 1/2] drm/i915: Add Darkscreen registers and timer handler.
Jani Nikula
jani.nikula at linux.intel.com
Wed Oct 25 11:18:26 UTC 2023
On Wed, 25 Oct 2023, Nemesa Garg <nemesa.garg at 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 at 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 at 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 at 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)
--
Jani Nikula, Intel
More information about the Intel-gfx
mailing list