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@quicinc.com>
---
 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;
+
+	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);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 0e944db3adf2..7910b11fd685 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -50,6 +50,8 @@ struct msm_dp_panel {
 	u32 max_dp_link_rate;
 
 	u32 max_bw_code;
+
+	bool tpg_enabled;
 };
 
 int msm_dp_panel_init_panel_info(struct msm_dp_panel *msm_dp_panel);

-- 
2.34.1

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@quicinc.com>
> ---
>  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?

> +
> +	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.

>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
> index 0e944db3adf2..7910b11fd685 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> @@ -50,6 +50,8 @@ struct msm_dp_panel {
>  	u32 max_dp_link_rate;
>  
>  	u32 max_bw_code;
> +
> +	bool tpg_enabled;
>  };
>  
>  int msm_dp_panel_init_panel_info(struct msm_dp_panel *msm_dp_panel);
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

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@quicinc.com>
>> ---
>>   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.

On Tue, Dec 03, 2024 at 07:57:25PM -0800, Abhinav Kumar wrote:
> 
> 
> 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@quicinc.com>
> > > ---
> > >   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.

Yes, I think it's worth doing that.

> 
> > > +
> > > +	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.

-- 
With best wishes
Dmitry