On Wed, Dec 13, 2023 at 02:36:40PM +0530, Nemesa Garg wrote:
> Darkscreen detection checks if all the pixels of the frame are less then
> or equal to the comparision value. The comparision value is set to 256
> i.e black. So upon getting black pixels from the pipe, the dark screen
> detect bit is set and an error message will be printed.
As others have noted, you seem to be mixing two different designs here
in a way that doesn't really make sense. At the hardware level, it's
possible to request a check for a dark screen[*] and then get a yes/no
response back shortly. But how that functionality gets utilized and
exposed to users is something that can be approached in different ways.
I can think of a couple options for how this could be handled:
Option 1: The simplest option is to create a debugfs that tells me
whether the screen is dark *right now*. I.e., "cat ..../darkscreen"
returns an immediate "yes" or "no" answer. Every time I re-cat it, I
get a new answer; there's no concept of enable/disable at the API
level. I can see this being very useful in some IGT tests like
testdisplay (make sure the output isn't blank after switching to a
mode) or kms_plane (make sure the output _is_ blank at certain points
when all planes are supposed to be off).
Option 2: A more complicated option is to create an interface with
start/stop behavior that indicates whether the screen ever went dark
between two points in time. I'm sure there are ways this could be
used, although it's a little harder to figure out where we'd utilize
it to our benefit in the IGT tests we have today.
The code you have in these patches seems to be trying to expose an
enable/disable debugfs interface, but only does its checking a single
time right at the 'enable' point. There's no reason to actually leave
the hardware setting enabled after that point because you never go back
and check again. And the current implementation also doesn't return the
status to the user in any way, it just prints out a log message, so that
pretty much kills the ability to use this in something like IGT.
So as others have said, you need to figure out what the goal is here.
I.e., who do you expect to use this and how? Based on the answer to
that, you can figure out what kind of user-facing interface is
appropriate, and then provide corresponding changes to some userspace
software (probably IGT for a debug-specific facility like this) that
utilizes the new capability.
[*] BTW, it's also worth noting that no matter what this interface can't
detect all cases where the display is blank. The hardware support for
this appears to be at the end of the pipe, so if the monitor is blank
due to mistakes in the programming of the DSC, transcoder, DDI, or phy,
the darkscreen detection won't notice. However it will be useful for
catching certain mistakes in the programming of planes, scalers, or
color management.
>
> v2: Addressed review comments [Jani]
> v3: Addressed review comments [Arun]
Changelogs like this are pretty useless; without digging through mailing
list history, nobody (often including the original reviewers) remembers
exactly what the review comments were. It's hard to tell which parts of
your patch have been reworked from the previous revision, or to
determine whether you saw and handled all of the feedback or just part
of it. It's better to give a brief description of what actually changed
in the new version. E.g.,
v2:
- Added foo check. (Jani)
- Reordered bar and baz. (Jani)
v3:
- Added locking to xyz. (Arun)
- Drop unnecessary abc check. (Arun)
Matt
>
> Signed-off-by: Nemesa Garg <nemesa.garg@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> .../gpu/drm/i915/display/intel_darkscreen.c | 95 +++++++++++++++++++
> .../gpu/drm/i915/display/intel_darkscreen.h | 26 +++++
> .../drm/i915/display/intel_display_types.h | 2 +
> drivers/gpu/drm/i915/i915_reg.h | 8 ++
> drivers/gpu/drm/xe/Makefile | 1 +
> 6 files changed, 133 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 dc2469a4ead7..0322105a4c05 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -244,6 +244,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..7c42988af354
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_darkscreen.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + *
> + */
> +
> +#include "i915_reg.h"
> +#include "intel_de.h"
> +#include "intel_display_types.h"
> +
> +#define COLOR_DEPTH_6BPC 6
> +#define COLOR_DEPTH_8BPC 8
> +#define COLOR_DEPTH_10BPC 10
> +#define COLOR_DEPTH_12BPC 12
> +
> +#define COMPARE_VALUE_6_BPC 4
> +#define COMPARE_VALUE_8_BPC 16
> +#define COMPARE_VALUE_10_BPC 64
> +#define COMPARE_VALUE_12_BPC 256
> +
> +#define COMPARE_VALUE_CALCULATION_FACTOR 12
> +
> +/*
> + * Check the color format and compute the compare value based on bpc.
> + */
> +void intel_dark_screen_enable(struct intel_crtc_state *crtc_state)
> +{
> + 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);
> + unsigned int frame_time_in_us;
> + u32 comparevalue;
> +
> + if (!crtc->dark_screen.enable)
> + return;
> +
> + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB) {
> + drm_err(&dev_priv->drm,
> + "RGB format not supported:%c\n",
> + pipe_name(crtc->pipe));
> + return;
> + }
> +
> + switch (crtc_state->pipe_bpp / 3) {
> + case COLOR_DEPTH_6BPC:
> + comparevalue = COMPARE_VALUE_6_BPC;
> + break;
> + case COLOR_DEPTH_8BPC:
> + comparevalue = COMPARE_VALUE_8_BPC;
> + break;
> + case COLOR_DEPTH_10BPC:
> + comparevalue = COMPARE_VALUE_10_BPC;
> + break;
> + case COLOR_DEPTH_12BPC:
> + comparevalue = COMPARE_VALUE_12_BPC;
> + break;
> + default:
> + drm_dbg(&dev_priv->drm,
> + "Bpc value is incorrect:%d\n",
> + crtc_state->pipe_bpp / 3);
> + return;
> + }
> +
> + comparevalue = comparevalue <<
> + (COMPARE_VALUE_CALCULATION_FACTOR - crtc_state->pipe_bpp / 3);
> +
> + intel_de_write(dev_priv, DARK_SCREEN(cpu_transcoder),
> + DARK_SCREEN_ENABLE | DARK_SCREEN_DETECT |
> + DARK_SCREEN_DONE | DARK_SCREEN_COMPARE_VAL(comparevalue));
> +
> + frame_time_in_us = (1000 / drm_mode_vrefresh(&crtc_state->hw.adjusted_mode)) * 2;
> +
> + intel_de_wait_for_set(dev_priv,
> + DARK_SCREEN(crtc->config->cpu_transcoder), DARK_SCREEN_DONE,
> + frame_time_in_us);
> +
> + 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), 1, DARK_SCREEN_DETECT |
> + DARK_SCREEN_DONE);
> +}
> +
> +void intel_dark_screen_disable(struct intel_crtc_state *crtc_state)
> +{
> + 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);
> +
> + intel_de_write(dev_priv, DARK_SCREEN(cpu_transcoder), 0);
> +}
> 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..366e43499fc4
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_darkscreen.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + *
> + */
> +
> +#ifndef __INTEL_DARKSCREEN_H__
> +#define __INTEL_DARKSCREEN_H__
> +
> +#include <linux/types.h>
> +
> +struct intel_crtc_state;
> +struct intel_crtc;
> +
> +struct intel_darkscreen {
> + bool enable;
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +void intel_dark_screen_enable(struct intel_crtc_state *crtc_state);
> +void intel_dark_screen_disable(struct intel_crtc_state *crtc_state);
> +void intel_darkscreen_crtc_debugfs_add(struct intel_crtc *crtc);
> +
> +#endif
> +
> +#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 3600497ff512..d441e08fef58 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -49,6 +49,7 @@
> #include "i915_vma.h"
> #include "i915_vma_types.h"
> #include "intel_bios.h"
> +#include "intel_darkscreen.h"
> #include "intel_display.h"
> #include "intel_display_limits.h"
> #include "intel_display_power.h"
> @@ -1539,6 +1540,7 @@ struct intel_crtc {
>
> #ifdef CONFIG_DEBUG_FS
> struct intel_pipe_crc pipe_crc;
> + struct intel_darkscreen dark_screen;
> #endif
> };
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c203dcfd6c7b..4e273c0ac801 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2097,6 +2097,14 @@
> #define TRANS_PUSH_EN REG_BIT(31)
> #define TRANS_PUSH_SEND REG_BIT(30)
>
> +#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)
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index fe659a97001c..ff533733245c 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -176,6 +176,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
> i915-display/intel_crtc_state_dump.o \
> i915-display/intel_cursor.o \
> i915-display/intel_cx0_phy.o \
> + i915-display/intel_darkscreen.o \
> i915-display/intel_ddi.o \
> i915-display/intel_ddi_buf_trans.o \
> i915-display/intel_display.o \
> --
> 2.25.1
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
