diff mbox series

drm/msm/dp: Make it possible to enable the test pattern

Message ID 20210629002234.1787149-1-bjorn.andersson@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm/msm/dp: Make it possible to enable the test pattern | expand

Commit Message

Bjorn Andersson June 29, 2021, 12:22 a.m. UTC
The debugfs interface contains the knobs to make the DisplayPort
controller output a test pattern, unfortunately there's nothing
currently that actually enables the defined test pattern.

Fixes: de3ee25473ba ("drm/msm/dp: add debugfs nodes for video pattern tests")
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_debug.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Abhinav Kumar June 29, 2021, 12:31 a.m. UTC | #1
Hi Bjorn

On 2021-06-28 17:22, Bjorn Andersson wrote:
> The debugfs interface contains the knobs to make the DisplayPort
> controller output a test pattern, unfortunately there's nothing
> currently that actually enables the defined test pattern.
> 
> Fixes: de3ee25473ba ("drm/msm/dp: add debugfs nodes for video pattern 
> tests")
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

This is not how this debugfs node works. This is meant to be used while 
running
DP compliance video pattern test.

https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tools/msm_dp_compliance.c

While the compliance test is being run with this msm_dp_compliance app 
running,
it will draw the test pattern when it gets the "test_active" from the 
driver.

The test pattern which this app draws is as per the requirements of the 
compliance test
as the test equipment will match the CRC of the pattern which is drawn.

The API dp_panel_tpg_config() which you are trying to call here draws 
the DP test pattern
from the DP controller hardware but not the pattern which the compliance 
test expects.

Its just a debug API to call when required during bringup/debug 
purposes.

Hence this is not the place to call it as it will end up breaking CTS.

Thanks

Abhinav

> ---
>  drivers/gpu/drm/msm/dp/dp_debug.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
> b/drivers/gpu/drm/msm/dp/dp_debug.c
> index 2f6247e80e9d..82911af44905 100644
> --- a/drivers/gpu/drm/msm/dp/dp_debug.c
> +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
> @@ -305,6 +305,8 @@ static ssize_t dp_test_active_write(struct file 
> *file,
>  				debug->panel->video_test = true;
>  			else
>  				debug->panel->video_test = false;
> +
> +			dp_panel_tpg_config(debug->panel, debug->panel->video_test);
>  		}
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
Bjorn Andersson June 29, 2021, 12:55 a.m. UTC | #2
On Mon 28 Jun 19:31 CDT 2021, abhinavk@codeaurora.org wrote:

> Hi Bjorn
> 
> On 2021-06-28 17:22, Bjorn Andersson wrote:
> > The debugfs interface contains the knobs to make the DisplayPort
> > controller output a test pattern, unfortunately there's nothing
> > currently that actually enables the defined test pattern.
> > 
> > Fixes: de3ee25473ba ("drm/msm/dp: add debugfs nodes for video pattern
> > tests")
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> This is not how this debugfs node works. This is meant to be used while
> running
> DP compliance video pattern test.
> 
> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tools/msm_dp_compliance.c
> 
> While the compliance test is being run with this msm_dp_compliance app
> running,
> it will draw the test pattern when it gets the "test_active" from the
> driver.
> 
> The test pattern which this app draws is as per the requirements of the
> compliance test
> as the test equipment will match the CRC of the pattern which is drawn.
> 
> The API dp_panel_tpg_config() which you are trying to call here draws the DP
> test pattern
> from the DP controller hardware but not the pattern which the compliance
> test expects.
> 

So clearly not an oversight, but rather me not understanding how to use
the test pattern.

You say that I should run msm_dp_compliance while the test is running,
so how do I run the test?

> Its just a debug API to call when required during bringup/debug purposes.
> 

Yes, I was trying to isolate the DP code from some misconfiguration in
the DPU during bringup and with this fix the debugfs interface became
useful.

Regards,
Bjorn

> Hence this is not the place to call it as it will end up breaking CTS.
> 
> Thanks
> 
> Abhinav
> 
> > ---
> >  drivers/gpu/drm/msm/dp/dp_debug.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
> > b/drivers/gpu/drm/msm/dp/dp_debug.c
> > index 2f6247e80e9d..82911af44905 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_debug.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
> > @@ -305,6 +305,8 @@ static ssize_t dp_test_active_write(struct file
> > *file,
> >  				debug->panel->video_test = true;
> >  			else
> >  				debug->panel->video_test = false;
> > +
> > +			dp_panel_tpg_config(debug->panel, debug->panel->video_test);
> >  		}
> >  	}
> >  	drm_connector_list_iter_end(&conn_iter);
Abhinav Kumar June 29, 2021, 1:07 a.m. UTC | #3
On 2021-06-28 17:55, Bjorn Andersson wrote:
> On Mon 28 Jun 19:31 CDT 2021, abhinavk@codeaurora.org wrote:
> 
>> Hi Bjorn
>> 
>> On 2021-06-28 17:22, Bjorn Andersson wrote:
>> > The debugfs interface contains the knobs to make the DisplayPort
>> > controller output a test pattern, unfortunately there's nothing
>> > currently that actually enables the defined test pattern.
>> >
>> > Fixes: de3ee25473ba ("drm/msm/dp: add debugfs nodes for video pattern
>> > tests")
>> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> 
>> This is not how this debugfs node works. This is meant to be used 
>> while
>> running
>> DP compliance video pattern test.
>> 
>> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tools/msm_dp_compliance.c
>> 
>> While the compliance test is being run with this msm_dp_compliance app
>> running,
>> it will draw the test pattern when it gets the "test_active" from the
>> driver.
>> 
>> The test pattern which this app draws is as per the requirements of 
>> the
>> compliance test
>> as the test equipment will match the CRC of the pattern which is 
>> drawn.
>> 
>> The API dp_panel_tpg_config() which you are trying to call here draws 
>> the DP
>> test pattern
>> from the DP controller hardware but not the pattern which the 
>> compliance
>> test expects.
>> 
> 
> So clearly not an oversight, but rather me not understanding how to use
> the test pattern.
> 
> You say that I should run msm_dp_compliance while the test is running,
> so how do I run the test?

There are two test patterns with different purposes. The one which the 
msm_dp_compliance
draws is strictly for the DP compliance test and it needs even the DPU 
to draw the frame because
it sets up the display pipeline and just draws the buffer.

That is not what you are looking for here.

So rather than trying to run msm_dp_compliance on your setup, just try 
calling dp_panel_tpg_config().
We typically just call this API, right after the link training is done.
But if you really need a debugfs node for this, you can write up a 
separate debugfs for it
Something like:

echo 1 > dp/tpg/en

Lets not disturb this one.

> 
>> Its just a debug API to call when required during bringup/debug 
>> purposes.
>> 
> 
> Yes, I was trying to isolate the DP code from some misconfiguration in
> the DPU during bringup and with this fix the debugfs interface became
> useful.

> 
> Regards,
> Bjorn
> 
>> Hence this is not the place to call it as it will end up breaking CTS.
>> 
>> Thanks
>> 
>> Abhinav
>> 
>> > ---
>> >  drivers/gpu/drm/msm/dp/dp_debug.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
>> > b/drivers/gpu/drm/msm/dp/dp_debug.c
>> > index 2f6247e80e9d..82911af44905 100644
>> > --- a/drivers/gpu/drm/msm/dp/dp_debug.c
>> > +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
>> > @@ -305,6 +305,8 @@ static ssize_t dp_test_active_write(struct file
>> > *file,
>> >  				debug->panel->video_test = true;
>> >  			else
>> >  				debug->panel->video_test = false;
>> > +
>> > +			dp_panel_tpg_config(debug->panel, debug->panel->video_test);
>> >  		}
>> >  	}
>> >  	drm_connector_list_iter_end(&conn_iter);
kernel test robot June 29, 2021, 10:54 a.m. UTC | #4
Hi Bjorn,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.13 next-20210628]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bjorn-Andersson/drm-msm-dp-Make-it-possible-to-enable-the-test-pattern/20210629-082507
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9840cfcb97fc8b6aa7b36cec3cc3fd763f14052e
config: arm64-randconfig-r004-20210628 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project aad87328fabff9382bac0b452c83934515e6d0c8)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/e7c606f6fabdb01a1b5b3942c660214b0fbf049f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Bjorn-Andersson/drm-msm-dp-Make-it-possible-to-enable-the-test-pattern/20210629-082507
        git checkout e7c606f6fabdb01a1b5b3942c660214b0fbf049f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "dp_panel_tpg_config" [drivers/gpu/drm/msm/msm.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Bjorn Andersson June 29, 2021, 2:46 p.m. UTC | #5
On Mon 28 Jun 20:07 CDT 2021, abhinavk@codeaurora.org wrote:

> On 2021-06-28 17:55, Bjorn Andersson wrote:
> > On Mon 28 Jun 19:31 CDT 2021, abhinavk@codeaurora.org wrote:
> > 
> > > Hi Bjorn
> > > 
> > > On 2021-06-28 17:22, Bjorn Andersson wrote:
> > > > The debugfs interface contains the knobs to make the DisplayPort
> > > > controller output a test pattern, unfortunately there's nothing
> > > > currently that actually enables the defined test pattern.
> > > >
> > > > Fixes: de3ee25473ba ("drm/msm/dp: add debugfs nodes for video pattern
> > > > tests")
> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > 
> > > This is not how this debugfs node works. This is meant to be used
> > > while
> > > running
> > > DP compliance video pattern test.
> > > 
> > > https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tools/msm_dp_compliance.c
> > > 
> > > While the compliance test is being run with this msm_dp_compliance app
> > > running,
> > > it will draw the test pattern when it gets the "test_active" from the
> > > driver.
> > > 
> > > The test pattern which this app draws is as per the requirements of
> > > the
> > > compliance test
> > > as the test equipment will match the CRC of the pattern which is
> > > drawn.
> > > 
> > > The API dp_panel_tpg_config() which you are trying to call here
> > > draws the DP
> > > test pattern
> > > from the DP controller hardware but not the pattern which the
> > > compliance
> > > test expects.
> > > 
> > 
> > So clearly not an oversight, but rather me not understanding how to use
> > the test pattern.
> > 
> > You say that I should run msm_dp_compliance while the test is running,
> > so how do I run the test?
> 
> There are two test patterns with different purposes. The one which the
> msm_dp_compliance
> draws is strictly for the DP compliance test and it needs even the DPU to
> draw the frame because
> it sets up the display pipeline and just draws the buffer.
> 
> That is not what you are looking for here.
> 
> So rather than trying to run msm_dp_compliance on your setup, just try
> calling dp_panel_tpg_config().
> We typically just call this API, right after the link training is done.
> But if you really need a debugfs node for this, you can write up a separate
> debugfs for it
> Something like:
> 
> echo 1 > dp/tpg/en
> 

Having the ability to turn on the test pattern was very useful to me and
I would use this next time I'm adding DP support on a new platform. So
adding some way of invoking that API without a lot of extra effort seems
useful.

> Lets not disturb this one.
> 

Agreed.

Thanks,
Bjorn

> > 
> > > Its just a debug API to call when required during bringup/debug
> > > purposes.
> > > 
> > 
> > Yes, I was trying to isolate the DP code from some misconfiguration in
> > the DPU during bringup and with this fix the debugfs interface became
> > useful.
> 
> > 
> > Regards,
> > Bjorn
> > 
> > > Hence this is not the place to call it as it will end up breaking CTS.
> > > 
> > > Thanks
> > > 
> > > Abhinav
> > > 
> > > > ---
> > > >  drivers/gpu/drm/msm/dp/dp_debug.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
> > > > b/drivers/gpu/drm/msm/dp/dp_debug.c
> > > > index 2f6247e80e9d..82911af44905 100644
> > > > --- a/drivers/gpu/drm/msm/dp/dp_debug.c
> > > > +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
> > > > @@ -305,6 +305,8 @@ static ssize_t dp_test_active_write(struct file
> > > > *file,
> > > >  				debug->panel->video_test = true;
> > > >  			else
> > > >  				debug->panel->video_test = false;
> > > > +
> > > > +			dp_panel_tpg_config(debug->panel, debug->panel->video_test);
> > > >  		}
> > > >  	}
> > > >  	drm_connector_list_iter_end(&conn_iter);
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c b/drivers/gpu/drm/msm/dp/dp_debug.c
index 2f6247e80e9d..82911af44905 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.c
+++ b/drivers/gpu/drm/msm/dp/dp_debug.c
@@ -305,6 +305,8 @@  static ssize_t dp_test_active_write(struct file *file,
 				debug->panel->video_test = true;
 			else
 				debug->panel->video_test = false;
+
+			dp_panel_tpg_config(debug->panel, debug->panel->video_test);
 		}
 	}
 	drm_connector_list_iter_end(&conn_iter);