Re: [Freedreno] [PATCH v1 8/8] drm/msm/dpu: move SSPP debugfs support from plane to SSPP code
On 10/12/2021 01:27, Abhinav Kumar wrote:
On 12/9/2021 2:18 PM, Abhinav Kumar wrote:
On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
We are preparing to change DPU plane implementation. Move SSPP debugfs
code from dpu_plane.c to dpu_hw_sspp.c, where it belongs.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 67 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 82 +++--
4 files changed, 84 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
index d77eb7da5daf..ae3cf2e4d7d9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -8,6 +8,8 @@
#include "dpu_hw_sspp.h"
#include "dpu_kms.h"
+#include
+
#define DPU_FETCH_CONFIG_RESET_VALUE 0x0087
/* DPU_SSPP_SRC */
@@ -686,6 +688,71 @@ static void _setup_layer_ops(struct dpu_hw_pipe *c,
c->ops.setup_cdp = dpu_hw_sspp_setup_cdp;
}
+#ifdef CONFIG_DEBUG_FS
+int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct
dpu_kms *kms, struct dentry *entry)
+{
+ const struct dpu_sspp_cfg *cfg = hw_pipe->cap;
+ const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
+ struct dentry *debugfs_root;
+ char sspp_name[32];
+
+ snprintf(sspp_name, sizeof(sspp_name), "%d", hw_pipe->idx);
+
+ /* create overall sub-directory for the pipe */
+ debugfs_root =
+ debugfs_create_dir(sspp_name, entry);
I would like to take a different approach to this. Let me know what
you think.
Let the directory names still be the drm plane names as someone who
would first get the DRM state and then try to lookup the register
values of that plane would not know where to look now.
Inside the /sys/kernel/debug/***/plane-X/ directory you can expose an
extra entry which tells the sspp_index.
This will also establish the plane to SSPP mapping.
Now when planes go virtual in the future, this will be helpful even more
so that we can know the plane to SSPP mapping.
OR i like rob's suggestion of implementing the atomic_print_state
callback which will printout the drm plane to SSPP mapping along with
this change so that when we look at DRM state, we also know the plane
to SSPP mapping and look in the right SSPP's dir.
I'd add atomic_print_state(), it seems simpler (and more future-proof).
+
+ /* don't error check these */
+ debugfs_create_xul("features", 0600,
+ debugfs_root, (unsigned long *)_pipe->cap->features);
+
+ /* add register dump support */
+ dpu_debugfs_create_regset32("src_blk", 0400,
+ debugfs_root,
+ sblk->src_blk.base + cfg->base,
+ sblk->src_blk.len,
+ kms);
+
+ if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
+ cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
+ cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
+ cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
+ dpu_debugfs_create_regset32("scaler_blk", 0400,
+ debugfs_root,
+ sblk->scaler_blk.base + cfg->base,
+ sblk->scaler_blk.len,
+ kms);
+
+ if (cfg->features & BIT(DPU_SSPP_CSC) ||
+ cfg->features & BIT(DPU_SSPP_CSC_10BIT))
+ dpu_debugfs_create_regset32("csc_blk", 0400,
+ debugfs_root,
+ sblk->csc_blk.base + cfg->base,
+ sblk->csc_blk.len,
+ kms);
+
+ debugfs_create_u32("xin_id",
+ 0400,
+ debugfs_root,
+ (u32 *) >xin_id);
+ debugfs_create_u32("clk_ctrl",
+ 0400,
+ debugfs_root,
+ (u32 *) >clk_ctrl);
+ debugfs_create_x32("creq_vblank",
+ 0600,
+ debugfs_root,
+ (u32 *) >creq_vblank);
+ debugfs_create_x32("danger_vblank",
+ 0600,
+ debugfs_root,
+ (u32 *) >danger_vblank);
+
+ return 0;
+}
+#endif
+
+
static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
void __iomem *addr,
struct dpu_mdss_cfg *catalog,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
index e8939d7387cb..cef281687bab 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -381,6 +381,7 @@ struct dpu_hw_pipe {
struct dpu_hw_sspp_ops ops;
};
+struct dpu_kms;
/**
* dpu_hw_sspp_init - initializes the sspp hw driver object.
* Should be called once before accessing every pipe.
@@ -400,5 +401,8 @@ struct dpu_hw_pipe *dpu_hw_sspp_init(enum
dpu_sspp idx,
*/
void dpu_hw_sspp_destroy(struct dpu_hw_pipe *ctx);
+void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry
*debugfs_root);
+int _dpu_hw_sspp_init_debugfs(struct
Re: [Freedreno] [PATCH v1 8/8] drm/msm/dpu: move SSPP debugfs support from plane to SSPP code
On 12/9/2021 2:18 PM, Abhinav Kumar wrote:
On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
We are preparing to change DPU plane implementation. Move SSPP debugfs
code from dpu_plane.c to dpu_hw_sspp.c, where it belongs.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 67 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 82 +++--
4 files changed, 84 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
index d77eb7da5daf..ae3cf2e4d7d9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -8,6 +8,8 @@
#include "dpu_hw_sspp.h"
#include "dpu_kms.h"
+#include
+
#define DPU_FETCH_CONFIG_RESET_VALUE 0x0087
/* DPU_SSPP_SRC */
@@ -686,6 +688,71 @@ static void _setup_layer_ops(struct dpu_hw_pipe *c,
c->ops.setup_cdp = dpu_hw_sspp_setup_cdp;
}
+#ifdef CONFIG_DEBUG_FS
+int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct
dpu_kms *kms, struct dentry *entry)
+{
+ const struct dpu_sspp_cfg *cfg = hw_pipe->cap;
+ const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
+ struct dentry *debugfs_root;
+ char sspp_name[32];
+
+ snprintf(sspp_name, sizeof(sspp_name), "%d", hw_pipe->idx);
+
+ /* create overall sub-directory for the pipe */
+ debugfs_root =
+ debugfs_create_dir(sspp_name, entry);
I would like to take a different approach to this. Let me know what you
think.
Let the directory names still be the drm plane names as someone who
would first get the DRM state and then try to lookup the register values
of that plane would not know where to look now.
Inside the /sys/kernel/debug/***/plane-X/ directory you can expose an
extra entry which tells the sspp_index.
This will also establish the plane to SSPP mapping.
Now when planes go virtual in the future, this will be helpful even more
so that we can know the plane to SSPP mapping.
OR i like rob's suggestion of implementing the atomic_print_state
callback which will printout the drm plane to SSPP mapping along with
this change so that when we look at DRM state, we also know the plane
to SSPP mapping and look in the right SSPP's dir.
+
+ /* don't error check these */
+ debugfs_create_xul("features", 0600,
+ debugfs_root, (unsigned long *)_pipe->cap->features);
+
+ /* add register dump support */
+ dpu_debugfs_create_regset32("src_blk", 0400,
+ debugfs_root,
+ sblk->src_blk.base + cfg->base,
+ sblk->src_blk.len,
+ kms);
+
+ if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
+ cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
+ cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
+ cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
+ dpu_debugfs_create_regset32("scaler_blk", 0400,
+ debugfs_root,
+ sblk->scaler_blk.base + cfg->base,
+ sblk->scaler_blk.len,
+ kms);
+
+ if (cfg->features & BIT(DPU_SSPP_CSC) ||
+ cfg->features & BIT(DPU_SSPP_CSC_10BIT))
+ dpu_debugfs_create_regset32("csc_blk", 0400,
+ debugfs_root,
+ sblk->csc_blk.base + cfg->base,
+ sblk->csc_blk.len,
+ kms);
+
+ debugfs_create_u32("xin_id",
+ 0400,
+ debugfs_root,
+ (u32 *) >xin_id);
+ debugfs_create_u32("clk_ctrl",
+ 0400,
+ debugfs_root,
+ (u32 *) >clk_ctrl);
+ debugfs_create_x32("creq_vblank",
+ 0600,
+ debugfs_root,
+ (u32 *) >creq_vblank);
+ debugfs_create_x32("danger_vblank",
+ 0600,
+ debugfs_root,
+ (u32 *) >danger_vblank);
+
+ return 0;
+}
+#endif
+
+
static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
void __iomem *addr,
struct dpu_mdss_cfg *catalog,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
index e8939d7387cb..cef281687bab 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -381,6 +381,7 @@ struct dpu_hw_pipe {
struct dpu_hw_sspp_ops ops;
};
+struct dpu_kms;
/**
* dpu_hw_sspp_init - initializes the sspp hw driver object.
* Should be called once before accessing every pipe.
@@ -400,5 +401,8 @@ struct dpu_hw_pipe *dpu_hw_sspp_init(enum dpu_sspp
idx,
*/
void dpu_hw_sspp_destroy(struct dpu_hw_pipe *ctx);
+void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry
*debugfs_root);
+int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct
dpu_kms *kms, struct dentry *entry);
+
#endif /*_DPU_HW_SSPP_H */
diff --git
Re: [Freedreno] [PATCH v1 8/8] drm/msm/dpu: move SSPP debugfs support from plane to SSPP code
On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
We are preparing to change DPU plane implementation. Move SSPP debugfs
code from dpu_plane.c to dpu_hw_sspp.c, where it belongs.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 67 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 4 +
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 82 +++--
4 files changed, 84 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
index d77eb7da5daf..ae3cf2e4d7d9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -8,6 +8,8 @@
#include "dpu_hw_sspp.h"
#include "dpu_kms.h"
+#include
+
#define DPU_FETCH_CONFIG_RESET_VALUE 0x0087
/* DPU_SSPP_SRC */
@@ -686,6 +688,71 @@ static void _setup_layer_ops(struct dpu_hw_pipe *c,
c->ops.setup_cdp = dpu_hw_sspp_setup_cdp;
}
+#ifdef CONFIG_DEBUG_FS
+int _dpu_hw_sspp_init_debugfs(struct dpu_hw_pipe *hw_pipe, struct dpu_kms
*kms, struct dentry *entry)
+{
+ const struct dpu_sspp_cfg *cfg = hw_pipe->cap;
+ const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
+ struct dentry *debugfs_root;
+ char sspp_name[32];
+
+ snprintf(sspp_name, sizeof(sspp_name), "%d", hw_pipe->idx);
+
+ /* create overall sub-directory for the pipe */
+ debugfs_root =
+ debugfs_create_dir(sspp_name, entry);
I would like to take a different approach to this. Let me know what you
think.
Let the directory names still be the drm plane names as someone who
would first get the DRM state and then try to lookup the register values
of that plane would not know where to look now.
Inside the /sys/kernel/debug/***/plane-X/ directory you can expose an
extra entry which tells the sspp_index.
This will also establish the plane to SSPP mapping.
Now when planes go virtual in the future, this will be helpful even more
so that we can know the plane to SSPP mapping.
+
+ /* don't error check these */
+ debugfs_create_xul("features", 0600,
+ debugfs_root, (unsigned long *)_pipe->cap->features);
+
+ /* add register dump support */
+ dpu_debugfs_create_regset32("src_blk", 0400,
+ debugfs_root,
+ sblk->src_blk.base + cfg->base,
+ sblk->src_blk.len,
+ kms);
+
+ if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
+ cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
+ cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
+ cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
+ dpu_debugfs_create_regset32("scaler_blk", 0400,
+ debugfs_root,
+ sblk->scaler_blk.base + cfg->base,
+ sblk->scaler_blk.len,
+ kms);
+
+ if (cfg->features & BIT(DPU_SSPP_CSC) ||
+ cfg->features & BIT(DPU_SSPP_CSC_10BIT))
+ dpu_debugfs_create_regset32("csc_blk", 0400,
+ debugfs_root,
+ sblk->csc_blk.base + cfg->base,
+ sblk->csc_blk.len,
+ kms);
+
+ debugfs_create_u32("xin_id",
+ 0400,
+ debugfs_root,
+ (u32 *) >xin_id);
+ debugfs_create_u32("clk_ctrl",
+ 0400,
+ debugfs_root,
+ (u32 *) >clk_ctrl);
+ debugfs_create_x32("creq_vblank",
+ 0600,
+ debugfs_root,
+ (u32 *) >creq_vblank);
+ debugfs_create_x32("danger_vblank",
+ 0600,
+ debugfs_root,
+ (u32 *) >danger_vblank);
+
+ return 0;
+}
+#endif
+
+
static const struct dpu_sspp_cfg *_sspp_offset(enum dpu_sspp sspp,
void __iomem *addr,
struct dpu_mdss_cfg *catalog,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
index e8939d7387cb..cef281687bab 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -381,6 +381,7 @@ struct dpu_hw_pipe {
struct dpu_hw_sspp_ops ops;
};
+struct dpu_kms;
/**
* dpu_hw_sspp_init - initializes the sspp hw driver object.
* Should be called once before accessing every pipe.
@@ -400,5 +401,8 @@ struct dpu_hw_pipe *dpu_hw_sspp_init(enum dpu_sspp idx,
*/
void dpu_hw_sspp_destroy(struct dpu_hw_pipe *ctx);
+void dpu_debugfs_sspp_init(struct dpu_kms *dpu_kms, struct dentry *debugfs_root);
+int
Re: [Freedreno] [PATCH v1 7/8] drm/msm/dpu: simplify DPU's regset32 code
On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
Squash dpu_debugfs_setup_regset32() into dpu_debugfs_create_regset32().
it makes little sense to have separate function to just setup the
structure.
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 32 ---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 38 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 27 +---
3 files changed, 33 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 4c04982c71b2..7e7a619769a8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -182,6 +182,15 @@ static void dpu_debugfs_danger_init(struct dpu_kms
*dpu_kms,
}
+/*
+ * Companion structure for dpu_debugfs_create_regset32.
+ */
+struct dpu_debugfs_regset32 {
+ uint32_t offset;
+ uint32_t blk_len;
+ struct dpu_kms *dpu_kms;
+};
+
static int _dpu_debugfs_show_regset32(struct seq_file *s, void *data)
{
struct dpu_debugfs_regset32 *regset = s->private;
@@ -229,24 +238,23 @@ static const struct file_operations dpu_fops_regset32 = {
.release = single_release,
};
-void dpu_debugfs_setup_regset32(struct dpu_debugfs_regset32 *regset,
+void dpu_debugfs_create_regset32(const char *name, umode_t mode,
+ void *parent,
uint32_t offset, uint32_t length, struct dpu_kms *dpu_kms)
{
- if (regset) {
- regset->offset = offset;
- regset->blk_len = length;
- regset->dpu_kms = dpu_kms;
- }
-}
+ struct dpu_debugfs_regset32 *regset;
-void dpu_debugfs_create_regset32(const char *name, umode_t mode,
- void *parent, struct dpu_debugfs_regset32 *regset)
-{
- if (!name || !regset || !regset->dpu_kms || !regset->blk_len)
+ if (WARN_ON(!name || !dpu_kms || !length))
+ return;
+
+ regset = devm_kzalloc(_kms->pdev->dev, sizeof(*regset), GFP_KERNEL);
+ if (!regset)
return;
/* make sure offset is a multiple of 4 */
- regset->offset = round_down(regset->offset, 4);
+ regset->offset = round_down(offset, 4);
+ regset->blk_len = length;
+ regset->dpu_kms = dpu_kms;
debugfs_create_file(name, mode, parent, regset, _fops_regset32);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 775bcbda860f..b53cdeb1b5c4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -160,33 +160,9 @@ struct dpu_global_state
*
* Documentation/filesystems/debugfs.rst
*
- * @dpu_debugfs_setup_regset32: Initialize data for dpu_debugfs_create_regset32
* @dpu_debugfs_create_regset32: Create 32-bit register dump file
- * @dpu_debugfs_get_root: Get root dentry for DPU_KMS's debugfs node
*/
-/**
- * Companion structure for dpu_debugfs_create_regset32. Do not initialize the
- * members of this structure explicitly; use dpu_debugfs_setup_regset32
instead.
- */
-struct dpu_debugfs_regset32 {
- uint32_t offset;
- uint32_t blk_len;
- struct dpu_kms *dpu_kms;
-};
-
-/**
- * dpu_debugfs_setup_regset32 - Initialize register block definition for
debugfs
- * This function is meant to initialize dpu_debugfs_regset32 structures for use
- * with dpu_debugfs_create_regset32.
- * @regset: opaque register definition structure
- * @offset: sub-block offset
- * @length: sub-block length, in bytes
- * @dpu_kms: pointer to dpu kms structure
- */
-void dpu_debugfs_setup_regset32(struct dpu_debugfs_regset32 *regset,
- uint32_t offset, uint32_t length, struct dpu_kms *dpu_kms);
-
/**
* dpu_debugfs_create_regset32 - Create register read back file for debugfs
*
@@ -195,20 +171,16 @@ void dpu_debugfs_setup_regset32(struct
dpu_debugfs_regset32 *regset,
* names/offsets do not need to be provided. The 'read' function simply
outputs
* sequential register values over a specified range.
*
- * Similar to the related debugfs_create_regset32 API, the structure pointed to
- * by regset needs to persist for the lifetime of the created file. The calling
- * code is responsible for initialization/management of this structure.
- *
- * The structure pointed to by regset is meant to be opaque. Please use
- * dpu_debugfs_setup_regset32 to initialize it.
- *
* @name: File name within debugfs
* @mode: File mode within debugfs
* @parent: Parent directory entry within debugfs, can be NULL
- * @regset: Pointer to persistent register block definition
+ * @offset: sub-block offset
+ * @length: sub-block length, in bytes
+ * @dpu_kms: pointer to dpu kms structure
*/
void dpu_debugfs_create_regset32(const char *name, umode_t mode,
- void *parent, struct dpu_debugfs_regset32 *regset);
+
[Freedreno] [PATCH v7 2/2] drm/msm/dp: do not initialize phy until plugin interrupt received
Current DP drivers have regulators, clocks, irq and phy are grouped
together within a function and executed not in a symmetric manner.
This increase difficulty of code maintenance and limited code scalability.
This patch divides the driver life cycle of operation into four states,
resume (including booting up), dongle plugin, dongle unplugged and suspend.
Regulators, core clocks and irq are grouped together and enabled at resume
(or booting up) so that the DP controller is armed and ready to receive HPD
plugin interrupts. HPD plugin interrupt is generated when a dongle plugs
into DUT (device under test). Once HPD plugin interrupt is received, DP
controller will initialize phy so that dpcd read/write will function and
following link training can be proceeded successfully. DP phy will be
disabled after main link is teared down at end of unplugged HPD interrupt
handle triggered by dongle unplugged out of DUT. Finally regulators, code
clocks and irq are disabled at corresponding suspension.
Changes in V2:
-- removed unnecessary dp_ctrl NULL check
-- removed unnecessary phy init_count and power_count DRM_DEBUG_DP logs
-- remove flip parameter out of dp_ctrl_irq_enable()
-- add fixes tag
Changes in V3:
-- call dp_display_host_phy_init() instead of dp_ctrl_phy_init() at
dp_display_host_init() for eDP
Changes in V4:
-- rewording commit text to match this commit changes
Changes in V5:
-- rebase on top of msm-next branch
Changes in V6:
-- delete flip variable
Changes in V7:
-- dp_ctrl_irq_enable/disabe() merged into dp_ctrl_reset_irq_ctrl()
Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon
Chipsets")
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_ctrl.c| 77 --
drivers/gpu/drm/msm/dp/dp_ctrl.h| 8 ++--
drivers/gpu/drm/msm/dp/dp_display.c | 84 ++---
3 files changed, 92 insertions(+), 77 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index c724cb0..39558a2 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1365,60 +1365,44 @@ static int dp_ctrl_enable_stream_clocks(struct
dp_ctrl_private *ctrl)
return ret;
}
-int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
+void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
+{
+ struct dp_ctrl_private *ctrl;
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+ dp_catalog_ctrl_reset(ctrl->catalog);
+
+ if (enable)
+ dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
+}
+
+void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
{
struct dp_ctrl_private *ctrl;
struct dp_io *dp_io;
struct phy *phy;
- if (!dp_ctrl) {
- DRM_ERROR("Invalid input data\n");
- return -EINVAL;
- }
-
ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
dp_io = >parser->io;
phy = dp_io->phy;
- ctrl->dp_ctrl.orientation = flip;
-
- if (reset)
- dp_catalog_ctrl_reset(ctrl->catalog);
-
- DRM_DEBUG_DP("flip=%d\n", flip);
dp_catalog_ctrl_phy_reset(ctrl->catalog);
phy_init(phy);
- dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
-
- return 0;
}
-/**
- * dp_ctrl_host_deinit() - Uninitialize DP controller
- * @dp_ctrl: Display Port Driver data
- *
- * Perform required steps to uninitialize DP controller
- * and its resources.
- */
-void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
+void dp_ctrl_phy_exit(struct dp_ctrl *dp_ctrl)
{
struct dp_ctrl_private *ctrl;
struct dp_io *dp_io;
struct phy *phy;
- if (!dp_ctrl) {
- DRM_ERROR("Invalid input data\n");
- return;
- }
-
ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
dp_io = >parser->io;
phy = dp_io->phy;
- dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
+ dp_catalog_ctrl_phy_reset(ctrl->catalog);
phy_exit(phy);
-
- DRM_DEBUG_DP("Host deinitialized successfully\n");
}
static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
@@ -1893,8 +1877,14 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
return ret;
}
+ DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
+ (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
+
phy_power_off(phy);
+ DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
+ (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
+
/* aux channel down, reinit phy */
phy_exit(phy);
phy_init(phy);
@@ -1903,23 +1893,6 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
return ret;
}
-void dp_ctrl_off_phy(struct dp_ctrl *dp_ctrl)
-{
- struct dp_ctrl_private *ctrl;
- struct dp_io *dp_io;
- struct phy
[Freedreno] [PATCH v7 1/2] drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read failed
Add checking aux read/write status at both dp_link_parse_sink_count()
and dp_link_parse_sink_status_filed() to avoid long timeout delay if
dp aux read/write failed at timeout due to cable unplugged. Also make
sure dp controller had been initialized before start dpcd read and write.
Changes in V4:
-- split this patch as stand alone patch
Changes in v5:
-- rebase on msm-next branch
Changes in v6:
-- add more details commit text
Signed-off-by: Kuogee Hsieh
Reviewed-by: Stephen Boyd
Tested-by: Stephen Boyd
---
drivers/gpu/drm/msm/dp/dp_display.c | 12 +---
drivers/gpu/drm/msm/dp/dp_link.c| 19 ++-
2 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 3d61459..0766752 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -692,9 +692,15 @@ static int dp_irq_hpd_handle(struct dp_display_private
*dp, u32 data)
return 0;
}
- ret = dp_display_usbpd_attention_cb(>pdev->dev);
- if (ret == -ECONNRESET) { /* cable unplugged */
- dp->core_initialized = false;
+ /*
+* dp core (ahb/aux clks) must be initialized before
+* irq_hpd be handled
+*/
+ if (dp->core_initialized) {
+ ret = dp_display_usbpd_attention_cb(>pdev->dev);
+ if (ret == -ECONNRESET) { /* cable unplugged */
+ dp->core_initialized = false;
+ }
}
DRM_DEBUG_DP("hpd_state=%d\n", state);
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
index a5bdfc5..d4d31e5 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.c
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -737,18 +737,25 @@ static int dp_link_parse_sink_count(struct dp_link
*dp_link)
return 0;
}
-static void dp_link_parse_sink_status_field(struct dp_link_private *link)
+static int dp_link_parse_sink_status_field(struct dp_link_private *link)
{
int len = 0;
link->prev_sink_count = link->dp_link.sink_count;
- dp_link_parse_sink_count(>dp_link);
+ len = dp_link_parse_sink_count(>dp_link);
+ if (len < 0) {
+ DRM_ERROR("DP parse sink count failed\n");
+ return len;
+ }
len = drm_dp_dpcd_read_link_status(link->aux,
link->link_status);
- if (len < DP_LINK_STATUS_SIZE)
+ if (len < DP_LINK_STATUS_SIZE) {
DRM_ERROR("DP link status read failed\n");
- dp_link_parse_request(link);
+ return len;
+ }
+
+ return dp_link_parse_request(link);
}
/**
@@ -1023,7 +1030,9 @@ int dp_link_process_request(struct dp_link *dp_link)
dp_link_reset_data(link);
- dp_link_parse_sink_status_field(link);
+ ret = dp_link_parse_sink_status_field(link);
+ if (ret)
+ return ret;
if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [Freedreno] [PATCH v1 6/8] drm/msm/dpu: stop manually removing debugfs files for the DPU CRTC
On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
DRM code handles removing all debugfs recursively. Drop CRTC-specific
code to perform that.
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 ---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 ---
2 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index d290809d59bd..9899f7424131 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1424,15 +1424,16 @@ DEFINE_SHOW_ATTRIBUTE(dpu_crtc_debugfs_state);
static int _dpu_crtc_init_debugfs(struct drm_crtc *crtc)
{
struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
+ struct dentry *debugfs_root;
- dpu_crtc->debugfs_root = debugfs_create_dir(dpu_crtc->name,
+ debugfs_root = debugfs_create_dir(dpu_crtc->name,
crtc->dev->primary->debugfs_root);
debugfs_create_file("status", 0400,
- dpu_crtc->debugfs_root,
+ debugfs_root,
dpu_crtc, &_dpu_debugfs_status_fops);
debugfs_create_file("state", 0600,
- dpu_crtc->debugfs_root,
+ debugfs_root,
_crtc->base,
_crtc_debugfs_state_fops);
@@ -1450,13 +1451,6 @@ static int dpu_crtc_late_register(struct drm_crtc *crtc)
return _dpu_crtc_init_debugfs(crtc);
}
-static void dpu_crtc_early_unregister(struct drm_crtc *crtc)
-{
- struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
-
- debugfs_remove_recursive(dpu_crtc->debugfs_root);
-}
-
static const struct drm_crtc_funcs dpu_crtc_funcs = {
.set_config = drm_atomic_helper_set_config,
.destroy = dpu_crtc_destroy,
@@ -1465,7 +1459,6 @@ static const struct drm_crtc_funcs dpu_crtc_funcs = {
.atomic_duplicate_state = dpu_crtc_duplicate_state,
.atomic_destroy_state = dpu_crtc_destroy_state,
.late_register = dpu_crtc_late_register,
- .early_unregister = dpu_crtc_early_unregister,
.verify_crc_source = dpu_crtc_verify_crc_source,
.set_crc_source = dpu_crtc_set_crc_source,
.enable_vblank = msm_crtc_enable_vblank,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 4328e133d71c..b8785c394fcc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -129,7 +129,6 @@ struct dpu_crtc_frame_event {
* @drm_requested_vblank : Whether vblanks have been enabled in the encoder
* @property_info : Opaque structure for generic property support
* @property_defaults : Array of default values for generic property support
- * @debugfs_root : Parent of debugfs node
* @vblank_cb_count : count of vblank callback since last reset
* @play_count: frame count between crtc enable and disable
* @vblank_cb_time : ktime at vblank count reset
@@ -160,8 +159,6 @@ struct dpu_crtc {
struct drm_pending_vblank_event *event;
u32 vsync_count;
- struct dentry *debugfs_root;
-
u32 vblank_cb_count;
u64 play_count;
ktime_t vblank_cb_time;
Re: [Freedreno] [PATCH v1 5/8] drm/msm/dpu: stop manually removing debugfs files for the DPU plane
On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
DRM code handles removing all debugfs recursively. Drop plane-specific
code to perform that.
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 28 ---
1 file changed, 10 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index f80ee3ba9a8a..d3176f58708e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -110,7 +110,6 @@ struct dpu_plane {
struct dpu_mdss_cfg *catalog;
/* debugfs related stuff */
- struct dentry *debugfs_root;
struct dpu_debugfs_regset32 debugfs_src;
struct dpu_debugfs_regset32 debugfs_scaler;
struct dpu_debugfs_regset32 debugfs_csc;
@@ -1368,15 +1367,16 @@ static int _dpu_plane_init_debugfs(struct drm_plane
*plane)
struct dpu_kms *kms = _dpu_plane_get_kms(plane);
const struct dpu_sspp_cfg *cfg = pdpu->pipe_hw->cap;
const struct dpu_sspp_sub_blks *sblk = cfg->sblk;
+ struct dentry *debugfs_root;
/* create overall sub-directory for the pipe */
- pdpu->debugfs_root =
+ debugfs_root =
debugfs_create_dir(plane->name,
plane->dev->primary->debugfs_root);
/* don't error check these */
debugfs_create_xul("features", 0600,
- pdpu->debugfs_root, (unsigned long
*)>pipe_hw->cap->features);
+ debugfs_root, (unsigned long
*)>pipe_hw->cap->features);
/* add register dump support */
dpu_debugfs_setup_regset32(>debugfs_src,
@@ -1384,7 +1384,7 @@ static int _dpu_plane_init_debugfs(struct drm_plane
*plane)
sblk->src_blk.len,
kms);
dpu_debugfs_create_regset32("src_blk", 0400,
- pdpu->debugfs_root, >debugfs_src);
+ debugfs_root, >debugfs_src);
if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
@@ -1395,7 +1395,7 @@ static int _dpu_plane_init_debugfs(struct drm_plane
*plane)
sblk->scaler_blk.len,
kms);
dpu_debugfs_create_regset32("scaler_blk", 0400,
- pdpu->debugfs_root,
+ debugfs_root,
>debugfs_scaler);
}
@@ -1406,24 +1406,24 @@ static int _dpu_plane_init_debugfs(struct drm_plane *plane)
sblk->csc_blk.len,
kms);
dpu_debugfs_create_regset32("csc_blk", 0400,
- pdpu->debugfs_root, >debugfs_csc);
+ debugfs_root, >debugfs_csc);
}
debugfs_create_u32("xin_id",
0400,
- pdpu->debugfs_root,
+ debugfs_root,
(u32 *) >xin_id);
debugfs_create_u32("clk_ctrl",
0400,
- pdpu->debugfs_root,
+ debugfs_root,
(u32 *) >clk_ctrl);
debugfs_create_x32("creq_vblank",
0600,
- pdpu->debugfs_root,
+ debugfs_root,
(u32 *) >creq_vblank);
debugfs_create_x32("danger_vblank",
0600,
- pdpu->debugfs_root,
+ debugfs_root,
(u32 *) >danger_vblank);
return 0;
@@ -1440,13 +1440,6 @@ static int dpu_plane_late_register(struct drm_plane
*plane)
return _dpu_plane_init_debugfs(plane);
}
-static void dpu_plane_early_unregister(struct drm_plane *plane)
-{
- struct dpu_plane *pdpu = to_dpu_plane(plane);
-
- debugfs_remove_recursive(pdpu->debugfs_root);
-}
-
static bool dpu_plane_format_mod_supported(struct drm_plane *plane,
uint32_t format, uint64_t modifier)
{
@@ -1472,7 +1465,6 @@ static const struct drm_plane_funcs dpu_plane_funcs = {
.atomic_duplicate_state = dpu_plane_duplicate_state,
.atomic_destroy_state = dpu_plane_destroy_state,
.late_register = dpu_plane_late_register,
- .early_unregister = dpu_plane_early_unregister,
.format_mod_supported = dpu_plane_format_mod_supported,
};
Re: [Freedreno] [PATCH v1 4/8] drm/msm/dpu: drop plane's default_scaling debugfs file
On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
Proper support for the 'default_scaling' debugfs file was removed during
DPU driver pre-merge cleanup. Remove leftover file.
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 6ea4db061c9f..f80ee3ba9a8a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -114,7 +114,6 @@ struct dpu_plane {
struct dpu_debugfs_regset32 debugfs_src;
struct dpu_debugfs_regset32 debugfs_scaler;
struct dpu_debugfs_regset32 debugfs_csc;
- bool debugfs_default_scale;
};
static const uint64_t supported_format_modifiers[] = {
@@ -1398,10 +1397,6 @@ static int _dpu_plane_init_debugfs(struct drm_plane
*plane)
dpu_debugfs_create_regset32("scaler_blk", 0400,
pdpu->debugfs_root,
>debugfs_scaler);
- debugfs_create_bool("default_scaling",
- 0600,
- pdpu->debugfs_root,
- >debugfs_default_scale);
}
if (cfg->features & BIT(DPU_SSPP_CSC) ||
Re: [Freedreno] [PATCH v1 3/8] drm/msm/dpu: make danger_status/safe_status readable
On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote: Change \t to \n in the print format to stop putting all SSPP status in a single line. Splitting it to one SSPP per line is much more readable. Signed-off-by: Dmitry Baryshkov Reviewed-by: Abhinav Kumar --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index e7f0cded2c6b..4c04982c71b2 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -82,7 +82,7 @@ static int _dpu_danger_signal_status(struct seq_file *s, seq_printf(s, "MDP : 0x%x\n", status.mdp); for (i = SSPP_VIG0; i < SSPP_MAX; i++) - seq_printf(s, "SSPP%d : 0x%x \t", i - SSPP_VIG0, + seq_printf(s, "SSPP%d : 0x%x \n", i - SSPP_VIG0, status.sspp[i]); seq_puts(s, "\n");
Re: [Freedreno] [PATCH v1 2/8] drm/msm/dpu: fix safe status debugfs file
On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
Make safe_status debugfs fs file actually return safe status rather than
danger status data.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 259d438bc6e8..e7f0cded2c6b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -73,8 +73,8 @@ static int _dpu_danger_signal_status(struct seq_file *s,
);
} else {
seq_puts(s, "\nSafe signal status:\n");
- if (kms->hw_mdp->ops.get_danger_status)
- kms->hw_mdp->ops.get_danger_status(kms->hw_mdp,
+ if (kms->hw_mdp->ops.get_safe_status)
+ kms->hw_mdp->ops.get_safe_status(kms->hw_mdp,
);
}
pm_runtime_put_sync(>pdev->dev);
Re: [Freedreno] [PATCH v1 1/8] drm/msm/dpu: move disable_danger out of plane subdir
On 12/1/2021 2:26 PM, Dmitry Baryshkov wrote:
The disable_danger debugfs file is not related to a single plane.
Instead it is used by all registered planes. Move it from plane subtree
to the global subtree next to danger_status and safe_status files,
so that the new file supplements them.
the danger_safe itself is a per-plane feature as each SSPP can be
controlled individually.
In todays code, yes we do it for all the active planes together.
Since this is the same behavior
Reviewed-by: Abhinav Kumar
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 70 +
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 74 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 6 ++
3 files changed, 77 insertions(+), 73 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 6c457c419412..259d438bc6e8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -101,6 +101,73 @@ static int dpu_debugfs_safe_stats_show(struct seq_file *s,
void *v)
}
DEFINE_SHOW_ATTRIBUTE(dpu_debugfs_safe_stats);
+static ssize_t _dpu_plane_danger_read(struct file *file,
+ char __user *buff, size_t count, loff_t *ppos)
+{
+ struct dpu_kms *kms = file->private_data;
+ int len;
+ char buf[40];
+
+ len = scnprintf(buf, sizeof(buf), "%d\n", !kms->has_danger_ctrl);
+
+ return simple_read_from_buffer(buff, count, ppos, buf, len);
+}
+
+static void _dpu_plane_set_danger_state(struct dpu_kms *kms, bool enable)
+{
+ struct drm_plane *plane;
+
+ drm_for_each_plane(plane, kms->dev) {
+ if (plane->fb && plane->state) {
+ dpu_plane_danger_signal_ctrl(plane, enable);
+ DPU_DEBUG("plane:%d img:%dx%d ",
+ plane->base.id, plane->fb->width,
+ plane->fb->height);
+ DPU_DEBUG("src[%d,%d,%d,%d] dst[%d,%d,%d,%d]\n",
+ plane->state->src_x >> 16,
+ plane->state->src_y >> 16,
+ plane->state->src_w >> 16,
+ plane->state->src_h >> 16,
+ plane->state->crtc_x, plane->state->crtc_y,
+ plane->state->crtc_w, plane->state->crtc_h);
+ } else {
+ DPU_DEBUG("Inactive plane:%d\n", plane->base.id);
+ }
+ }
+}
+
+static ssize_t _dpu_plane_danger_write(struct file *file,
+ const char __user *user_buf, size_t count, loff_t *ppos)
+{
+ struct dpu_kms *kms = file->private_data;
+ int disable_panic;
+ int ret;
+
+ ret = kstrtouint_from_user(user_buf, count, 0, _panic);
+ if (ret)
+ return ret;
+
+ if (disable_panic) {
+ /* Disable panic signal for all active pipes */
+ DPU_DEBUG("Disabling danger:\n");
+ _dpu_plane_set_danger_state(kms, false);
+ kms->has_danger_ctrl = false;
+ } else {
+ /* Enable panic signal for all active pipes */
+ DPU_DEBUG("Enabling danger:\n");
+ kms->has_danger_ctrl = true;
+ _dpu_plane_set_danger_state(kms, true);
+ }
+
+ return count;
+}
+
+static const struct file_operations dpu_plane_danger_enable = {
+ .open = simple_open,
+ .read = _dpu_plane_danger_read,
+ .write = _dpu_plane_danger_write,
+};
+
static void dpu_debugfs_danger_init(struct dpu_kms *dpu_kms,
struct dentry *parent)
{
@@ -110,6 +177,9 @@ static void dpu_debugfs_danger_init(struct dpu_kms *dpu_kms,
dpu_kms, _debugfs_danger_stats_fops);
debugfs_create_file("safe_status", 0600, entry,
dpu_kms, _debugfs_safe_stats_fops);
+ debugfs_create_file("disable_danger", 0600, entry,
+ dpu_kms, _plane_danger_enable);
+
}
static int _dpu_debugfs_show_regset32(struct seq_file *s, void *data)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index ca190d92f0d5..6ea4db061c9f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1350,7 +1350,7 @@ static void dpu_plane_reset(struct drm_plane *plane)
}
#ifdef CONFIG_DEBUG_FS
-static void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
+void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
{
struct dpu_plane *pdpu = to_dpu_plane(plane);
struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
@@ -1363,73 +1363,6 @@ static void dpu_plane_danger_signal_ctrl(struct
drm_plane *plane, bool enable)
pm_runtime_put_sync(_kms->pdev->dev);
}
-static ssize_t
[Freedreno] [PATCH] drm/msm/a6xx: Skip crashdumper state if GPU needs_hw_init
From: Rob Clark
I am seeing some crash logs which imply that we are trying to use
crashdumper hw to read back GPU state when the GPU isn't initialized.
This doesn't go well (for example, GPU could be in 32b address mode
and ignoring the upper bits of buffer that it is trying to dump state
to).
I'm not *quite* sure how we get into this state in the first place,
but lets not make a bad situation worse by triggering iova fault
crashes.
While we're at it, also add the information about whether the GPU is
initialized to the devcore dump to make this easier to see in the
logs (which makes the WARN_ON() redundant and even harmful because
it fills up the small bit of dmesg we get with the crash report).
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 9 -
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 1 -
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
index bdd0059a81ff..55f443328d8e 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
@@ -49,6 +49,8 @@ struct a6xx_gpu_state {
s32 hfi_queue_history[2][HFI_HISTORY_SZ];
struct list_head objs;
+
+ bool gpu_initialized;
};
static inline int CRASHDUMP_WRITE(u64 *in, u32 reg, u32 val)
@@ -1001,7 +1003,8 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu
*gpu)
* write out GPU state, so we need to skip this when the SMMU is
* stalled in response to an iova fault
*/
- if (!stalled && !a6xx_crashdumper_init(gpu, &_dumper)) {
+ if (!stalled && !gpu->needs_hw_init &&
+ !a6xx_crashdumper_init(gpu, &_dumper)) {
dumper = &_dumper;
}
@@ -1018,6 +1021,8 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu
*gpu)
if (snapshot_debugbus)
a6xx_get_debugbus(gpu, a6xx_state);
+ a6xx_state->gpu_initialized = !gpu->needs_hw_init;
+
return _state->base;
}
@@ -1246,6 +1251,8 @@ void a6xx_show(struct msm_gpu *gpu, struct msm_gpu_state
*state,
if (IS_ERR_OR_NULL(state))
return;
+ drm_printf(p, "gpu-initialized: %d\n", a6xx_state->gpu_initialized);
+
adreno_show(gpu, state, p);
drm_puts(p, "gmu-log:\n");
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 47cb40bdbd43..f33cfa4ef1c8 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -504,7 +504,6 @@ int adreno_gpu_state_get(struct msm_gpu *gpu, struct
msm_gpu_state *state)
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
int i, count = 0;
- WARN_ON(gpu->needs_hw_init);
WARN_ON(!mutex_is_locked(>lock));
kref_init(>ref);
--
2.33.1
[Freedreno] [PATCH v7] phy: qcom-qmp: add display port v4 voltage and pre-emphasis swing tables
From: Kuogee Hsieh
The previous patch from Fixes (aff188feb5e1) added functions to support V4
phy. But it did not update voltage and pre-emphasis tables accordingly.
This patch add v4 voltage and pre-emphasis swing tables to complete v4
phy implementation. Both voltage and pre-emphasis swing level are set
during link training negotiation between host and sink. There are totally
four tables added. A voltage swing table for both hbr and hbr1, a voltage
table for both hbr2 and hbr3, a pre-emphasis table for both hbr and hbr1
and a pre-emphasis table for both hbr2 and hbr3. In addition, write 0x0a
to TX_TX_POL_INV is added to complete the sequence of configure dp phy
base on HPG.
Fixes: aff188feb5e1 ("phy: qcom-qmp: add support for sm8250-usb3-dp phy")
Signed-off-by: Kuogee Hsieh
Reviewed-by: Stephen Boyd
Reviewed-by: Bjorn Andersson
---
drivers/phy/qualcomm/phy-qcom-qmp.c | 112 +---
1 file changed, 77 insertions(+), 35 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 456a59d..d41e30c 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -4255,40 +4255,50 @@ static void qcom_qmp_v3_phy_dp_aux_init(struct qmp_phy
*qphy)
qphy->pcs + QSERDES_V3_DP_PHY_AUX_INTERRUPT_MASK);
}
-static const u8 qmp_dp_v3_pre_emphasis_hbr3_hbr2[4][4] = {
+#define MAX_SWING_LEVEL 4
+#define MAX_VOLTAGE_LEVEL 4
+#define MAX_EMPHASIS_LEVEL 4
+
+static const u8
qmp_dp_v3_pre_emphasis_hbr3_hbr2[MAX_SWING_LEVEL][MAX_EMPHASIS_LEVEL] = {
{ 0x00, 0x0c, 0x15, 0x1a },
{ 0x02, 0x0e, 0x16, 0xff },
{ 0x02, 0x11, 0xff, 0xff },
{ 0x04, 0xff, 0xff, 0xff }
};
-static const u8 qmp_dp_v3_voltage_swing_hbr3_hbr2[4][4] = {
+static const u8
qmp_dp_v3_voltage_swing_hbr3_hbr2[MAX_SWING_LEVEL][MAX_VOLTAGE_LEVEL] = {
{ 0x02, 0x12, 0x16, 0x1a },
{ 0x09, 0x19, 0x1f, 0xff },
{ 0x10, 0x1f, 0xff, 0xff },
{ 0x1f, 0xff, 0xff, 0xff }
};
-static const u8 qmp_dp_v3_pre_emphasis_hbr_rbr[4][4] = {
+static const u8
qmp_dp_v3_pre_emphasis_hbr_rbr[MAX_SWING_LEVEL][MAX_EMPHASIS_LEVEL] = {
{ 0x00, 0x0c, 0x14, 0x19 },
{ 0x00, 0x0b, 0x12, 0xff },
{ 0x00, 0x0b, 0xff, 0xff },
{ 0x04, 0xff, 0xff, 0xff }
};
-static const u8 qmp_dp_v3_voltage_swing_hbr_rbr[4][4] = {
+static const u8
qmp_dp_v3_voltage_swing_hbr_rbr[MAX_SWING_LEVEL][MAX_VOLTAGE_LEVEL] = {
{ 0x08, 0x0f, 0x16, 0x1f },
{ 0x11, 0x1e, 0x1f, 0xff },
{ 0x19, 0x1f, 0xff, 0xff },
{ 0x1f, 0xff, 0xff, 0xff }
};
-static int qcom_qmp_phy_configure_dp_swing(struct qmp_phy *qphy,
- unsigned int drv_lvl_reg, unsigned int emp_post_reg)
+static int __qcom_qmp_phy_configure_dp_swing
+ (struct qmp_phy *qphy,
+ unsigned int drv_lvl_reg,
+ unsigned int emp_post_reg,
+ const u8
voltage_swing_hbr_rbr[MAX_SWING_LEVEL][MAX_VOLTAGE_LEVEL],
+ const u8
pre_emphasis_hbr_rbr[MAX_SWING_LEVEL][MAX_EMPHASIS_LEVEL],
+ const u8
voltage_swing_hbr3_hbr2[MAX_SWING_LEVEL][MAX_VOLTAGE_LEVEL],
+ const u8
pre_emphasis_hbr3_hbr2[MAX_SWING_LEVEL][MAX_EMPHASIS_LEVEL])
{
const struct phy_configure_opts_dp *dp_opts = >dp_opts;
unsigned int v_level = 0, p_level = 0;
- u8 voltage_swing_cfg, pre_emphasis_cfg;
+ u8 voltage, emphasis;
int i;
for (i = 0; i < dp_opts->lanes; i++) {
@@ -4297,26 +4307,25 @@ static int qcom_qmp_phy_configure_dp_swing(struct
qmp_phy *qphy,
}
if (dp_opts->link_rate <= 2700) {
- voltage_swing_cfg =
qmp_dp_v3_voltage_swing_hbr_rbr[v_level][p_level];
- pre_emphasis_cfg =
qmp_dp_v3_pre_emphasis_hbr_rbr[v_level][p_level];
+ voltage = voltage_swing_hbr_rbr[v_level][p_level];
+ emphasis = pre_emphasis_hbr_rbr[v_level][p_level];
} else {
- voltage_swing_cfg =
qmp_dp_v3_voltage_swing_hbr3_hbr2[v_level][p_level];
- pre_emphasis_cfg =
qmp_dp_v3_pre_emphasis_hbr3_hbr2[v_level][p_level];
+ voltage = voltage_swing_hbr3_hbr2[v_level][p_level];
+ emphasis = pre_emphasis_hbr3_hbr2[v_level][p_level];
}
/* TODO: Move check to config check */
- if (voltage_swing_cfg == 0xFF && pre_emphasis_cfg == 0xFF)
+ if (voltage == 0xFF && emphasis == 0xFF)
return -EINVAL;
/* Enable MUX to use Cursor values from these registers */
- voltage_swing_cfg |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
- pre_emphasis_cfg |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;
-
- writel(voltage_swing_cfg, qphy->tx + drv_lvl_reg);
- writel(pre_emphasis_cfg, qphy->tx + emp_post_reg);
- writel(voltage_swing_cfg, qphy->tx2 + drv_lvl_reg);
- writel(pre_emphasis_cfg, qphy->tx2
Re: [Freedreno] [PATCH 1/2] drm/ bridge: tc358762: move bridge init to enable callback
Hi Dmitry,
(CC'ing Tomi)
On Thu, Dec 09, 2021 at 07:53:43PM +0300, Dmitry Baryshkov wrote:
> On Fri, 26 Nov 2021 at 14:56, Dave Stevenson wrote:
> > On Fri, 26 Nov 2021 at 00:32, Dmitry Baryshkov wrote:
> > >
> > > During the pre_enable time the previous bridge (e.g. DSI host) might be
> > > not initialized yet. Move the regulator enablement and bridge init to
> > > te enable callback (and consequently regulator disblement to disable).
> >
> > Except that in the enable callback the DSI host has video enabled too,
> > so the data lanes may be in HS mode too, and the bridge may not be
> > prepared to accept that during power on / initialisation. That means
> > you've got a race condition over how quickly the composition hardware
> > starts producing pixel data vs when this enable callback is called. I
> > suspect that is why we had [1] for the rare case when the race
> > condition failed.
> > There's also seems to be no guarantee that a host can do LP commands
> > between HS video packets (eg sunxi [2])
>
> I see.
>
> > This is the same issue that was being hacked around in [3], and is one
> > of the questions I'd raised back in July [4].
> > The DSI support is broken when it comes to accommodating
> > initialisation sequences, but in trying to ensure all possible
> > sequences can be accomodated, all currently proposed solutions have
> > been shot down.
> > Some platforms have worked around it by powering up the DSI host in
> > mode_set (dw-mipi-dsi),
>
> Looks like this approach is supported by panel-bridge, so I have
> proposed a similar change to the msm dsi driver. [5]
>
> > others have broken the bridge chain apart so
> > their pre_enable gets called first (exynos and currently vc4) except
> > that approach is broken for the atomic API.
> >
> > There is a need for some form of resolution, even if it is only
> > documenting the correct hack to implement in the DSI host driver.
> > Hacking bridge or panel drivers to try and workaround it seems wrong.
>
> Thank you for this lengthy explanation, background and pointers. This
> helped a lot.
>
> It really looks like we need a separate callback between pre-enable
> and enable (or before pre-enable, but that would break the 'clocks not
> enabled' constraint.
>
> Another option would be to move video mode enablement from bridge ops
> to dsi host interface, making the panel/bridge implicitly tell the
> host to switch from LP to HS mode.
Do you mean explicitly ?
> This way the bridge's enable() callback would start with the DSI host
> powered up, but not sending the video, so the DSI device can send
> setup commands. Then it'd enable the video/cmd mode on the DSI host
> and then make final adjustments (like enabling the backlight/etc, so
> that the video is visible/sent to the next item in a chain. WDYT?
This is how the omapdrm driver originally implemented support for
bridges (as custom omapdrm components, before it was ported to
drm_bridge), with an that allowed downstream components to explicitly
control the enable/disable sequence of upstream components. I'm
beginning to think it's a better model, but switching drm_bridge to that
is likely an impossible task. Keeping it specific to the DSI interface
could be a good middle-ground, although some DSI encoders may need to
propagate any control operation they receive from the downstream
component up to their upstream component (for instance instructing the
CRTC to enable video when the panel requests video to be enabled).
Still, it's probably worth being investigated.
I can't agree more with Dave about the need for documentation, DSI
drivers (both on the TX and RX side) are very creative these days,
causing lots of interoperability issues. This wild west situation really
needs some policing.
If anyone documents rules that can be enforced, I'll be very happy to
buy them a few rounds of drinks in any conference we'll happen to both
attend (even more so because that will mean travel restrictions will be
lifted :-)).
> > [1]
> > https://lists.freedesktop.org/archives/dri-devel/2021-September/322119.html
> > [2]
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#n776
> > [3]
> > https://lists.freedesktop.org/archives/dri-devel/2021-November/332003.html
> > [4] https://lists.freedesktop.org/archives/dri-devel/2021-July/313576.html
>
> [5] https://patchwork.freedesktop.org/patch/465764/?series=97688=1
>
> > > Signed-off-by: Dmitry Baryshkov
> > > ---
> > > drivers/gpu/drm/bridge/tc358762.c | 20 ++--
> > > 1 file changed, 10 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/tc358762.c
> > > b/drivers/gpu/drm/bridge/tc358762.c
> > > index 7104828024fd..ebdf771a1e49 100644
> > > --- a/drivers/gpu/drm/bridge/tc358762.c
> > > +++ b/drivers/gpu/drm/bridge/tc358762.c
> > > @@ -64,7 +64,7 @@ struct tc358762 {
> > > struct drm_connector connector;
> > > struct regulator
Re: [Freedreno] [PATCH v2 2/2] drm/msm/dpu: Fix timeout issues on command mode panels
Il 02/10/21 00:33, Dmitry Baryshkov ha scritto: On 11/09/2021 19:39, AngeloGioacchino Del Regno wrote: In function dpu_encoder_phys_cmd_wait_for_commit_done we are always checking if the relative CTL is started by waiting for an interrupt to fire: it is fine to do that, but then sometimes we call this function while the CTL is up and has never been put down, but that interrupt gets raised only when the CTL gets a state change from 0 to 1 (disabled to enabled), so we're going to wait for something that will never happen on its own. Solving this while avoiding to restart the CTL is actually possible and can be done by just checking if it is already up and running when the wait_for_commit_done function is called: in this case, so, if the CTL was already running, we can say that the commit is done if the command transmission is complete (in other terms, if the interface has been flushed). I've compared this with the MDP5 driver, where we always wait for PP_DONE interrupt. Would it be enough to always wait for it (= always call dpu_encoder_phys_cmd_wait_for_tx_complete())? This sets my delay record to reply to two months. Great achievement! Jokes apart, yes it would make sense to do that, it's something that works at least... but we should verify that such a thing doesn't break new platforms (like sm8150 and newer).
Re: [Freedreno] [PATCH 1/2] drm/ bridge: tc358762: move bridge init to enable callback
Hi Dave,
On Fri, 26 Nov 2021 at 14:56, Dave Stevenson
wrote:
>
> Hi Dmitry
>
> On Fri, 26 Nov 2021 at 00:32, Dmitry Baryshkov
> wrote:
> >
> > During the pre_enable time the previous bridge (e.g. DSI host) might be
> > not initialized yet. Move the regulator enablement and bridge init to
> > te enable callback (and consequently regulator disblement to disable).
>
> Except that in the enable callback the DSI host has video enabled too,
> so the data lanes may be in HS mode too, and the bridge may not be
> prepared to accept that during power on / initialisation. That means
> you've got a race condition over how quickly the composition hardware
> starts producing pixel data vs when this enable callback is called. I
> suspect that is why we had [1] for the rare case when the race
> condition failed.
> There's also seems to be no guarantee that a host can do LP commands
> between HS video packets (eg sunxi [2])
I see.
>
> This is the same issue that was being hacked around in [3], and is one
> of the questions I'd raised back in July [4].
> The DSI support is broken when it comes to accommodating
> initialisation sequences, but in trying to ensure all possible
> sequences can be accomodated, all currently proposed solutions have
> been shot down.
> Some platforms have worked around it by powering up the DSI host in
> mode_set (dw-mipi-dsi),
Looks like this approach is supported by panel-bridge, so I have
proposed a similar change to the msm dsi driver. [5]
> others have broken the bridge chain apart so
> their pre_enable gets called first (exynos and currently vc4) except
> that approach is broken for the atomic API.
>
> There is a need for some form of resolution, even if it is only
> documenting the correct hack to implement in the DSI host driver.
> Hacking bridge or panel drivers to try and workaround it seems wrong.
Thank you for this lengthy explanation, background and pointers. This
helped a lot.
It really looks like we need a separate callback between pre-enable
and enable (or before pre-enable, but that would break the 'clocks not
enabled' constraint.
Another option would be to move video mode enablement from bridge ops
to dsi host interface, making the panel/bridge implicitly tell the
host to switch from LP to HS mode.
This way the bridge's enable() callback would start with the DSI host
powered up, but not sending the video, so the DSI device can send
setup commands. Then it'd enable the video/cmd mode on the DSI host
and then make final adjustments (like enabling the backlight/etc, so
that the video is visible/sent to the next item in a chain. WDYT?
> [1]
> https://lists.freedesktop.org/archives/dri-devel/2021-September/322119.html
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#n776
> [3] https://lists.freedesktop.org/archives/dri-devel/2021-November/332003.html
> [4] https://lists.freedesktop.org/archives/dri-devel/2021-July/313576.html
[5] https://patchwork.freedesktop.org/patch/465764/?series=97688=1
>
> > Signed-off-by: Dmitry Baryshkov
> > ---
> > drivers/gpu/drm/bridge/tc358762.c | 20 ++--
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358762.c
> > b/drivers/gpu/drm/bridge/tc358762.c
> > index 7104828024fd..ebdf771a1e49 100644
> > --- a/drivers/gpu/drm/bridge/tc358762.c
> > +++ b/drivers/gpu/drm/bridge/tc358762.c
> > @@ -64,7 +64,7 @@ struct tc358762 {
> > struct drm_connector connector;
> > struct regulator *regulator;
> > struct drm_bridge *panel_bridge;
> > - bool pre_enabled;
> > + bool enabled;
> > int error;
> > };
> >
> > @@ -125,26 +125,26 @@ static int tc358762_init(struct tc358762 *ctx)
> > return tc358762_clear_error(ctx);
> > }
> >
> > -static void tc358762_post_disable(struct drm_bridge *bridge)
> > +static void tc358762_disable(struct drm_bridge *bridge)
> > {
> > struct tc358762 *ctx = bridge_to_tc358762(bridge);
> > int ret;
> >
> > /*
> > -* The post_disable hook might be called multiple times.
> > +* The disable hook might be called multiple times.
> > * We want to avoid regulator imbalance below.
> > */
> > - if (!ctx->pre_enabled)
> > + if (!ctx->enabled)
> > return;
> >
> > - ctx->pre_enabled = false;
> > + ctx->enabled = false;
> >
> > ret = regulator_disable(ctx->regulator);
> > if (ret < 0)
> > dev_err(ctx->dev, "error disabling regulators (%d)\n", ret);
> > }
> >
> > -static void tc358762_pre_enable(struct drm_bridge *bridge)
> > +static void tc358762_enable(struct drm_bridge *bridge)
> > {
> > struct tc358762 *ctx = bridge_to_tc358762(bridge);
> > int ret;
> > @@ -157,7 +157,7 @@ static void tc358762_pre_enable(struct drm_bridge
> > *bridge)
> > if (ret < 0)
> > dev_err(ctx->dev,
Re: [Freedreno] [PATCH v6] phy: qcom-qmp: add display port v4 voltage and pre-emphasis swing tables
On 08-12-21, 13:22, Kuogee Hsieh wrote:
> From: Kuogee Hsieh
>
> "add support for sm8250-usb3-dp phy" patch added functions to support V4
^^
why this leading quote here?
> phy. But it did not update voltage and pre-emphasis tables accordingly.
> This patch add v4 voltage and pre-emphasis swing tables to complete v4
> phy implementation. Both voltage and pre-emphasis swing level are set
> during link training negotiation between host and sink. There are totally
> four tables added. A voltage swing table for both hbr and hbr1, a voltage
> table for both hbr2 and hbr3, a pre-emphasis table for both hbr and hbr1
> and a pre-emphasis table for both hbr2 and hbr3. In addition, write 0x0a
> to TX_TX_POL_INV is added to complete the sequence of configure dp phy
> base on HPG.
>
> Chnages in v2:
There is a typo :), as Bjorn said please drop this from non drm code, we
dont need changelog in phy patches
> -- revise commit test
> -- add Fixes tag
> -- replaced voltage_swing_cfg with voltage
> -- replaced pre_emphasis_cfg with emphasis
> -- delete drv_lvl_reg and emp_post_reg parameters from
> qcom_qmp_v4_phy_configure_dp_swing()
> -- delete drv_lvl_reg and emp_post_reg parameters from
> qcom_qmp_phy_configure_dp_swing()
>
> Changes in V3:
> -- add __qcom_qmp_phy_configure_dp_swing() to commit swing/pre-emphasis level
>
> Changes in V4:
> -- pass 2D array to __qcom_qmp_phy_configure_dp_swing()
>
> Changes in V5:
> -- rebase on msm-next
>
> Changes in V6:
> -- change commit text title
> -- re wording commit text
>
> Fixes: aff188feb5e1 ("phy: qcom-qmp: add support for sm8250-usb3-dp phy")
> Signed-off-by: Kuogee Hsieh
> Reviewed-by: Stephen Boyd
> ---
> drivers/phy/qualcomm/phy-qcom-qmp.c | 97
> +
> 1 file changed, 66 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
> b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 456a59d..1f3585d 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -4283,12 +4283,17 @@ static const u8 qmp_dp_v3_voltage_swing_hbr_rbr[4][4]
> = {
> { 0x1f, 0xff, 0xff, 0xff }
> };
>
> -static int qcom_qmp_phy_configure_dp_swing(struct qmp_phy *qphy,
> - unsigned int drv_lvl_reg, unsigned int emp_post_reg)
> +static int __qcom_qmp_phy_configure_dp_swing(struct qmp_phy *qphy,
> + unsigned int drv_lvl_reg,
> + unsigned int emp_post_reg,
> + const u8 voltage_swing_hbr_rbr[4][4],
> + const u8 pre_emphasis_hbr_rbr[4][4],
> + const u8 voltage_swing_hbr3_hbr2[4][4],
> + const u8 pre_emphasis_hbr3_hbr2[4][4])
Pls align these to opening brances (hint checkpatch with --strict should
give you a warning)
Second I see that we are hardcoding the 4 here and all over the driver.
I guess it would make sense to define it and use it here and
everywhere..?
> {
> const struct phy_configure_opts_dp *dp_opts = >dp_opts;
> unsigned int v_level = 0, p_level = 0;
> - u8 voltage_swing_cfg, pre_emphasis_cfg;
> + u8 voltage, emphasis;
> int i;
>
> for (i = 0; i < dp_opts->lanes; i++) {
> @@ -4297,26 +4302,25 @@ static int qcom_qmp_phy_configure_dp_swing(struct
> qmp_phy *qphy,
> }
>
> if (dp_opts->link_rate <= 2700) {
> - voltage_swing_cfg =
> qmp_dp_v3_voltage_swing_hbr_rbr[v_level][p_level];
> - pre_emphasis_cfg =
> qmp_dp_v3_pre_emphasis_hbr_rbr[v_level][p_level];
> + voltage = voltage_swing_hbr_rbr[v_level][p_level];
> + emphasis = pre_emphasis_hbr_rbr[v_level][p_level];
> } else {
> - voltage_swing_cfg =
> qmp_dp_v3_voltage_swing_hbr3_hbr2[v_level][p_level];
> - pre_emphasis_cfg =
> qmp_dp_v3_pre_emphasis_hbr3_hbr2[v_level][p_level];
> + voltage = voltage_swing_hbr3_hbr2[v_level][p_level];
> + emphasis = pre_emphasis_hbr3_hbr2[v_level][p_level];
> }
>
> /* TODO: Move check to config check */
> - if (voltage_swing_cfg == 0xFF && pre_emphasis_cfg == 0xFF)
> + if (voltage == 0xFF && emphasis == 0xFF)
> return -EINVAL;
>
> /* Enable MUX to use Cursor values from these registers */
> - voltage_swing_cfg |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
> - pre_emphasis_cfg |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;
> -
> - writel(voltage_swing_cfg, qphy->tx + drv_lvl_reg);
> - writel(pre_emphasis_cfg, qphy->tx + emp_post_reg);
> - writel(voltage_swing_cfg, qphy->tx2 + drv_lvl_reg);
> - writel(pre_emphasis_cfg, qphy->tx2 + emp_post_reg);
> + voltage |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
> + emphasis |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;
>
> + writel(voltage, qphy->tx + drv_lvl_reg);
> + writel(emphasis, qphy->tx + emp_post_reg);
> + writel(voltage, qphy->tx2 + drv_lvl_reg);
> +
Re: [Freedreno] [PATCH] drm/msm/dp: Move debugfs files into subdirectory
On 2021-10-18 11:36, Bjorn Andersson wrote:
On Mon 18 Oct 11:07 PDT 2021, abhin...@codeaurora.org wrote:
Hi Bjorn
On 2021-10-17 09:42, Bjorn Andersson wrote:
> On Fri 15 Oct 16:53 PDT 2021, abhin...@codeaurora.org wrote:
>
> > On 2021-10-15 16:17, Bjorn Andersson wrote:
> > > In the cleanup path of the MSM DP driver the DP driver's debugfs files
> > > are destroyed by invoking debugfs_remove_recursive() on debug->root,
> > > which during initialization has been set to minor->debugfs_root.
> > >
> > > To allow cleaning up the DP driver's debugfs files either each dentry
> > > needs to be kept track of or the files needs to be put in a subdirectory
> > > which can be removed in one go.
> > >
> > > By choosing to put the debugfs files in a subdirectory, based on the
> > > name of the associated connector this also solves the problem that these
> > > names would collide as support for multiple DP instances are introduced.
> > >
> > > One alternative solution to the problem with colliding file names would
> > > have been to put keep track of the individual files and put them under
> > > the connector's debugfs directory. But while the drm_connector has been
> > > allocated, its associated debugfs directory has not been created at the
> > > time of initialization of the dp_debug.
> > >
> > > Signed-off-by: Bjorn Andersson
> >
> > I have been thinking about this problem ever since multi-DP has been
> > posted
> > :)
> > Creating sub-directories seems right but at the moment it looks like
> > IGT
> > which
> > uses these debugfs nodes doesnt check sub-directories:
> >
> >
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tools/msm_dp_compliance.c#L215
> >
> > It looks for the DP debugfs nodes under /sys/kernel/debug/dri/*/
> >
> > We have to fix IGT too to be able to handle multi-DP cases. I will
> > try to
> > come up
> > with a proposal to address this.
> >
> > Till then, can we go with the other solution to keep track of the
> > dentries?
> >
>
> I'm afraid I don't see what you're proposing.
>
> Afaict we need one set of dp_test{type,active,data} per DP controller,
> so even doing this by keeping track of the dentries requires that we
> rename the files based on some identifier (id or connector name) - which
> will cause igt to break.
Yes, I also thought the same that there needs to be some identifier.
"To allow cleaning up the DP driver's debugfs files either each dentry
needs to be kept track of or the files needs to be put in a
subdirectory
which can be removed in one go"
I guess I misunderstood your statement in the commit text thinking
that you
had some other way to keep track of the dentries as it mentioned that
use a subdirectory OR keep track of each dentry.
No, I did write that code as well and then ditched it.
Unfortunately I don't think it would help you, because we still need to
add some identifier to the file names and preferably we should add that
to the single case as well to make things consistent.
>
> As such, I think the practical path forward is that we merge the
> multi-DP series as currently proposed. This will not cause any issues on
> single-DP systems, but on multi-DP systems we will have warnings about
> duplicate debugfs entries in the kernel logs.
>
> Then you can figure out how to rework igt to deal with the multiple DP
> instances and update the dp_debug interface accordingly.
>
Fine with me, I will take care of this.
Cool, thanks.
Regards,
Bjorn
Following up on this, Rob has posted the igt change today which i acked.
https://patchwork.freedesktop.org/patch/465930/
With this in place, we can actually go ahead with this change.
Hence,
Reviewed-by: Abhinav Kumar
>
> Which also implies that we should hold this patch back. But if we go
> that path, I think we should fix dp_debug_deinit() so that it doesn't
> remove /sys/kernel/debug/dri/128 when the DP driver is unloaded.
Yes, lets hold this patch back till I fix multi-DP for IGT.
>
> Regards,
> Bjorn
>
> > > ---
> > >
> > > This depends on
> > >
https://lore.kernel.org/linux-arm-msm/20211010030435.4000642-1-bjorn.anders...@linaro.org/
> > > reducing the connector from a double pointer.
> > >
> > > drivers/gpu/drm/msm/dp/dp_debug.c | 15 +--
> > > 1 file changed, 9 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
> > > b/drivers/gpu/drm/msm/dp/dp_debug.c
> > > index da4323556ef3..67da4c69eca1 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_debug.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
> > > @@ -210,26 +210,29 @@ static const struct file_operations
> > > test_active_fops = {
> > > static int dp_debug_init(struct dp_debug *dp_debug, struct drm_minor
> > > *minor)
> > > {
> > > int rc = 0;
> > > +char path[64];
> > > struct dp_debug_private *debug = container_of(dp_debug,
> > > struct dp_debug_private, dp_debug);
> > >
> > > -debugfs_create_file("dp_debug", 0444,
Re: [Freedreno] [PATCH v6] drm/msm/dp: do not initialize phy until plugin interrupt received
Quoting Kuogee Hsieh (2021-12-08 12:36:24)
> Current DP drivers have regulators, clocks, irq and phy are grouped
> together within a function and executed not in a symmetric manner.
> This increase difficulty of code maintenance and limited code scalability.
> This patch divided the driver life cycle of operation into four states,
s/divided/divides/
> resume (including booting up), dongle plugin, dongle unplugged and suspend.
> Regulators, core clocks and irq are grouped together and enabled at resume
> (or booting up) so that the DP controller is armed and ready to receive HPD
> plugin interrupts. HPD plugin interrupt is generated when a dongle plugs
> into DUT (device under test). Once HPD plugin interrupt is received, DP
> controller will initialize phy so that dpcd read/write will function and
> following link training can be proceeded successfully. DP phy will be
> disabled after main link is teared down at end of unplugged HPD interrupt
> handle triggered by dongle unplugged out of DUT. Finally regulators, code
> clocks and irq are disabled at corresponding suspension.
>
> Changes in V2:
> -- removed unnecessary dp_ctrl NULL check
> -- removed unnecessary phy init_count and power_count DRM_DEBUG_DP logs
> -- remove flip parameter out of dp_ctrl_irq_enable()
> -- add fixes tag
>
> Changes in V3:
> -- call dp_display_host_phy_init() instead of dp_ctrl_phy_init() at
> dp_display_host_init() for eDP
>
> Changes in V4:
> -- rewording commit text to match this commit changes
>
> Changes in V5:
> -- rebase on top of msm-next branch
>
> Changes in V6:
> -- delete flip variable
>
> Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon
> Chipsets")
>
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_ctrl.c| 87
> -
> drivers/gpu/drm/msm/dp/dp_ctrl.h| 9 ++--
> drivers/gpu/drm/msm/dp/dp_display.c | 81 --
> 3 files changed, 102 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index c724cb0..7f0d647 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1365,60 +1365,54 @@ static int dp_ctrl_enable_stream_clocks(struct
> dp_ctrl_private *ctrl)
> return ret;
> }
>
> -int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
> +void dp_ctrl_irq_enable(struct dp_ctrl *dp_ctrl)
> +{
> + struct dp_ctrl_private *ctrl;
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> + dp_catalog_ctrl_reset(ctrl->catalog);
> +
> + dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
> +}
> +
> +void dp_ctrl_irq_disable(struct dp_ctrl *dp_ctrl)
> +{
> + struct dp_ctrl_private *ctrl;
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> + dp_catalog_ctrl_reset(ctrl->catalog);
> +
> + dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
> +}
The above could be one function with true/false for enable wrappers and
be a little shorter or the same length. It would also avoid problems if
the sequence needs to change. But speaking of that sequence, why doesn't
a dp_catalog_ctrl_reset() disable the irq automatically? I'd think we
could simply reset the whole controller for irq disable? But then,
shouldn't we only reset the controller on the enable path and only
disable the irq bits on irq_disable? If not, then the function name is
misleading. We're resetting the dp controller and enabling or disabling
irqs.
> +
> +void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
> {
> struct dp_ctrl_private *ctrl;
> struct dp_io *dp_io;
> struct phy *phy;
>
> - if (!dp_ctrl) {
> - DRM_ERROR("Invalid input data\n");
> - return -EINVAL;
> - }
> -
> ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> dp_io = >parser->io;
> phy = dp_io->phy;
>
> - ctrl->dp_ctrl.orientation = flip;
> -
> - if (reset)
> - dp_catalog_ctrl_reset(ctrl->catalog);
> -
> - DRM_DEBUG_DP("flip=%d\n", flip);
> dp_catalog_ctrl_phy_reset(ctrl->catalog);
> phy_init(phy);
> - dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
> -
> - return 0;
> }
>
[...]
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index d44f18b..0a53066 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -83,6 +83,7 @@ struct dp_display_private {
>
> /* state variables */
> bool core_initialized;
> + bool phy_initialized;
> bool hpd_irq_on;
> bool audio_supported;
>
> @@ -375,21 +376,46 @@ static int dp_display_process_hpd_high(struct
> dp_display_private *dp)
> return rc;
> }
>
> -static void dp_display_host_init(struct dp_display_private *dp, int reset)
> +static void
Re: [Freedreno] [PATCH v4 14/14] drm/msm: Implement HDCP 1.x using the new drm HDCP helpers
Quoting Sean Paul (2021-11-04 20:04:31)
> From: Sean Paul
>
> This patch adds HDCP 1.x support to msm DP connectors using the new HDCP
$ git grep "This patch" -- Documentation/process/
> helpers.
>
> Cc: Stephen Boyd
> Cc: Abhinav Kumar
> Signed-off-by: Sean Paul
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-15-s...@poorly.run
> #v1
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-14-s...@poorly.run
> #v2
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-15-s...@poorly.run
> #v3
>
> Changes in v2:
> -Squash [1] into this patch with the following changes (Stephen)
> -Update the sc7180 dtsi file
> -Remove resource names and just use index (Stephen)
> Changes in v3:
> -Split out the dtsi change from v2 (Stephen)
> -Fix set-but-unused warning identified by 0-day
> -Fix up a couple of style nits (Stephen)
> -Store HDCP key directly in dp_hdcp struct (Stephen)
> -Remove wmb in HDCP key initialization, move an_seed (Stephen)
> -Use FIELD_PREP for bstatus/bcaps (Stephen)
> -#define read_poll_timeout values (Stephen)
> -Remove unnecessary parentheses in dp_hdcp_store_ksv_fifo (Stephen)
> -Add compatible string for hdcp (Stephen)
> -Rename dp_hdcp_write_* functions (Abhinav)
> -Add 1us delay between An reads (Abhinav)
> -Delete unused dp_hdcp_read_* functions
> Changes in v4:
> -Rebase on Bjorn's multi-dp patchset
>
> [1]
> https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-14-s...@poorly.run
Looks mostly ok to me. One nit below but otherwise you can have my
Reviewed-by: Stephen Boyd
> diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
> b/drivers/gpu/drm/msm/dp/dp_debug.c
> index da4323556ef3..c16fce17d096 100644
> --- a/drivers/gpu/drm/msm/dp/dp_debug.c
> +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
> @@ -198,6 +201,35 @@ static int dp_test_active_open(struct inode *inode,
> inode->i_private);
> }
>
> +static ssize_t dp_hdcp_key_write(struct file *file, const char __user *ubuf,
> +size_t len, loff_t *offp)
I deem this API through debugfs no good, but I can see that opening the
can of worms that is programming the key other ways is worse, so alright.
> +{
> + char *input_buffer;
> + int ret;
> + struct dp_debug_private *debug = file->private_data;
> +
> + if (len != (DRM_HDCP_KSV_LEN + DP_HDCP_NUM_KEYS * DP_HDCP_KEY_LEN))
> + return -EINVAL;
> +
[]
> diff --git a/drivers/gpu/drm/msm/dp/dp_hdcp.c
> b/drivers/gpu/drm/msm/dp/dp_hdcp.c
> new file mode 100644
> index ..03ea3a974576
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/dp_hdcp.c
> @@ -0,0 +1,462 @@
[...]
> +
> +int dp_hdcp_attach(struct dp_hdcp *hdcp, struct drm_connector *connector)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_hdcp_helper_data *helper_data;
> + int ret;
> +
> + /* HDCP is not configured for this device */
> + if (!hdcp->parser->io.dp_controller.hdcp_key.base)
> + return 0;
> +
> + helper_data = drm_hdcp_helper_initialize_dp(connector, hdcp->aux,
> + _hdcp_funcs, false);
> + if (IS_ERR_OR_NULL(helper_data))
Just IS_ERR()?
> + return PTR_ERR(helper_data);
Because PTR_ERR() on NULL is zero. Maybe return PTR_ERR_OR_ZERO() is
supposed to be here? Or I don't understand why
drm_hdcp_helper_initialize_dp() would return NULL.
> +
> + ret = drm_connector_attach_content_protection_property(connector,
> false);
> + if (ret) {
> + drm_hdcp_helper_destroy(helper_data);
> + drm_err(dev, "Failed to attach content protection prop %d\n",
> ret);
> + return ret;
> + }
Re: [Freedreno] [PATCH v5] drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read failed
Quoting Kuogee Hsieh (2021-12-08 09:41:02)
> Add checking aux read/write status at both dp_link_parse_sink_count()
> and dp_link_parse_sink_status_filed() to avoid long timeout delay if
> dp aux read/write failed at timeout due to cable unplugged.
>
> Changes in V4:
> -- split this patch as stand alone patch
>
> Changes in v5:
> -- rebase on msm-next branch
>
> Signed-off-by: Kuogee Hsieh
>
Remove this newline please.
> Reviewed-by: Stephen Boyd
> Tested-by: Stephen Boyd
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 12 +---
> drivers/gpu/drm/msm/dp/dp_link.c| 19 ++-
> 2 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 3d61459..0766752 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -692,9 +692,15 @@ static int dp_irq_hpd_handle(struct dp_display_private
> *dp, u32 data)
> return 0;
> }
>
> - ret = dp_display_usbpd_attention_cb(>pdev->dev);
> - if (ret == -ECONNRESET) { /* cable unplugged */
> - dp->core_initialized = false;
> + /*
> +* dp core (ahb/aux clks) must be initialized before
> +* irq_hpd be handled
> +*/
> + if (dp->core_initialized) {
This part of the commit isn't described in the commit text. Can you add
some more details in the commit text about this?
> + ret = dp_display_usbpd_attention_cb(>pdev->dev);
> + if (ret == -ECONNRESET) { /* cable unplugged */
> + dp->core_initialized = false;
> + }
> }
> DRM_DEBUG_DP("hpd_state=%d\n", state);
>
