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