Messages in this thread Patch in this message | | | Subject | Re: [PATCH] drm/bridge: tc358767: Expose test mode functionality via debugfs | From | Tomi Valkeinen <> | Date | Tue, 27 Aug 2019 08:46:13 +0300 |
| |
Hi,
On 26/08/2019 21:25, 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
It might be worth explaining the format in the commit msg or in a comment in the driver.
> 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@gmail.com> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > Cc: Cory Tusar <cory.tusar@zii.aero> > Cc: Chris Healy <cphealy@gmail.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: dri-devel@lists.freedesktop.org > Cc: linux-kernel@vger.kernel.org > --- > 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); > +#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; > }
Maybe it's just me, but I rather have the last if() block do a "return ret"; and have "return 0;" at the end of the function to keep all the if blocks consistent.
> > -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); > }
Maybe have this enable/disable change as a separate patch?
> > 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; > + } 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"); > + > +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; > +}
I very recently wanted to add quick debugfs functionality to a bridge too, but as there didn't seem an easy way to do it, I just lazily added the debugfs files to the debugfs root...
I'm not sure if adding bridge's debugfs files to connector's directory is a good idea. What if we have two bridges, both have the same debugfs file? Or even if there's no conflict, there's also no way to know which bridge a particular debugfs file belongs to.
Maybe we need some DRM infrastructure for bridge debugfs?
Tomi
-- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
| |