[PATCH 0/3] drm/msm/dp: Fix and utilize TPG with a debugfs

[PATCH 0/3] drm/msm/dp: Fix and utilize TPG with a debugfs

[Abhinav Kumar]

DP Test Patten Generator is a very useful tool to debug issues such
as blank screen or corruption seen on the DP monitor by isolating it
to whether the corruption is coming from further upstream such as DPU
OR from the DP controller and below. It was noted in [1] that this API
is unused. Rather than dropping the API, it should be fixed and used.

Hence, this series fixes the DP Test Patten Generator API and also utilizes
it by adding a debugfs for it.

[1] : https://patchwork.freedesktop.org/patch/623508/?series=141074&rev=1

To: Rob Clark <robdclark@gmail.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Sean Paul <sean@poorly.run>
To: Marijn Suijten <marijn.suijten@somainline.org>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Stephen Boyd <swboyd@chromium.org>
To: Kuogee Hsieh <quic_khsieh@quicinc.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: freedreno@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: Jessica Zhang <quic_jesszhan@quicinc.com>

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
Abhinav Kumar (3):
      drm/msm/dp: account for widebus in msm_dp_catalog_panel_tpg_enable()
      drm/msm/dp: do not touch the MMSS_DP_INTF_CONFIG for tpg
      drm/msm/dp: add a debugfs node for using tpg

 drivers/gpu/drm/msm/dp/dp_catalog.c | 15 +++++++--
 drivers/gpu/drm/msm/dp/dp_debug.c   | 61 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_panel.h   |  2 ++
 3 files changed, 76 insertions(+), 2 deletions(-)
---
base-commit: 798bb342e0416d846cf67f4725a3428f39bfb96b
change-id: 20241202-tpg-3f7543c036ac

Best regards,
-- 
Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH 1/3] drm/msm/dp: account for widebus in msm_dp_catalog_panel_tpg_enable()

[Abhinav Kumar]

Adjust the h_active calculation to account for widebus in tpg.

Fixes: 757a2f36ab09 ("drm/msm/dp: enable widebus feature for display port")
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_catalog.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index b4c8856fb25d..05c8e1996f60 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -1011,9 +1011,21 @@ void msm_dp_catalog_panel_tpg_enable(struct msm_dp_catalog *msm_dp_catalog,
 	u32 v_sync_width;
 	u32 hsync_ctl;
 	u32 display_hctl;
+	u32 h_sync_width;
+	u32 h_front_porch;
+	u32 h_back_porch;
+	u32 h_active;
+
+	h_active = drm_mode->hdisplay;
+	h_back_porch = drm_mode->htotal - drm_mode->hsync_end;
+	h_sync_width = drm_mode->htotal - (drm_mode->hsync_start + h_back_porch);
+	h_front_porch = drm_mode->hsync_start - drm_mode->hdisplay;
+
+	if (msm_dp_catalog->wide_bus_en)
+		h_active /= 2;
 
 	/* TPG config parameters*/
-	hsync_period = drm_mode->htotal;
+	hsync_period = h_sync_width + h_back_porch + h_active + h_front_porch;
 	vsync_period = drm_mode->vtotal;
 
 	display_v_start = ((drm_mode->vtotal - drm_mode->vsync_start) *

-- 
2.34.1

Re: [PATCH 1/3] drm/msm/dp: account for widebus in msm_dp_catalog_panel_tpg_enable()

[Dmitry Baryshkov]

On Mon, Dec 02, 2024 at 12:41:58PM -0800, Abhinav Kumar wrote:
> Adjust the h_active calculation to account for widebus in tpg.
> 
> Fixes: 757a2f36ab09 ("drm/msm/dp: enable widebus feature for display port")
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index b4c8856fb25d..05c8e1996f60 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -1011,9 +1011,21 @@ void msm_dp_catalog_panel_tpg_enable(struct msm_dp_catalog *msm_dp_catalog,
>  	u32 v_sync_width;
>  	u32 hsync_ctl;
>  	u32 display_hctl;
> +	u32 h_sync_width;
> +	u32 h_front_porch;
> +	u32 h_back_porch;
> +	u32 h_active;
> +
> +	h_active = drm_mode->hdisplay;
> +	h_back_porch = drm_mode->htotal - drm_mode->hsync_end;
> +	h_sync_width = drm_mode->htotal - (drm_mode->hsync_start + h_back_porch);

It's at least drm_mode->hsync_end - drm_mode->hsync_start

> +	h_front_porch = drm_mode->hsync_start - drm_mode->hdisplay;
> +
> +	if (msm_dp_catalog->wide_bus_en)
> +		h_active /= 2;
>  
>  	/* TPG config parameters*/
> -	hsync_period = drm_mode->htotal;
> +	hsync_period = h_sync_width + h_back_porch + h_active + h_front_porch;

Is it equivalent to:

hsync_period = drm_mode->htotal;
if (msm_dp_catalog->wide_bus_en)
    hsync_period -= drm_mode->hdisplay / 2;

I think it's simpler to handle.

>  	vsync_period = drm_mode->vtotal;
>  
>  	display_v_start = ((drm_mode->vtotal - drm_mode->vsync_start) *
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/msm/dp: account for widebus in msm_dp_catalog_panel_tpg_enable()

[Abhinav Kumar]

On 12/3/2024 6:34 AM, Dmitry Baryshkov wrote:
> On Mon, Dec 02, 2024 at 12:41:58PM -0800, Abhinav Kumar wrote:
>> Adjust the h_active calculation to account for widebus in tpg.
>>
>> Fixes: 757a2f36ab09 ("drm/msm/dp: enable widebus feature for display port")
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_catalog.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index b4c8856fb25d..05c8e1996f60 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -1011,9 +1011,21 @@ void msm_dp_catalog_panel_tpg_enable(struct msm_dp_catalog *msm_dp_catalog,
>>   	u32 v_sync_width;
>>   	u32 hsync_ctl;
>>   	u32 display_hctl;
>> +	u32 h_sync_width;
>> +	u32 h_front_porch;
>> +	u32 h_back_porch;
>> +	u32 h_active;
>> +
>> +	h_active = drm_mode->hdisplay;
>> +	h_back_porch = drm_mode->htotal - drm_mode->hsync_end;
>> +	h_sync_width = drm_mode->htotal - (drm_mode->hsync_start + h_back_porch);
> 
> It's at least drm_mode->hsync_end - drm_mode->hsync_start
> 
>> +	h_front_porch = drm_mode->hsync_start - drm_mode->hdisplay;
>> +
>> +	if (msm_dp_catalog->wide_bus_en)
>> +		h_active /= 2;
>>   
>>   	/* TPG config parameters*/
>> -	hsync_period = drm_mode->htotal;
>> +	hsync_period = h_sync_width + h_back_porch + h_active + h_front_porch;
> 
> Is it equivalent to:
> 
> hsync_period = drm_mode->htotal;
> if (msm_dp_catalog->wide_bus_en)
>      hsync_period -= drm_mode->hdisplay / 2;
> 
> I think it's simpler to handle.
> 

Yes certainly!

Thanks for the feedback. Will fix it up.

>>   	vsync_period = drm_mode->vtotal;
>>   
>>   	display_v_start = ((drm_mode->vtotal - drm_mode->vsync_start) *
>>
>> -- 
>> 2.34.1
>>
> 

[PATCH 2/3] drm/msm/dp: do not touch the MMSS_DP_INTF_CONFIG for tpg

[Abhinav Kumar]

MMSS_DP_INTF_CONFIG has already been setup by the main datapath
for DP to account for widebus to be used/unused etc.

In current implementation, TPG only switches the DP controller
to use the main datapath stream OR use the test pattern but expects
the rest of the controller to be already setup.

Keeping the same behavior intact, drop the clearing of MMSS_DP_INTF_CONFIG
from the msm_dp_catalog_panel_tpg_enable() API.

Fixes: 757a2f36ab09 ("drm/msm/dp: enable widebus feature for display port")
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_catalog.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 05c8e1996f60..36d3f3d248ca 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -1048,7 +1048,6 @@ void msm_dp_catalog_panel_tpg_enable(struct msm_dp_catalog *msm_dp_catalog,
 	display_hctl = (hsync_end_x << 16) | hsync_start_x;
 
 
-	msm_dp_write_p0(catalog, MMSS_DP_INTF_CONFIG, 0x0);
 	msm_dp_write_p0(catalog, MMSS_DP_INTF_HSYNC_CTL, hsync_ctl);
 	msm_dp_write_p0(catalog, MMSS_DP_INTF_VSYNC_PERIOD_F0, vsync_period *
 			hsync_period);

-- 
2.34.1

Re: [PATCH 2/3] drm/msm/dp: do not touch the MMSS_DP_INTF_CONFIG for tpg

[Dmitry Baryshkov]

On Mon, Dec 02, 2024 at 12:41:59PM -0800, Abhinav Kumar wrote:
> MMSS_DP_INTF_CONFIG has already been setup by the main datapath
> for DP to account for widebus to be used/unused etc.
> 
> In current implementation, TPG only switches the DP controller
> to use the main datapath stream OR use the test pattern but expects
> the rest of the controller to be already setup.
> 
> Keeping the same behavior intact, drop the clearing of MMSS_DP_INTF_CONFIG
> from the msm_dp_catalog_panel_tpg_enable() API.
> 
> Fixes: 757a2f36ab09 ("drm/msm/dp: enable widebus feature for display port")
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 1 -
>  1 file changed, 1 deletion(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 3/3] drm/msm/dp: add a debugfs node for using tpg

[Abhinav Kumar]

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

Re: [PATCH 3/3] drm/msm/dp: add a debugfs node for using tpg

[Dmitry Baryshkov]

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

Re: [PATCH 3/3] drm/msm/dp: add a debugfs node for using tpg

[Abhinav Kumar]

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.

Re: [PATCH 3/3] drm/msm/dp: add a debugfs node for using tpg

[Dmitry Baryshkov]

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

Re: [PATCH 0/3] drm/msm/dp: Fix and utilize TPG with a debugfs

[Dmitry Baryshkov]

On Mon, Dec 02, 2024 at 12:41:57PM -0800, Abhinav Kumar wrote:
> DP Test Patten Generator is a very useful tool to debug issues such
> as blank screen or corruption seen on the DP monitor by isolating it
> to whether the corruption is coming from further upstream such as DPU
> OR from the DP controller and below. It was noted in [1] that this API
> is unused. Rather than dropping the API, it should be fixed and used.
> 
> Hence, this series fixes the DP Test Patten Generator API and also utilizes
> it by adding a debugfs for it.
> 
> [1] : https://patchwork.freedesktop.org/patch/623508/?series=141074&rev=1

I'd prefer for this to be rebased on top of [2]. The series has been
posted a month ago.

[2] https://patchwork.freedesktop.org/series/141074/

> 
> To: Rob Clark <robdclark@gmail.com>
> To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> To: Sean Paul <sean@poorly.run>
> To: Marijn Suijten <marijn.suijten@somainline.org>
> To: David Airlie <airlied@gmail.com>
> To: Simona Vetter <simona@ffwll.ch>
> To: Stephen Boyd <swboyd@chromium.org>
> To: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: freedreno@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Jessica Zhang <quic_jesszhan@quicinc.com>
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> Abhinav Kumar (3):
>       drm/msm/dp: account for widebus in msm_dp_catalog_panel_tpg_enable()
>       drm/msm/dp: do not touch the MMSS_DP_INTF_CONFIG for tpg
>       drm/msm/dp: add a debugfs node for using tpg
> 
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 15 +++++++--
>  drivers/gpu/drm/msm/dp/dp_debug.c   | 61 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_panel.h   |  2 ++
>  3 files changed, 76 insertions(+), 2 deletions(-)
> ---
> base-commit: 798bb342e0416d846cf67f4725a3428f39bfb96b
> change-id: 20241202-tpg-3f7543c036ac
> 
> Best regards,
> -- 
> Abhinav Kumar <quic_abhinavk@quicinc.com>
> 

-- 
With best wishes
Dmitry

Re: [PATCH 0/3] drm/msm/dp: Fix and utilize TPG with a debugfs

[Abhinav Kumar]

On 12/3/2024 6:31 AM, Dmitry Baryshkov wrote:
> On Mon, Dec 02, 2024 at 12:41:57PM -0800, Abhinav Kumar wrote:
>> DP Test Patten Generator is a very useful tool to debug issues such
>> as blank screen or corruption seen on the DP monitor by isolating it
>> to whether the corruption is coming from further upstream such as DPU
>> OR from the DP controller and below. It was noted in [1] that this API
>> is unused. Rather than dropping the API, it should be fixed and used.
>>
>> Hence, this series fixes the DP Test Patten Generator API and also utilizes
>> it by adding a debugfs for it.
>>
>> [1] : https://patchwork.freedesktop.org/patch/623508/?series=141074&rev=1
> 
> I'd prefer for this to be rebased on top of [2]. The series has been
> posted a month ago.
> 
> [2] https://patchwork.freedesktop.org/series/141074/
> 

Well, the review of that series which tried dropping the tpg led to this 
one really.

I will review that one within a couple of days. Lets first agree on that 
series before deciding to rebase.

>>
>> To: Rob Clark <robdclark@gmail.com>
>> To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> To: Sean Paul <sean@poorly.run>
>> To: Marijn Suijten <marijn.suijten@somainline.org>
>> To: David Airlie <airlied@gmail.com>
>> To: Simona Vetter <simona@ffwll.ch>
>> To: Stephen Boyd <swboyd@chromium.org>
>> To: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> Cc: linux-arm-msm@vger.kernel.org
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: freedreno@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: Jessica Zhang <quic_jesszhan@quicinc.com>
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>> Abhinav Kumar (3):
>>        drm/msm/dp: account for widebus in msm_dp_catalog_panel_tpg_enable()
>>        drm/msm/dp: do not touch the MMSS_DP_INTF_CONFIG for tpg
>>        drm/msm/dp: add a debugfs node for using tpg
>>
>>   drivers/gpu/drm/msm/dp/dp_catalog.c | 15 +++++++--
>>   drivers/gpu/drm/msm/dp/dp_debug.c   | 61 +++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/msm/dp/dp_panel.h   |  2 ++
>>   3 files changed, 76 insertions(+), 2 deletions(-)
>> ---
>> base-commit: 798bb342e0416d846cf67f4725a3428f39bfb96b
>> change-id: 20241202-tpg-3f7543c036ac
>>
>> Best regards,
>> -- 
>> Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
> 

Re: [PATCH 0/3] drm/msm/dp: Fix and utilize TPG with a debugfs

[Dmitry Baryshkov]

On Tue, 3 Dec 2024 at 21:42, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/3/2024 6:31 AM, Dmitry Baryshkov wrote:
> > On Mon, Dec 02, 2024 at 12:41:57PM -0800, Abhinav Kumar wrote:
> >> DP Test Patten Generator is a very useful tool to debug issues such
> >> as blank screen or corruption seen on the DP monitor by isolating it
> >> to whether the corruption is coming from further upstream such as DPU
> >> OR from the DP controller and below. It was noted in [1] that this API
> >> is unused. Rather than dropping the API, it should be fixed and used.
> >>
> >> Hence, this series fixes the DP Test Patten Generator API and also utilizes
> >> it by adding a debugfs for it.
> >>
> >> [1] : https://patchwork.freedesktop.org/patch/623508/?series=141074&rev=1
> >
> > I'd prefer for this to be rebased on top of [2]. The series has been
> > posted a month ago.
> >
> > [2] https://patchwork.freedesktop.org/series/141074/
> >
>
> Well, the review of that series which tried dropping the tpg led to this
> one really.
>
> I will review that one within a couple of days. Lets first agree on that
> series before deciding to rebase.

SGTM. Note, v2 kept the TPG in place, as you have asked.

>
> >>
> >> To: Rob Clark <robdclark@gmail.com>
> >> To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> To: Sean Paul <sean@poorly.run>
> >> To: Marijn Suijten <marijn.suijten@somainline.org>
> >> To: David Airlie <airlied@gmail.com>
> >> To: Simona Vetter <simona@ffwll.ch>
> >> To: Stephen Boyd <swboyd@chromium.org>
> >> To: Kuogee Hsieh <quic_khsieh@quicinc.com>
> >> Cc: linux-arm-msm@vger.kernel.org
> >> Cc: dri-devel@lists.freedesktop.org
> >> Cc: freedreno@lists.freedesktop.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: Jessica Zhang <quic_jesszhan@quicinc.com>
> >>
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >> ---
> >> Abhinav Kumar (3):
> >>        drm/msm/dp: account for widebus in msm_dp_catalog_panel_tpg_enable()
> >>        drm/msm/dp: do not touch the MMSS_DP_INTF_CONFIG for tpg
> >>        drm/msm/dp: add a debugfs node for using tpg
> >>
> >>   drivers/gpu/drm/msm/dp/dp_catalog.c | 15 +++++++--
> >>   drivers/gpu/drm/msm/dp/dp_debug.c   | 61 +++++++++++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/msm/dp/dp_panel.h   |  2 ++
> >>   3 files changed, 76 insertions(+), 2 deletions(-)
> >> ---
> >> base-commit: 798bb342e0416d846cf67f4725a3428f39bfb96b
> >> change-id: 20241202-tpg-3f7543c036ac
> >>
> >> Best regards,
> >> --
> >> Abhinav Kumar <quic_abhinavk@quicinc.com>
> >>
> >



-- 
With best wishes
Dmitry

Re: [PATCH 0/3] drm/msm/dp: Fix and utilize TPG with a debugfs

[Marijn Suijten]

On 2024-12-02 12:41:57, Abhinav Kumar wrote:
> DP Test Patten Generator is a very useful tool to debug issues such
> as blank screen or corruption seen on the DP monitor by isolating it
> to whether the corruption is coming from further upstream such as DPU
> OR from the DP controller and below. It was noted in [1] that this API
> is unused. Rather than dropping the API, it should be fixed and used.
> 
> Hence, this series fixes the DP Test Patten Generator API and also utilizes
> it by adding a debugfs for it.
> 
> [1] : https://patchwork.freedesktop.org/patch/623508/?series=141074&rev=1

This all sounds great, but perhaps the "unused function" issue applies to the
DSI TPG from your series at [1] as well.  There, the patch describes where to
insert the function call, but I remember that the contents of that function also
changed over time.

Are you planning to add a debugfs entry, or perhaps entries to configure the
pattern bits for DPU's DSI TPG as well, as suggested in [2]?  Or should someone
else look into it to make it useful again?

[1]: https://lore.kernel.org/linux-arm-msm/1626922232-29105-2-git-send-email-abhinavk@codeaurora.org/
[2]: https://lore.kernel.org/linux-arm-msm/CAA8EJpqjWbEbgn9Sw=Hd8f7pP+NXDeM308jG8HirKjk7QcfOxA@mail.gmail.com/

> 
> To: Rob Clark <robdclark@gmail.com>
> To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> To: Sean Paul <sean@poorly.run>
> To: Marijn Suijten <marijn.suijten@somainline.org>
> To: David Airlie <airlied@gmail.com>
> To: Simona Vetter <simona@ffwll.ch>
> To: Stephen Boyd <swboyd@chromium.org>
> To: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: freedreno@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Jessica Zhang <quic_jesszhan@quicinc.com>
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
> Abhinav Kumar (3):
>       drm/msm/dp: account for widebus in msm_dp_catalog_panel_tpg_enable()
>       drm/msm/dp: do not touch the MMSS_DP_INTF_CONFIG for tpg
>       drm/msm/dp: add a debugfs node for using tpg

Should TPG be capitalized in these titles, as it is an abbreviation with
capitalized words in your description as well?

Thanks!

- Marijn

> 
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 15 +++++++--
>  drivers/gpu/drm/msm/dp/dp_debug.c   | 61 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_panel.h   |  2 ++
>  3 files changed, 76 insertions(+), 2 deletions(-)
> ---
> base-commit: 798bb342e0416d846cf67f4725a3428f39bfb96b
> change-id: 20241202-tpg-3f7543c036ac
> 
> Best regards,
> -- 
> Abhinav Kumar <quic_abhinavk@quicinc.com>
>