On Wed, 24 Apr 2024, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> wrote:
> Add functions to enable darkscreen detection and corresponding
> additions to Makefile to build them.
> The enable and detect functions will be used in case we encounter
> a FIFO underrun which will help to check if a darkscreen occurred.
Why? The commit message needs to answer the question why. This is the
main thing I've requested ever since the patches were first posted.
Well, it also needs to answer the question what, and you need to explain
what "dark screen detection" is, and what we're doing with it.
Like, why do I have to read the code to realize the *only* thing we're
doing with this information is to debug log a single line? Is that
useful information? Are there further plans?
The patches for dark screen detection have been circulating for quite
some time now, and I have yet to see a compelling argument what we
benefit from enabling this. What's the story?
> Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> Signed-off-by: Nemesa Garg <nemesa.garg@xxxxxxxxx>
The authorship and signed-off-bys and co-developed-by need to be sorted
out. The last time I saw these they were authored by Nemesa.
Some detailed comments inline.
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> .../gpu/drm/i915/display/intel_darkscreen.c | 139 ++++++++++++++++++
> .../gpu/drm/i915/display/intel_darkscreen.h | 25 ++++
> .../drm/i915/display/intel_display_types.h | 3 +
> drivers/gpu/drm/xe/Makefile | 1 +
> 5 files changed, 169 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 7cad944b825c..00e36169a74d 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -255,6 +255,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..3ac3e8e6c1e3
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_darkscreen.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
2024.
> +
> +#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
What's all of the above based on?
> +
> +static void intel_darkscreen_detect(struct intel_crtc *crtc);
Please just reorder the file to not need this.
> +
> +static u32 intel_darkscreen_get_comp_val(struct drm_i915_private *i915, int bpc)
> +{
> + u32 compare_value = 0;
No need to set to 0.
> +
> + switch (bpc) {
> + case COLOR_DEPTH_6BPC:
> + compare_value = COMPARE_VALUE_6_BPC;
> + break;
> + case COLOR_DEPTH_8BPC:
> + compare_value = COMPARE_VALUE_8_BPC;
> + break;
> + case COLOR_DEPTH_10BPC:
> + compare_value = COMPARE_VALUE_10_BPC;
> + break;
> + case COLOR_DEPTH_12BPC:
> + compare_value = COMPARE_VALUE_12_BPC;
> + break;
> + default:
> + drm_dbg(&i915->drm, "Bpc value is incorrect:%d\n", bpc);
drm_dbg_kms
> + return -EINVAL;
Returning -EINVAL from a u32 function with no error checks anywhere
is... interesting.
> + }
> +
> + compare_value = compare_value << (COMPARE_VALUE_CALCULATION_FACTOR - bpc);
> + return DARK_SCREEN_COMPARE_VAL(compare_value);
It would probably be better for this function to return an int with just
the value, and for the caller to use DARK_SCREEN_COMPARE_VAL() to shove
it to the register.
> +}
> +
> +static void intel_darkscreen_work_fn(struct work_struct *work)
> +{
> + struct intel_darkscreen *dark_screen =
> + container_of(work, typeof(*dark_screen), darkscreen_detect_work);
> +
> + if (!dark_screen->enable)
> + intel_darkscreen_enable(dark_screen->crtc);
> +
> + intel_darkscreen_detect(dark_screen->crtc);
> +}
> +
> +void intel_darkscreen_schedule_work(struct intel_crtc *crtc)
Why do the callers of this function need to know the "schedule work"
implementation detail?
> +{
> + struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> + struct intel_darkscreen *dark_screen = &crtc->dark_screen;
> +
> + dark_screen->crtc = crtc;
You should not have to do this every time.
> + queue_work(i915->unordered_wq, &dark_screen->darkscreen_detect_work);
> +}
> +
> +void intel_darkscreen_setup(struct intel_crtc *crtc)
You don't call this anywhere. The whole thing can't work.
> +{
> + struct intel_darkscreen *dark_screen;
> +
> + dark_screen = &crtc->dark_screen;
> + dark_screen = kzalloc(sizeof(*dark_screen), GFP_KERNEL);
These two lines are just completely wrong. Make up your mind what the
member should be.
If this function were called, the above would leak. There's no cleanup.
> + if (!dark_screen)
> + return;
> + dark_screen->enable = false;
Unnecessary. But you should set dark_screen->crtc here.
> +
> + INIT_WORK(&dark_screen->darkscreen_detect_work, intel_darkscreen_work_fn);
Now, *if* this function were actually called, this would initialize the
work in an allocated struct that's thrown away and never used.
Also, please get the naming uniform. Is it darkscreen or dark_screen?
Make it one or the other, and stick with it throughout.
> +}
> +
> +/*
> + * Check the color format and compute the compare value based on bpc.
> + */
> +int intel_darkscreen_enable(struct intel_crtc *crtc)
> +{
> + struct intel_crtc_state *crtc_state = crtc->config;
> + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> + int bpc = crtc_state->pipe_bpp / 3;
> + u32 val;
> +
> + if (!crtc->dark_screen.enable)
> + return 0;
> +
> + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB) {
> + drm_dbg_kms(&dev_priv->drm,
> + "YUV format not supported:%c for darkscreen detection\n",
> + pipe_name(crtc->pipe));
> + return -EPROTO;
Just -EINVAL. But then you're not checking the return values anywhere...
> + }
> +
> + val = intel_darkscreen_get_comp_val(dev_priv, bpc);
> + val |= DARK_SCREEN_ENABLE;
> + intel_de_write(dev_priv, DARK_SCREEN(cpu_transcoder), val);
> + crtc->dark_screen.enable = true;
> +
> + return 0;
> +}
> +
> +static void intel_darkscreen_detect(struct intel_crtc *crtc)
> +{
> + struct intel_crtc_state *crtc_state = crtc->config;
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> + unsigned int frame_time_in_us;
> + u32 val = 0;
> +
> + val |= DARK_SCREEN_DETECT | DARK_SCREEN_DONE;
> + intel_de_rmw(dev_priv, DARK_SCREEN(crtc->config->cpu_transcoder), 0, val);
> +
> + frame_time_in_us = (1000 / drm_mode_vrefresh(&crtc_state->hw.adjusted_mode)) * 2;
Seems inaccurate.
> + 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_dbg_kms(&dev_priv->drm, "Dark screen detected:%c\n", pipe_name(crtc->pipe));
> + }
> +}
> +
> +void intel_darkscreen_disable(struct intel_crtc *crtc)
This is not called anywhere.
> +{
> + struct intel_crtc_state *crtc_state = crtc->config;
> + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> + 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..3b4b3d3df672
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_darkscreen.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef __INTEL_DARKSCREEN_H__
> +#define __INTEL_DARKSCREEN_H__
> +
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +
> +struct intel_crtc;
> +
> +struct intel_darkscreen {
> + bool enable;
> + struct work_struct darkscreen_detect_work;
Just "work" is enough and customary for the name. The context is
obvious.
> + struct intel_crtc *crtc;
> +};
This could be internal, and used as an opaque pointer, if the
allocations were handled properly.
> +
> +void intel_darkscreen_setup(struct intel_crtc *crtc);
> +int intel_darkscreen_enable(struct intel_crtc *crtc);
> +void intel_darkscreen_disable(struct intel_crtc *crtc);
> +void intel_darkscreen_schedule_work(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 62f7a30c37dc..177cf5ff8d77 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -50,6 +50,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"
> @@ -1511,6 +1512,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/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 8321ec4f9b46..c83217e20cb9 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -228,6 +228,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 \
--
Jani Nikula, Intel
