RE: [PATCH 3/3] drm/amdgpu: skip put fence if signal fails
[AMD Official Use Only - General]
Hi Andrey,
Thank you for your clarifying.
The refcount modified by amdgpu_fence_emit on my side is different.
I update my code and get the patch 'drm/amdgpu: Follow up change to previous
drm scheduler change.' , the " underflow " disappears.
My patch is not needed.
Thanks,
Jiadong
-Original Message-
From: Grodzovsky, Andrey
Sent: Friday, July 15, 2022 11:43 PM
To: Zhu, Jiadong ; Christian König
; amd-gfx@lists.freedesktop.org
Cc: Huang, Ray ; Liu, Aaron
Subject: Re: [PATCH 3/3] drm/amdgpu: skip put fence if signal fails
On 2022-07-15 05:28, Zhu, Jiadong wrote:
> [AMD Official Use Only - General]
>
> Updated some comments
>
> -Original Message-
> From: Zhu, Jiadong
> Sent: Friday, July 15, 2022 5:13 PM
> To: Christian König ;
> amd-gfx@lists.freedesktop.org; Grodzovsky, Andrey
>
> Cc: Huang, Ray ; Liu, Aaron
> Subject: RE: [PATCH 3/3] drm/amdgpu: skip put fence if signal fails
>
> Hi Christian,
>
> The resubmitted job in function amdgpu_ib_preempt_job_recovery returns the
> same hw fence because of this commit:
>
> static void amdgpu_ib_preempt_job_recovery(struct drm_gpu_scheduler *sched) {
> struct drm_sched_job *s_job;
> struct dma_fence *fence;
>
> spin_lock(>job_list_lock);
> list_for_each_entry(s_job, >pending_list, list) {
> fence = sched->ops->run_job(s_job); //fence returned
> has the same address with swapped fences
> dma_fence_put(fence);
> }
> spin_unlock(>job_list_lock);
> }
>
>
>
> commit c530b02f39850a639b72d01ebbf7e5d745c60831
> Author: Jack Zhang
> Date: Wed May 12 15:06:35 2021 +0800
>
> drm/amd/amdgpu embed hw_fence into amdgpu_job
>
> Why: Previously hw fence is alloced separately with job.
> It caused historical lifetime issues and corner cases.
> The ideal situation is to take fence to manage both job
> and fence's lifetime, and simplify the design of gpu-scheduler.
>
> How:
> We propose to embed hw_fence into amdgpu_job.
> 1. We cover the normal job submission by this method.
> 2. For ib_test, and submit without a parent job keep the
> legacy way to create a hw fence separately.
> v2:
> use AMDGPU_FENCE_FLAG_EMBED_IN_JOB_BIT to show that the fence is
> embedded in a job.
> v3:
> remove redundant variable ring in amdgpu_job
> v4:
> add tdr sequence support for this feature. Add a job_run_counter to
> indicate whether this job is a resubmit job.
> v5
> add missing handling in amdgpu_fence_enable_signaling
>
> Signed-off-by: Jingwen Chen
> Signed-off-by: Jack Zhang
> Reviewed-by: Andrey Grodzovsky
> Reviewed by: Monk Liu
> Signed-off-by: Alex Deucher
>
>
> Thus the fence we swapped out is signaled and put twice in the following 2
> functions and we get " refcount_t: underflow; use-after-free. " errors latter.
>
> /* wait for jobs finished */
> amdgpu_fence_wait_empty(ring); //wait on the resubmitted
> fence which is signaled and put somewhere else. The refcount decreased by 1
> after amdgpu_fence_wait_empty.
>
> /* signal the old fences */
> amdgpu_ib_preempt_signal_fences(fences, length); //signal
> and put the previous swapped fence, signal would return -22.
>
> Thanks,
> Jiadong
Did you have 'drm/amdgpu: Follow up change to previous drm scheduler change.'
this commit in your branch while you encountered this problem ?
I don't see an underflow issue
for the preempted job when inspecting the code with this commit in mind -
amdgpu_fence_emit
dma_fence_init 1
dma_fence_get(fence) 2
rcu_assign_pointer(*ptr, dma_fence_get(fence) 3
drm_sched_main
s_fence->parent = dma_fence_get(fence); 4
dma_fence_put(fence); 3
amdgpu_ib_preempt_job_recovery
amdgpu_fence_emit
if (job && job->job_run_counter) -> dma_fence_get(fence); 4
rcu_assign_pointer(*ptr, dma_fence_get(fence)); 5
dma_fence_put(fence); 4
amdgpu_fence_wait_empty
dma_fence_get_rcu(fence) 5
dma_fence_put(fence) 4
amdgpu_process_fence (EOP interrupt for re-submission of preempted job)
dma_fence_put 3
amdgpu_ib_preempt_signal_fences
dma_fence_put 2
amdgpu_job_free_cb
dma_fence_put(>hw_fence) 1
drm_sched_fence_release_scheduled
dma_fence_put(fence->parent); 0
Also take a look here for reference -
https://drive.google.com/file/d/1yEoeW6OQC9WnwmzFW6NBLhFP_jD0xcHm/view
Andrey
Andrey
>
>
> -Original Message-
> From: Christian König
> Sent: Friday, July 15, 2022 4:48 PM
> To: Zhu, Jiadong ; amd-gfx@lists.freedesktop.org;
> Grodzovsky, Andrey
> Cc: Huang, Ray ; Liu, Aaron
> Subject: Re: [PATCH 3/3] drm/amdgpu: skip put fence if signal fails
>
> [CAUTION: External Email]
>
> Am 15.07.22 um 10:43 schrieb jiadong@amd.com:
>> From: "Jiadong.Zhu"
>>
>>
RE: [PATCH] drm/amdgpu: correct the PSP_BL_CMD enum
[AMD Official Use Only - General]
Reviewed-by: John Clements
-Original Message-
From: Zhang, Hawking
Sent: Monday, July 18, 2022 11:35 AM
To: amd-gfx@lists.freedesktop.org; Clements, John
Cc: Zhang, Hawking
Subject: [PATCH] drm/amdgpu: correct the PSP_BL_CMD enum
To match with the enum defined in trusted os
Signed-off-by: Hawking Zhang
Reviewed-by: Le Ma
---
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
index e431f4994931..180634616b0f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
@@ -69,8 +69,8 @@ enum psp_bootloader_cmd {
PSP_BL__LOAD_SOSDRV = 0x2,
PSP_BL__LOAD_KEY_DATABASE = 0x8,
PSP_BL__LOAD_SOCDRV = 0xB,
- PSP_BL__LOAD_INTFDRV= 0xC,
- PSP_BL__LOAD_DBGDRV = 0xD,
+ PSP_BL__LOAD_DBGDRV = 0xC,
+ PSP_BL__LOAD_INTFDRV= 0xD,
PSP_BL__DRAM_LONG_TRAIN = 0x10,
PSP_BL__DRAM_SHORT_TRAIN= 0x20,
PSP_BL__LOAD_TOS_SPL_TABLE = 0x1000,
--
2.17.1
[PATCH] drm/amdgpu: correct the PSP_BL_CMD enum
To match with the enum defined in trusted os
Signed-off-by: Hawking Zhang
Reviewed-by: Le Ma
---
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
index e431f4994931..180634616b0f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h
@@ -69,8 +69,8 @@ enum psp_bootloader_cmd {
PSP_BL__LOAD_SOSDRV = 0x2,
PSP_BL__LOAD_KEY_DATABASE = 0x8,
PSP_BL__LOAD_SOCDRV = 0xB,
- PSP_BL__LOAD_INTFDRV= 0xC,
- PSP_BL__LOAD_DBGDRV = 0xD,
+ PSP_BL__LOAD_DBGDRV = 0xC,
+ PSP_BL__LOAD_INTFDRV= 0xD,
PSP_BL__DRAM_LONG_TRAIN = 0x10,
PSP_BL__DRAM_SHORT_TRAIN= 0x20,
PSP_BL__LOAD_TOS_SPL_TABLE = 0x1000,
--
2.17.1
Re: [PATCH 4/4] Documentation/gpu/amdgpu/amdgpu_dm: add DM docs for pixel blend mode
On 16/07/2022 19:25, Melissa Wen wrote: AMD GPU display manager (DM) maps DRM pixel blend modes (None, Pre-multiplied, Coverage) to MPC hw blocks through blend configuration options. Describe relevant elements and how to set and test them to get the expected DRM blend mode on DCN hw. Signed-off-by: Melissa Wen --- .../gpu/amdgpu/display/display-manager.rst| 98 +++ Documentation/gpu/drm-kms.rst | 2 + 2 files changed, 100 insertions(+) diff --git a/Documentation/gpu/amdgpu/display/display-manager.rst b/Documentation/gpu/amdgpu/display/display-manager.rst index 8960a5f1fa66..7a495ed1f69e 100644 --- a/Documentation/gpu/amdgpu/display/display-manager.rst +++ b/Documentation/gpu/amdgpu/display/display-manager.rst @@ -84,3 +84,101 @@ families below. **DCN 3.0 family color caps and mapping** .. kernel-figure:: dcn3_cm_drm_current.svg + +Blend Mode Properties += + +Pixel blend mode is a DRM plane composition property of :c:type:`drm_plane` used to +describes how pixels from a foreground plane (fg) are composited with the +background plane (bg). Here, we present main concepts of DRM blend mode to help +to understand how this property is mapped to AMD DC interface. See more about +this DRM property and the alpha blending equations in :ref:`DRM Plane +Composition Properties `. + +Basically, a blend mode sets the alpha blending equation for plane +composition that fits the mode in which the alpha channel affects the state of +pixel color values and, therefore, the resulted pixel color. For +example, consider the following elements of the alpha blending equation: + +- *fg.rgb*: Each of the RGB component values from the foreground's pixel. +- *fg.alpha*: Alpha component value from the foreground's pixel. +- *bg.rgb*: Each of the RGB component values from the background. +- *plane_alpha*: Plane alpha value set by the **plane "alpha" property**, see + more in `DRM Plane Composition Properties `. You forgot to use :ref: in here. + +in the basic alpha blending equation:: + + out.rgb = alpha * fg.rgb + (1 - alpha) * bg.rgb + +the alpha channel value of each pixel in a plane is ignored and only the plane +alpha affects the resulted pixel color values. + +DRM has three blend mode to define the blend formula in the plane composition: + +* **None**: Blend formula that ignores the pixel alpha. + +* **Pre-multiplied**: Blend formula that assumes the pixel color values in a + plane was already pre-multiplied by its own alpha channel before storage. + +* **Coverage**: Blend formula that assumes the pixel color values were not + pre-multiplied with the alpha channel values. + +and pre-multiplied is the default pixel blend mode, that means, when no blend +mode property is created or defined, DRM considers the plane's pixels has +pre-multiplied color values. On IGT GPU tools, the kms_plane_alpha_blend test +provides a set of subtests to verify plane alpha and blend mode properties. + +The DRM blend mode and its elements are then mapped by AMDGPU display manager +(DM) to program the blending configuration of the Multiple Pipe/Plane Combined +(MPC), as follows: + +.. kernel-doc:: drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h + :doc: mpc-overview + +.. kernel-doc:: drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h + :functions: mpcc_blnd_cfg + +Therefore, the blending configuration for a single MPCC instance on the MPC +tree is defined by :c:type:`mpcc_blnd_cfg`, where +:c:type:`pre_multiplied_alpha` is the alpha pre-multiplied mode flag used to +set :c:type:`MPCC_ALPHA_MULTIPLIED_MODE`. It controls whether alpha is +multiplied (true/false), being only true for DRM pre-multiplied blend mode. +:c:type:`mpcc_alpha_blend_mode` defines the alpha blend mode regarding pixel +alpha and plane alpha values. It sets one of the three modes for +:c:type:`MPCC_ALPHA_BLND_MODE`, as described below. + +.. kernel-doc:: drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h + :functions: mpcc_alpha_blend_mode + +DM then maps the elements of `enum mpcc_alpha_blend_mode` to those in the DRM +blend formula, as follows: + +* *MPC pixel alpha* matches *DRM fg.alpha* as the alpha component value + from the plane's pixel +* *MPC global alpha* matches *DRM plane_alpha* when the pixel alpha should + be ignored and, therefore, pixel values are not pre-multiplied +* *MPC global gain* assumes *MPC global alpha* value when both *DRM + fg.alpha* and *DRM plane_alpha* participate in the blend equation + +In short, *fg.alpha* is ignored by selecting +:c:type:`MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA`. On the other hand, (plane_alpha * +fg.alpha) component becomes available by selecting +:c:type:`MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN`. And the +:c:type:`MPCC_ALPHA_MULTIPLIED_MODE` defines if the pixel color values are +pre-multiplied by alpha or not. + +Blend configuration flow + + +The alpha blending equation is configured from DRM to DC interface by the
Re: [PATCH 3/4] drm/amd/display: add doc entries for MPC blending configuration
On 16/07/2022 19:25, Melissa Wen wrote:
Describe structs and enums used to set blend mode properties to MPC
blocks. Some pieces of information are already available as code
comments, and were just formatted. Others were collected and summarised
from discusssions on AMD issue tracker[1][2].
Typo in the commit message: discusssions -> discussions
[1] https://gitlab.freedesktop.org/drm/amd/-/issues/1734
[2] https://gitlab.freedesktop.org/drm/amd/-/issues/1769
Signed-off-by: Melissa Wen
---
drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h | 91 +
1 file changed, 77 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h
b/drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h
index 5097037e3962..cf28b841c42d 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h
@@ -22,6 +22,16 @@
*
*/
+/**
+ * DOC: mpc-overview
+ *
+ * Multiple Pipe/Plane Combined (MPC) is a component in the hardware pipeline
+ * that performs blending of multiple planes, using global and per-pixel alpha.
+ * It also performs post-blending color correction operations according to the
+ * hardware capabilities, such as color transformation matrix and gamma 1D and
+ * 3D LUT.
+ */
+
#ifndef __DC_MPCC_H__
#define __DC_MPCC_H__
@@ -48,14 +58,39 @@ enum mpcc_blend_mode {
MPCC_BLEND_MODE_TOP_BOT_BLENDING
};
+/**
+ * enum mpcc_alpha_blend_mode - define the alpha blend mode regarding pixel
+ * alpha and plane alpha values
+ */
enum mpcc_alpha_blend_mode {
+ /**
+* @MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA: per pixel alpha using DPP
+* alpha value
+*/
MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA,
+ /**
+* @MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN: per
+* pixel alpha using DPP alpha value multiplied by a global gain (plane
+* alpha)
+*/
MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN,
+ /**
+* @MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA: global alpha value, ignores
+* pixel alpha and consider only plane alpha
+*/
MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA
};
-/*
- * MPCC blending configuration
+/**
+ * struct mpcc_blnd_cfg - MPCC blending configuration
+ *
+ * @black_color: background color
+ * @alpha_mode: alpha blend mode (MPCC_ALPHA_BLND_MODE)
+ * @pre_multiplied_alpha: whether pixel color values were pre-multiplied by the
+ * alpha channel (MPCC_ALPHA_MULTIPLIED_MODE)
+ * @global_gain: used when blend mode considers both pixel alpha and plane
+ * alpha value and assumes the global alpha value.
+ * @global_alpha: plane alpha value
There's quite a few members missing definition. After reading the 4th
patch may I conclude that they weren't relevant for what's being
described about alpha blending?
./drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h:109: warning: Function
parameter or member 'overlap_only' not described in 'mpcc_blnd_cfg'
./drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h:109: warning: Function
parameter or member 'bottom_gain_mode' not described in 'mpcc_blnd_cfg'
./drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h:109: warning: Function
parameter or member 'background_color_bpc' not described in 'mpcc_blnd_cfg'
./drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h:109: warning: Function
parameter or member 'top_gain' not described in 'mpcc_blnd_cfg'
./drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h:109: warning: Function
parameter or member 'bottom_inside_gain' not described in 'mpcc_blnd_cfg'
./drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h:109: warning: Function
parameter or member 'bottom_outside_gain' not described in 'mpcc_blnd_cfg'
*/
struct mpcc_blnd_cfg {
struct tg_color black_color;/* background color */
@@ -107,8 +142,15 @@ struct mpc_dwb_flow_control {
int flow_ctrl_cnt1;
};
-/*
- * MPCC connection and blending configuration for a single MPCC instance.
+/**
+ * struct mpcc - MPCC connection and blending configuration for a single MPCC
instance.
Might be worth writing the definition of the abbreviation, if not here,
in the glossary... I couldn't find what the last "C" stands for, my
guess would be "context". hehehe
+ * @mpcc_id: MPCC physical instance
+ * @dpp_id: DPP input to this MPCC
+ * @mpcc_bot: pointer to bottom layer MPCC. NULL when not connected.
+ * @blnd_cfg: the blending configuration for this MPCC
+ * @sm_cfg: stereo mix setting for this MPCC
+ * @shared_bottom: if MPCC output to both OPP and DWB endpoints, true.
Othewise, false.
Typo Othewise -> Otherwise
+ *
* This struct is used as a node in an MPC tree.
*/
struct mpcc {
@@ -120,8 +162,12 @@ struct mpcc {
bool shared_bottom; /* TRUE if MPCC output to both OPP and
DWB endpoints, else FALSE */
};
-/*
- * MPC tree represents all MPCC connections for a pipe.
+/**
+ * struct mpc_tree - MPC tree represents all MPCC connections for a pipe.
+
Re: [PATCH 2/4] Documentation/amdgpu/display: add DC color caps info
On 16/07/2022 19:25, Melissa Wen wrote: Add details about color correction capabilities and explain a bit about differences between DC hw generations and also how they are mapped between DRM and DC interface. Two schemas for DCN 2.0 and 3.0 (converted to svg from the original png) is included to illustrate it. They were obtained from a discussion[1] in the amd-gfx mailing list. [1] https://lore.kernel.org/amd-gfx/20220422142811.dm6vtk6v64jcw...@mail.igalia.com/ v2: - remove redundant comments (Harry) - fix typo (Harry) Signed-off-by: Melissa Wen --- .../amdgpu/display/dcn2_cm_drm_current.svg| 1370 +++ .../amdgpu/display/dcn3_cm_drm_current.svg| 1529 + .../gpu/amdgpu/display/display-manager.rst| 35 + drivers/gpu/drm/amd/display/dc/dc.h | 74 +- 4 files changed, 2995 insertions(+), 13 deletions(-) create mode 100644 Documentation/gpu/amdgpu/display/dcn2_cm_drm_current.svg create mode 100644 Documentation/gpu/amdgpu/display/dcn3_cm_drm_current.svg diff --git a/Documentation/gpu/amdgpu/display/dcn2_cm_drm_current.svg b/Documentation/gpu/amdgpu/display/dcn2_cm_drm_current.svg new file mode 100644 index ..315ffc5a1a4b --- /dev/null +++ b/Documentation/gpu/amdgpu/display/dcn2_cm_drm_current.svg @@ -0,0 +1,1370 @@ + + + +http://www.inkscape.org/namespaces/inkscape; + xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd; + xmlns="http://www.w3.org/2000/svg; + xmlns:svg="http://www.w3.org/2000/svg;> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +Matrix +1D LUT +3D LUT +Unpacking +Other +drm_framebuffer +format +drm_plane +drm_crtc +Stream +MPC +DPP + +Blender +Degamma +CTM +Gamma +format +bias_and_scale +color space matrix +input_csc_color_matrix +in_transfer_func +hdr_mult +gamut_remap_matrix +in_shaper_func +lut3d_func +blend_tf +Blender +gamut_remap_matrix +func_shaper +lut3d_func +out_transfer_func +csc_color_matrix +bit_depth_param +clamping +output_color_space +Plane +Legend +DCN 2.0 +DC Interface +DRM Interface + +CNVC +Input CSC +DeGammaRAM and ROM(sRGB, BT2020 +HDR Multiply +Gamut Remap +Shaper LUTRAM +3D LUTRAM +Blend Gamma +Blender +GammaRAM +OCSC + + +color_encoding + +pixel_blend_mode + +color_range + + + + + + + + + + + + + + diff --git a/Documentation/gpu/amdgpu/display/dcn3_cm_drm_current.svg b/Documentation/gpu/amdgpu/display/dcn3_cm_drm_current.svg new file mode 100644 index ..7299ee9b6d64 --- /dev/null +++ b/Documentation/gpu/amdgpu/display/dcn3_cm_drm_current.svg @@ -0,0 +1,1529 @@ + + + +http://www.inkscape.org/namespaces/inkscape; + xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd; + xmlns="http://www.w3.org/2000/svg; + xmlns:svg="http://www.w3.org/2000/svg;> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +Matrix +1D LUT +3D LUT +Unpacking +Other +drm_framebuffer +format +drm_plane +drm_crtc +Stream +MPC +DPP + +Blender +Degamma +CTM +Gamma +format +bias_and_scale +color space matrix +input_csc_color_matrix +in_transfer_func +hdr_mult +gamut_remap_matrix +in_shaper_func +lut3d_func +blend_tf +Blender +gamut_remap_matrix +func_shaper +lut3d_func +out_transfer_func +csc_color_matrix +bit_depth_param +clamping +output_color_space +Plane +Legend +DCN 3.0 +DC Interface +DRM Interface + +CNVC +Input CSC +DeGammaROM(sRGB, BT2020, Gamma 2.2,PQ, HLG) +Post CSC +Gamma Correction +HDR Multiply +Gamut Remap +Shaper LUTRAM +3D LUTRAM +Blend Gamma +Blender +Gamut Remap +Shaper LUTRAM +3D LUTRAM +GammaRAM +
Re: [PATCH 1/4] Documentation/amdgpu_dm: Add DM color correction documentation
On 16/07/2022 19:25, Melissa Wen wrote:
AMDGPU DM maps DRM color management properties (degamma, ctm and gamma)
to DC color correction entities. Part of this mapping is already
documented as code comments and can be converted as kernel docs.
v2:
- rebase to amd-staging-drm-next
Reviewed-by: Harry Wentland
Signed-off-by: Melissa Wen
---
.../gpu/amdgpu/display/display-manager.rst| 9 ++
.../amd/display/amdgpu_dm/amdgpu_dm_color.c | 121 +-
2 files changed, 98 insertions(+), 32 deletions(-)
diff --git a/Documentation/gpu/amdgpu/display/display-manager.rst
b/Documentation/gpu/amdgpu/display/display-manager.rst
index 7ce31f89d9a0..b1b0f11aed83 100644
--- a/Documentation/gpu/amdgpu/display/display-manager.rst
+++ b/Documentation/gpu/amdgpu/display/display-manager.rst
@@ -40,3 +40,12 @@ Atomic Implementation
.. kernel-doc:: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
:functions: amdgpu_dm_atomic_check amdgpu_dm_atomic_commit_tail
+
+Color Management Properties
+===
+
+.. kernel-doc:: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+ :doc: overview
+
+.. kernel-doc:: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+ :internal:
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index a71177305bcd..93c813089bff 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -29,7 +29,9 @@
#include "modules/color/color_gamma.h"
#include "basics/conversion.h"
-/*
+/**
+ * DOC: overview
+ *
* The DC interface to HW gives us the following color management blocks
* per pipe (surface):
*
@@ -71,8 +73,8 @@
#define MAX_DRM_LUT_VALUE 0x
-/*
- * Initialize the color module.
+/**
+ * amdgpu_dm_init_color_mod - Initialize the color module.
*
* We're not using the full color module, only certain components.
* Only call setup functions for components that we need.
@@ -82,7 +84,14 @@ void amdgpu_dm_init_color_mod(void)
setup_x_points_distribution();
}
-/* Extracts the DRM lut and lut size from a blob. */
+/**
+ * __extract_blob_lut - Extracts the DRM lut and lut size from a blob.
+ * @blob: DRM color mgmt property blob
+ * @size: lut size
+ *
+ * Returns:
+ * DRM LUT or NULL
+ */
static const struct drm_color_lut *
__extract_blob_lut(const struct drm_property_blob *blob, uint32_t *size)
{
@@ -90,13 +99,18 @@ __extract_blob_lut(const struct drm_property_blob *blob,
uint32_t *size)
return blob ? (struct drm_color_lut *)blob->data : NULL;
}
I don't think everyone would approve using actual kernel-doc for these
static functions, but I can appreciate they being formatted as such.
Consider replacing /** with /*.
-/*
- * Return true if the given lut is a linear mapping of values, i.e. it acts
- * like a bypass LUT.
+/**
+ * __is_lut_linear - check if the given lut is a linear mapping of values
+ * @lut: given lut to check values
+ * @size: lut size
*
* It is considered linear if the lut represents:
- * f(a) = (0xFF00/MAX_COLOR_LUT_ENTRIES-1)a; for integer a in
- * [0, MAX_COLOR_LUT_ENTRIES)
+ * f(a) = (0xFF00/MAX_COLOR_LUT_ENTRIES-1)a; for integer a in [0,
+ * MAX_COLOR_LUT_ENTRIES)
+ *
+ * Returns:
+ * True if the given lut is a linear mapping of values, i.e. it acts like a
+ * bypass LUT. Otherwise, false.
*/
static bool __is_lut_linear(const struct drm_color_lut *lut, uint32_t size)
{
@@ -119,9 +133,13 @@ static bool __is_lut_linear(const struct drm_color_lut
*lut, uint32_t size)
return true;
}
-/*
- * Convert the drm_color_lut to dc_gamma. The conversion depends on the size
- * of the lut - whether or not it's legacy.
+/**
+ * __drm_lut_to_dc_gamma - convert the drm_color_lut to dc_gamma.
+ * @lut: DRM lookup table for color conversion
+ * @gamma: DC gamma to set entries
+ * @is_legacy: legacy or atomic gamma
+ *
+ * The conversion depends on the size of the lut - whether or not it's legacy.
*/
static void __drm_lut_to_dc_gamma(const struct drm_color_lut *lut,
struct dc_gamma *gamma, bool is_legacy)
@@ -154,8 +172,11 @@ static void __drm_lut_to_dc_gamma(const struct
drm_color_lut *lut,
}
}
-/*
- * Converts a DRM CTM to a DC CSC float matrix.
+/**
+ * __drm_ctm_to_dc_matrix - converts a DRM CTM to a DC CSC float matrix
+ * @ctm: DRM color transformation matrix
+ * @matrix: DC CSC float matrix
+ *
* The matrix needs to be a 3x4 (12 entry) matrix.
*/
static void __drm_ctm_to_dc_matrix(const struct drm_color_ctm *ctm,
@@ -189,7 +210,18 @@ static void __drm_ctm_to_dc_matrix(const struct
drm_color_ctm *ctm,
}
}
-/* Calculates the legacy transfer function - only for sRGB input space. */
+/**
+ * __set_legacy_tf - Calculates the legacy transfer function
+ * @func: transfer function
freedesktop
Dear Manager, (Please forward this to your CEO, because this is urgent. Thanks!) We are a Network Service Company which is the domain name registration center in Shanghai, China. On July 11, 2022, we received an application from Lishen Holdings Ltd requested “freedesktop” as their internet keyword and China (CN) domain names( freedesktop.cn/ freedesktop.com.cn/ freedesktop.net.cn/ freedesktop.org.cn). After checking it, we find this name conflict with your company name or trademark. In order to deal with this matter better, we send email to you and confirm whether this company is your distributor or business partner in China? Kind regards Brian - Mr.Brian Tang | Services & Operating Manager Asia Network | Headquarters | w w w[.]as ian etwo rk[.]or g[.]cn 8008, Tianan Building, No. 1399 Jinqiao Road, Shangh ai 200120, Chin a T: 008 6-134- 8281-9147 | Tel: 008 6- 21-6191-8696 | Fax: 008 6-21- 6191-869 7 - This email contains privileged and confidential information intended for the addressee only. If you are not the intended recipient, please destroy this email and inform the sender immediately. We appreciate you respecting the confidentiality of this information by not disclosing or using the information in this email.
Re: [PATCH v2 16/29] ACPI: video: Add Nvidia WMI EC brightness control detection
Hi,
On 7/15/22 17:32, Daniel Dadap wrote:
>
>
>> On Jul 15, 2022, at 06:59, Hans de Goede wrote:
>>
>> Hi Daniel,
>>
>>> On 7/12/22 22:13, Daniel Dadap wrote:
>>> Thanks, Hans:
>>>
On 7/12/22 14:38, Hans de Goede wrote:
On some new laptop designs a new Nvidia specific WMI interface is present
which gives info about panel brightness control and may allow controlling
the brightness through this interface when the embedded controller is used
for brightness control.
When this WMI interface is present and indicates that the EC is used,
then this interface should be used for brightness control.
Signed-off-by: Hans de Goede
---
drivers/acpi/Kconfig | 1 +
drivers/acpi/video_detect.c| 35 ++
drivers/gpu/drm/gma500/Kconfig | 2 ++
drivers/gpu/drm/i915/Kconfig | 2 ++
include/acpi/video.h | 1 +
5 files changed, 41 insertions(+)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 1e34f846508f..c372385cfc3f 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -212,6 +212,7 @@ config ACPI_VIDEO
tristate "Video"
depends on X86 && BACKLIGHT_CLASS_DEVICE
depends on INPUT
+depends on ACPI_WMI
select THERMAL
help
This driver implements the ACPI Extensions For Display Adapters
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 8c2863403040..7b89dc9a04a2 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -75,6 +75,35 @@ find_video(acpi_handle handle, u32 lvl, void *context,
void **rv)
return AE_OK;
}
+#define WMI_BRIGHTNESS_METHOD_SOURCE2
+#define WMI_BRIGHTNESS_MODE_GET0
+#define WMI_BRIGHTNESS_SOURCE_EC2
+
+struct wmi_brightness_args {
+u32 mode;
+u32 val;
+u32 ret;
+u32 ignored[3];
+};
+
+static bool nvidia_wmi_ec_supported(void)
+{
+struct wmi_brightness_args args = {
+.mode = WMI_BRIGHTNESS_MODE_GET,
+.val = 0,
+.ret = 0,
+};
+struct acpi_buffer buf = { (acpi_size)sizeof(args), };
+acpi_status status;
+
+status = wmi_evaluate_method("603E9613-EF25-4338-A3D0-C46177516DB7",
0,
+ WMI_BRIGHTNESS_METHOD_SOURCE, , );
+if (ACPI_FAILURE(status))
+return false;
+
+return args.ret == WMI_BRIGHTNESS_SOURCE_EC;
+}
+
>>>
>>>
>>> The code duplication here with nvidia-wmi-ec-backlight.c is a little
>>> unfortunate. Can we move the constants, struct definition, and WMI GUID
>>> from that file to a header file that's used both by the EC backlight driver
>>> and the ACPI video driver?
>>
>> Yes that is a good idea. I suggest using
>> include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
>> to move the shared definitions there.
>>
>> If you can submit 2 patches on top of this series:
>>
>> 1. Moving the definitions from
>> drivers/platform/x86/nvidia-wmi-ec-backlight.c to
>> include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
>>
>> 2. Switching the code from this patch over to using the new
>> nvidia-wmi-ec-backlight.h
>>
>> Then for the next version I'll add patch 1. to the series and squash patch 2.
>> into this one.
>>
>>> I was thinking it might be nice to add a wrapper around
>>> wmi_brightness_notify() in nvidia-wmi-ec-backlight.c that does this source
>>> == WMI_BRIGHTNESS_SOURCE_EC test, and then export it so that it can be
>>> called both here and in the EC backlight driver's probe routine, but then I
>>> guess that would make video.ko depend on nvidia-wmi-ec-backlight.ko, which
>>> seems wrong. It also seems wrong to implement the WMI plumbing in the ACPI
>>> video driver, and export it so that the EC backlight driver can use it, so
>>> I guess I can live with the duplication of the relatively simple WMI stuff
>>> here, it would just be nice to not have to define all of the API constants,
>>> structure, and GUID twice.
>>
>> Agreed.
>>
>>>
>>>
/* Force to use vendor driver when the ACPI device is known to be
* buggy */
static int video_detect_force_vendor(const struct dmi_system_id *d)
@@ -518,6 +547,7 @@ static const struct dmi_system_id
video_detect_dmi_table[] = {
static enum acpi_backlight_type __acpi_video_get_backlight_type(bool
native)
{
static DEFINE_MUTEX(init_mutex);
+static bool nvidia_wmi_ec_present;
static bool native_available;
static bool init_done;
static long video_caps;
@@ -530,6 +560,7 @@ static enum acpi_backlight_type
__acpi_video_get_backlight_type(bool native)
Re: [PATCH v2 16/29] ACPI: video: Add Nvidia WMI EC brightness control detection
> On Jul 15, 2022, at 06:59, Hans de Goede wrote:
>
> Hi Daniel,
>
>> On 7/12/22 22:13, Daniel Dadap wrote:
>> Thanks, Hans:
>>
>>> On 7/12/22 14:38, Hans de Goede wrote:
>>> On some new laptop designs a new Nvidia specific WMI interface is present
>>> which gives info about panel brightness control and may allow controlling
>>> the brightness through this interface when the embedded controller is used
>>> for brightness control.
>>>
>>> When this WMI interface is present and indicates that the EC is used,
>>> then this interface should be used for brightness control.
>>>
>>> Signed-off-by: Hans de Goede
>>> ---
>>> drivers/acpi/Kconfig | 1 +
>>> drivers/acpi/video_detect.c| 35 ++
>>> drivers/gpu/drm/gma500/Kconfig | 2 ++
>>> drivers/gpu/drm/i915/Kconfig | 2 ++
>>> include/acpi/video.h | 1 +
>>> 5 files changed, 41 insertions(+)
>>>
>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>>> index 1e34f846508f..c372385cfc3f 100644
>>> --- a/drivers/acpi/Kconfig
>>> +++ b/drivers/acpi/Kconfig
>>> @@ -212,6 +212,7 @@ config ACPI_VIDEO
>>> tristate "Video"
>>> depends on X86 && BACKLIGHT_CLASS_DEVICE
>>> depends on INPUT
>>> +depends on ACPI_WMI
>>> select THERMAL
>>> help
>>> This driver implements the ACPI Extensions For Display Adapters
>>> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
>>> index 8c2863403040..7b89dc9a04a2 100644
>>> --- a/drivers/acpi/video_detect.c
>>> +++ b/drivers/acpi/video_detect.c
>>> @@ -75,6 +75,35 @@ find_video(acpi_handle handle, u32 lvl, void *context,
>>> void **rv)
>>> return AE_OK;
>>> }
>>> +#define WMI_BRIGHTNESS_METHOD_SOURCE2
>>> +#define WMI_BRIGHTNESS_MODE_GET0
>>> +#define WMI_BRIGHTNESS_SOURCE_EC2
>>> +
>>> +struct wmi_brightness_args {
>>> +u32 mode;
>>> +u32 val;
>>> +u32 ret;
>>> +u32 ignored[3];
>>> +};
>>> +
>>> +static bool nvidia_wmi_ec_supported(void)
>>> +{
>>> +struct wmi_brightness_args args = {
>>> +.mode = WMI_BRIGHTNESS_MODE_GET,
>>> +.val = 0,
>>> +.ret = 0,
>>> +};
>>> +struct acpi_buffer buf = { (acpi_size)sizeof(args), };
>>> +acpi_status status;
>>> +
>>> +status = wmi_evaluate_method("603E9613-EF25-4338-A3D0-C46177516DB7", 0,
>>> + WMI_BRIGHTNESS_METHOD_SOURCE, , );
>>> +if (ACPI_FAILURE(status))
>>> +return false;
>>> +
>>> +return args.ret == WMI_BRIGHTNESS_SOURCE_EC;
>>> +}
>>> +
>>
>>
>> The code duplication here with nvidia-wmi-ec-backlight.c is a little
>> unfortunate. Can we move the constants, struct definition, and WMI GUID from
>> that file to a header file that's used both by the EC backlight driver and
>> the ACPI video driver?
>
> Yes that is a good idea. I suggest using
> include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
> to move the shared definitions there.
>
> If you can submit 2 patches on top of this series:
>
> 1. Moving the definitions from drivers/platform/x86/nvidia-wmi-ec-backlight.c
> to
> include/linux/platform_data/x86/nvidia-wmi-ec-backlight.h
>
> 2. Switching the code from this patch over to using the new
> nvidia-wmi-ec-backlight.h
>
> Then for the next version I'll add patch 1. to the series and squash patch 2.
> into this one.
>
>> I was thinking it might be nice to add a wrapper around
>> wmi_brightness_notify() in nvidia-wmi-ec-backlight.c that does this source
>> == WMI_BRIGHTNESS_SOURCE_EC test, and then export it so that it can be
>> called both here and in the EC backlight driver's probe routine, but then I
>> guess that would make video.ko depend on nvidia-wmi-ec-backlight.ko, which
>> seems wrong. It also seems wrong to implement the WMI plumbing in the ACPI
>> video driver, and export it so that the EC backlight driver can use it, so I
>> guess I can live with the duplication of the relatively simple WMI stuff
>> here, it would just be nice to not have to define all of the API constants,
>> structure, and GUID twice.
>
> Agreed.
>
>>
>>
>>> /* Force to use vendor driver when the ACPI device is known to be
>>>* buggy */
>>> static int video_detect_force_vendor(const struct dmi_system_id *d)
>>> @@ -518,6 +547,7 @@ static const struct dmi_system_id
>>> video_detect_dmi_table[] = {
>>> static enum acpi_backlight_type __acpi_video_get_backlight_type(bool
>>> native)
>>> {
>>> static DEFINE_MUTEX(init_mutex);
>>> +static bool nvidia_wmi_ec_present;
>>> static bool native_available;
>>> static bool init_done;
>>> static long video_caps;
>>> @@ -530,6 +560,7 @@ static enum acpi_backlight_type
>>> __acpi_video_get_backlight_type(bool native)
>>> acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
>>> ACPI_UINT32_MAX, find_video, NULL,
>>> _caps, NULL);
>>> +
