On 12/3/2024 3:38 PM, Dmitry Baryshkov wrote:
On Mon, Dec 02, 2024 at 12:42:00PM -0800, Abhinav Kumar wrote:DP test pattern generator is a very useful tool to debug issues where monitor is showing incorrect output as it helps to isolate whether the issue is due to rest of DPU pipeline or in the DP controller itself. Expose a debugfs to use the TPG configuration to help debug DP issues. Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> --- drivers/gpu/drm/msm/dp/dp_debug.c | 61 +++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/msm/dp/dp_panel.h | 2 ++ 2 files changed, 63 insertions(+) diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c b/drivers/gpu/drm/msm/dp/dp_debug.c index 22fd946ee201..843fe77268f8 100644 --- a/drivers/gpu/drm/msm/dp/dp_debug.c +++ b/drivers/gpu/drm/msm/dp/dp_debug.c @@ -197,6 +197,65 @@ static const struct file_operations test_active_fops = { .write = msm_dp_test_active_write };+static ssize_t msm_dp_tpg_write(struct file *file, const char __user *ubuf,+ size_t len, loff_t *offp) +{ + const struct msm_dp_debug_private *debug; + char *input_buffer; + int val; + int status = 0; + struct msm_dp_panel *dp_panel; + + debug = ((struct seq_file *)file->private_data)->private; + dp_panel = debug->panel; + + input_buffer = memdup_user_nul(ubuf, len); + if (IS_ERR(input_buffer)) + return PTR_ERR(input_buffer); + + status = kstrtoint(input_buffer, 10, &val); + if (status < 0) { + kfree(input_buffer); + return status; + } + + msm_dp_panel_tpg_config(dp_panel, val); + + dp_panel->tpg_enabled = val;Does this need any kind of locking? The driver performs some actions, then we write the global state. What if the user in parallel writes different values to the file?
Sure, I can add a lock to struct msm_dp_debug_private and use it to protect the tpg_write and tpg_reads.
+ + kfree(input_buffer); + + *offp += len; + return len; +} + +static int msm_dp_tpg_show(struct seq_file *f, void *data) +{ + struct msm_dp_debug_private *debug = f->private; + struct msm_dp_panel *dp_panel = debug->panel; + + if (dp_panel->tpg_enabled) + seq_puts(f, "1"); + else + seq_puts(f, "0"); + + return 0; +} + +static int msm_dp_tpg_open(struct inode *inode, struct file *file) +{ + return single_open(file, msm_dp_tpg_show, inode->i_private); +} + +static const struct file_operations msm_dp_tpg_fops = { + .owner = THIS_MODULE, + .open = msm_dp_tpg_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, + .write = msm_dp_tpg_write +}; + int msm_dp_debug_init(struct device *dev, struct msm_dp_panel *panel, struct msm_dp_link *link, struct drm_connector *connector, @@ -231,6 +290,8 @@ int msm_dp_debug_init(struct device *dev, struct msm_dp_panel *panel, debugfs_create_file("dp_test_type", 0444, root, debug, &msm_dp_test_type_fops); + + debugfs_create_file("dp_tpg", 0444, root, debug, &msm_dp_tpg_fops);I'd say, skip the dp_ part of the name, everything in that dir is DP-related.
Ack.
