Re: [PATCH] drm/bridge: tc358767: Expose test mode functionality via debugfs

From: Laurent Pinchart
Date: Tue Aug 27 2019 - 04:06:59 EST

Next message: Greg Kroah-Hartman: "[PATCH 5.2 150/162] mm, page_owner: handle THP splits correctly"
Previous message: Greg Kroah-Hartman: "[PATCH 5.2 159/162] io_uring: add need_resched() check in inner poll loop"
In reply to: Andrey Smirnov: "Re: [PATCH] drm/bridge: tc358767: Expose test mode functionality via debugfs"
Next in thread: Andrey Smirnov: "Re: [PATCH] drm/bridge: tc358767: Expose test mode functionality via debugfs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Andrey,

On Mon, Aug 26, 2019 at 09:24:57PM -0700, Andrey Smirnov wrote:
> On Mon, Aug 26, 2019 at 3:08 PM Laurent Pinchart wrote:
> > On Mon, Aug 26, 2019 at 11:25:24AM -0700, Andrey Smirnov wrote:
> > > Presently, the driver code artificially limits test pattern mode to a
> > > single pattern with fixed color selection. It being a kernel module
> > > parameter makes switching "test patter" <-> "proper output" modes
> > > on-the-fly clunky and outright impossible if the driver is built into
> > > the kernel.
> > >
> > > To improve the situation a bit, convert current test pattern code to
> > > use debugfs instead by exposing "TestCtl" register. This way old
> > > "tc_test_pattern=1" functionality can be emulated via:
> > >
> > > echo -n 0x78146312 > tstctl
> > >
> > > and switch back to regular mode can be done with:
> > >
> > > echo -n 0x78146310 > tstctl
> >
> > Can't we make this more userfriendly by exposing either a test pattern
> > index, or a string ?
>
> We could, but then a) it would require more code in the driver b) the
> files wouldn't correspond directly to something described in the
> part's datasheet. Just didn't seem worth it to me.

Could you then provide me with the datasheet ? :-) The whole point of a
driver is to avoid needing detailed knowledge of the device's internals
in userspace.

> > Do all bits in the register need to be controlled
> > from userspace ?
>
> Pretty much, yes. It's formatted as RR_GG_BB_X_M, where R, G, B
> specifies color used for various patterns, X is irrelevant and M
> specifies test pattern to use.
>
> > > Note that switching to any of the test patterns, will NOT trigger link
> > > re-establishment whereas switching to normal operation WILL. This is
> > > done so:
> > >
> > > a) we can isolate and verify (e)DP link functionality by switching to
> > > one of the test patters
> > >
> > > b) trigger a link re-establishment by switching back to normal mode
> > >
> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> > > Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> > > Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > Cc: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> > > Cc: Cory Tusar <cory.tusar@xxxxxxxx>
> > > Cc: Chris Healy <cphealy@xxxxxxxxx>
> > > Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > > ---
> > > drivers/gpu/drm/bridge/tc358767.c | 137 ++++++++++++++++++++++--------
> > > 1 file changed, 101 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > > index 6308d93ad91d..7a795b613ed0 100644
> > > --- a/drivers/gpu/drm/bridge/tc358767.c
> > > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > > @@ -17,6 +17,7 @@
> > >
> > > #include <linux/bitfield.h>
> > > #include <linux/clk.h>
> > > +#include <linux/debugfs.h>
> > > #include <linux/device.h>
> > > #include <linux/gpio/consumer.h>
> > > #include <linux/i2c.h>
> > > @@ -222,11 +223,10 @@
> > > #define COLOR_B GENMASK(15, 8)
> > > #define ENI2CFILTER BIT(4)
> > > #define COLOR_BAR_MODE GENMASK(1, 0)
> > > +#define COLOR_BAR_MODE_NORMAL 0
> > > #define COLOR_BAR_MODE_BARS 2
> > > -#define PLL_DBG 0x0a04
> > >
> > > -static bool tc_test_pattern;
> > > -module_param_named(test, tc_test_pattern, bool, 0644);
> >
> > I assume that his being a debug feature there's no system relying on the
> > module parameter that would break if you remove it ?
>
> Yeah, I don't know of any system that needs that parameter. It seems
> pretty useless for anything but basic debugging.
>
> > > +#define PLL_DBG 0x0a04
> > >
> > > struct tc_edp_link {
> > > struct drm_dp_link base;
> > > @@ -789,16 +789,6 @@ static int tc_set_video_mode(struct tc_data *tc,
> > > if (ret)
> > > return ret;
> > >
> > > - /* Test pattern settings */
> > > - ret = regmap_write(tc->regmap, TSTCTL,
> > > - FIELD_PREP(COLOR_R, 120) |
> > > - FIELD_PREP(COLOR_G, 20) |
> > > - FIELD_PREP(COLOR_B, 99) |
> > > - ENI2CFILTER |
> > > - FIELD_PREP(COLOR_BAR_MODE, COLOR_BAR_MODE_BARS));
> > > - if (ret)
> > > - return ret;
> > > -
> > > /* DP Main Stream Attributes */
> > > vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
> > > ret = regmap_write(tc->regmap, DP0_VIDSYNCDELAY,
> > > @@ -1150,14 +1140,6 @@ static int tc_stream_enable(struct tc_data *tc)
> > >
> > > dev_dbg(tc->dev, "enable video stream\n");
> > >
> > > - /* PXL PLL setup */
> > > - if (tc_test_pattern) {
> > > - ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
> > > - 1000 * tc->mode.clock);
> > > - if (ret)
> > > - return ret;
> > > - }
> > > -
> > > ret = tc_set_video_mode(tc, &tc->mode);
> > > if (ret)
> > > return ret;
> > > @@ -1186,12 +1168,8 @@ static int tc_stream_enable(struct tc_data *tc)
> > > if (ret)
> > > return ret;
> > > /* Set input interface */
> > > - value = DP0_AUDSRC_NO_INPUT;
> > > - if (tc_test_pattern)
> > > - value |= DP0_VIDSRC_COLOR_BAR;
> > > - else
> > > - value |= DP0_VIDSRC_DPI_RX;
> > > - ret = regmap_write(tc->regmap, SYSCTRL, value);
> > > + ret = regmap_write(tc->regmap, SYSCTRL,
> > > + DP0_AUDSRC_NO_INPUT | DP0_VIDSRC_DPI_RX);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -1220,39 +1198,44 @@ static void tc_bridge_pre_enable(struct drm_bridge *bridge)
> > > drm_panel_prepare(tc->panel);
> > > }
> > >
> > > -static void tc_bridge_enable(struct drm_bridge *bridge)
> > > +static int __tc_bridge_enable(struct tc_data *tc)
> > > {
> > > - struct tc_data *tc = bridge_to_tc(bridge);
> > > int ret;
> > >
> > > ret = tc_get_display_props(tc);
> > > if (ret < 0) {
> > > dev_err(tc->dev, "failed to read display props: %d\n", ret);
> > > - return;
> > > + return ret;
> > > }
> > >
> > > ret = tc_main_link_enable(tc);
> > > if (ret < 0) {
> > > dev_err(tc->dev, "main link enable error: %d\n", ret);
> > > - return;
> > > + return ret;
> > > }
> > >
> > > ret = tc_stream_enable(tc);
> > > if (ret < 0) {
> > > dev_err(tc->dev, "main link stream start error: %d\n", ret);
> > > tc_main_link_disable(tc);
> > > - return;
> > > }
> > >
> > > - drm_panel_enable(tc->panel);
> > > + return ret;
> > > }
> > >
> > > -static void tc_bridge_disable(struct drm_bridge *bridge)
> > > +static void tc_bridge_enable(struct drm_bridge *bridge)
> > > {
> > > struct tc_data *tc = bridge_to_tc(bridge);
> > > - int ret;
> > >
> > > - drm_panel_disable(tc->panel);
> > > + if (__tc_bridge_enable(tc) < 0)
> > > + return;
> > > +
> > > + drm_panel_enable(tc->panel);
> > > +}
> > > +
> > > +static int __tc_bridge_disable(struct tc_data *tc)
> > > +{
> > > + int ret;
> > >
> > > ret = tc_stream_disable(tc);
> > > if (ret < 0)
> > > @@ -1261,6 +1244,16 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
> > > ret = tc_main_link_disable(tc);
> > > if (ret < 0)
> > > dev_err(tc->dev, "main link disable error: %d\n", ret);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void tc_bridge_disable(struct drm_bridge *bridge)
> > > +{
> > > + struct tc_data *tc = bridge_to_tc(bridge);
> > > +
> > > + drm_panel_disable(tc->panel);
> > > + __tc_bridge_disable(tc);
> > > }
> > >
> > > static void tc_bridge_post_disable(struct drm_bridge *bridge)
> > > @@ -1372,6 +1365,77 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
> > > return connector_status_disconnected;
> > > }
> > >
> > > +static int tc_tstctl_set(void *data, u64 val)
> > > +{
> > > + struct tc_data *tc = data;
> > > + int ret;
> > > +
> > > + if (FIELD_GET(COLOR_BAR_MODE, val) == COLOR_BAR_MODE_NORMAL) {
> > > + ret = regmap_write(tc->regmap, SYSCTRL, DP0_VIDSRC_DPI_RX);
> > > + if (ret) {
> > > + dev_err(tc->dev,
> > > + "failed to select dpi video stream\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = regmap_write(tc->regmap, TSTCTL, val | ENI2CFILTER);
> > > + if (ret) {
> > > + dev_err(tc->dev, "failed to set TSTCTL\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = tc_pxl_pll_dis(tc);
> > > + if (ret) {
> > > + dev_err(tc->dev, "failed to disable PLL\n");
> > > + return ret;
> > > + }
> > > +
> > > + /*
> > > + * Re-establish DP link
> > > + */
> > > + ret = __tc_bridge_disable(tc);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = __tc_bridge_enable(tc);
> > > + if (ret)
> > > + return ret;
> >
> > If the bridge is currently disabled you should not enable it. I think
> > you need to guard against race conditions with the enable/disable
> > functions, as well as delay writes until the bridge gets enabled if it
> > is currently disabled.
>
> OK, will do in v2.
>
> > > + } else {
> > > + ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
> > > + 1000 * tc->mode.clock);
> > > + if (ret) {
> > > + dev_err(tc->dev, "failed to enable PLL\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = regmap_write(tc->regmap, TSTCTL, val | ENI2CFILTER);
> > > + if (ret) {
> > > + dev_err(tc->dev, "failed to set TSTCTL\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = regmap_write(tc->regmap, SYSCTRL, DP0_VIDSRC_COLOR_BAR);
> > > + if (ret) {
> > > + dev_err(tc->dev, "failed to color bar video stream\n");
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +DEFINE_SIMPLE_ATTRIBUTE(tc_tstctl_fops, NULL, tc_tstctl_set, "%llu\n");
> >
> > Shouldn't you also provide a get handler ?
>
> There really isn't a use-case for reading the value of TestCtl AFAICT,
> so, I skipped it to avoid having extra code.
>
> > > +static int tc_late_register(struct drm_connector *connector)
> > > +{
> > > + if (connector->debugfs_entry)
> > > + debugfs_create_file_unsafe("tstctl", 0644,
> > > + connector->debugfs_entry,
> > > + connector_to_tc(connector),
> > > + &tc_tstctl_fops);
> > > + return 0;
> > > +}
> >
> > Why a .late_register() handler, can't you do this at probe time ?
>
> .late_register() happens after drm_debugfs_connector_add() where
> connector->debugfs_entry gets initialized. AFAICT, I could move that
> code to probe, but then I'd have create a separate debugfs root
> directory as well as add code to clean it up (currently I rely on
> drm_debugfs_connector_remove() removing everything recursively.

The usual approach for this kind of debug feature is to create a
directory named after dev_name() in order to guarantee uniqueness. I
feel that could be better, but have no strong preference.

--
Regards,

Laurent Pinchart

Next message: Greg Kroah-Hartman: "[PATCH 5.2 150/162] mm, page_owner: handle THP splits correctly"
Previous message: Greg Kroah-Hartman: "[PATCH 5.2 159/162] io_uring: add need_resched() check in inner poll loop"
In reply to: Andrey Smirnov: "Re: [PATCH] drm/bridge: tc358767: Expose test mode functionality via debugfs"
Next in thread: Andrey Smirnov: "Re: [PATCH] drm/bridge: tc358767: Expose test mode functionality via debugfs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]