Everyone:
This series is a couple of patches exposing TestCtl register of tc358767, which can be pretty handy when troubleshooting link problems.
Changes since [v2]:
- Series rebased on 5.4 kernel
Changes since [v1]:
- Debugfs moved into a standalone directory and is now created as a part of probe()
- Added tstctl_lock to ensure mutual exclusion of tstctl code and bridge's enable/disable methods
- tc_tstctl_set() changed to function only if bridge was previosly enabled
- Added comment explaining data format expected by "tstctl"
- Debugfs permission changed to reflect write-only nature of this feature
- Original commit split into two
- Minor formatting changes
Thanks, Andrey Smirnov
[v1] lore.kernel.org/r/20190826182524.5064-1-andrew.smirnov@gmail.com [v2] lore.kernel.org/r/20190912013740.5638-1-andrew.smirnov@gmail.com
Andrey Smirnov (2): drm/bridge: tc358767: Introduce __tc_bridge_enable/disable() drm/bridge: tc358767: Expose test mode functionality via debugfs
drivers/gpu/drm/bridge/tc358767.c | 184 ++++++++++++++++++++++++------ 1 file changed, 148 insertions(+), 36 deletions(-)
Expose underlying implementation of bridge's enable/disable functions, so it would be possible to use them in other parts of the driver.
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 | 32 ++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 8a8d605021f0..3c252ae0ee6f 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1222,39 +1222,43 @@ 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; + return ret; }
- drm_panel_enable(tc->panel); + return 0; }
-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)) + drm_panel_enable(tc->panel); +} + +static int __tc_bridge_disable(struct tc_data *tc) +{ + int ret;
ret = tc_stream_disable(tc); if (ret < 0) @@ -1263,6 +1267,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)
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 pattern" <-> "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 0x78146302 > tstctl
and switch back to regular mode can be done with:
echo -n 0x78146300 > tstctl
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 | 152 ++++++++++++++++++++++++------ 1 file changed, 125 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 3c252ae0ee6f..12a8829e0ed1 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> @@ -221,11 +222,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; @@ -263,6 +263,9 @@ struct tc_data {
/* HPD pin number (0 or 1) or -ENODEV */ int hpd_pin; + + struct mutex tstctl_lock; + bool enabled; };
static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a) @@ -791,16 +794,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, @@ -1152,14 +1145,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; @@ -1188,12 +1173,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;
@@ -1252,8 +1233,13 @@ static void tc_bridge_enable(struct drm_bridge *bridge) { struct tc_data *tc = bridge_to_tc(bridge);
+ mutex_lock(&tc->tstctl_lock); + if (!__tc_bridge_enable(tc)) drm_panel_enable(tc->panel); + + tc->enabled = true; + mutex_unlock(&tc->tstctl_lock); }
static int __tc_bridge_disable(struct tc_data *tc) @@ -1275,8 +1261,13 @@ static void tc_bridge_disable(struct drm_bridge *bridge) { struct tc_data *tc = bridge_to_tc(bridge);
+ mutex_lock(&tc->tstctl_lock); + drm_panel_disable(tc->panel); __tc_bridge_disable(tc); + + tc->enabled = false; + mutex_unlock(&tc->tstctl_lock); }
static void tc_bridge_post_disable(struct drm_bridge *bridge) @@ -1388,6 +1379,99 @@ 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; + + mutex_lock(&tc->tstctl_lock); + + if (!tc->enabled) { + dev_err(tc->dev, "bridge needs to be enabled first\n"); + ret = -EBUSY; + goto unlock; + } + + 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"); + goto unlock; + } + + ret = regmap_write(tc->regmap, TSTCTL, val | ENI2CFILTER); + if (ret) { + dev_err(tc->dev, "failed to set TSTCTL\n"); + goto unlock; + } + + ret = tc_pxl_pll_dis(tc); + if (ret) { + dev_err(tc->dev, "failed to disable PLL\n"); + goto unlock; + } + + /* + * Re-establish DP link + */ + ret = __tc_bridge_disable(tc); + if (ret) + goto unlock; + + ret = __tc_bridge_enable(tc); + if (ret) + goto unlock; + } 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"); + goto unlock; + } + + ret = regmap_write(tc->regmap, TSTCTL, val | ENI2CFILTER); + if (ret) { + dev_err(tc->dev, "failed to set TSTCTL\n"); + goto unlock; + } + + ret = regmap_write(tc->regmap, SYSCTRL, DP0_VIDSRC_COLOR_BAR); + if (ret) { + dev_err(tc->dev, "failed to set SYSCTRL\n"); + goto unlock; + } + } + +unlock: + mutex_unlock(&tc->tstctl_lock); + return ret; +} +/* + * "tstctl" attribute has the following format: + * + * RR_GG_BB_0_P + * + * "RR" is red, "GG" is green and "BB" is blue color components (byte + * each) used for various test patterns controlled by this register. + * + * "P" represents test pattern of the bridge. Valid values are: + * + * "0" - normal operation + * "1" - solid color test pattern + * "2" - color bar test pattern + * "3" - "checkers" test pattern + * + * This way old "tc_test_pattern=1" functionality can be emulated via: + * + * echo -n 0x78146302 > tstctl + * + * and switch back to regular mode can be done with: + * + * echo -n 0 > tstctl + */ +DEFINE_SIMPLE_ATTRIBUTE(tc_tstctl_fops, NULL, tc_tstctl_set, "%llu\n"); + static const struct drm_connector_funcs tc_connector_funcs = { .detect = tc_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, @@ -1530,9 +1614,15 @@ static irqreturn_t tc_irq_handler(int irq, void *arg) return IRQ_HANDLED; }
+static void tc_remove_debugfs(void *data) +{ + debugfs_remove_recursive(data); +} + static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct device *dev = &client->dev; + struct dentry *debugfs; struct tc_data *tc; int ret;
@@ -1541,6 +1631,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) return -ENOMEM;
tc->dev = dev; + mutex_init(&tc->tstctl_lock);
/* port@2 is the output port */ ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &tc->panel, NULL); @@ -1671,6 +1762,13 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
i2c_set_clientdata(client, tc);
+ debugfs = debugfs_create_dir(dev_name(dev), NULL); + if (!IS_ERR(debugfs)) { + debugfs_create_file_unsafe("tstctl", 0200, debugfs, tc, + &tc_tstctl_fops); + devm_add_action_or_reset(dev, tc_remove_debugfs, debugfs); + } + return 0; }
(Cc'ing Daniel for the last paragraph)
On 09/12/2019 07:08, 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 pattern" <-> "proper output" modes on-the-fly clunky and outright impossible if the driver is built into the kernel.
That's not correct, /sys/module/tc358767/parameters/test is there even if the driver is built-in.
I think the bigger problems are that there's just one value, even if there are multiple devices, and that with kernel parameter the driver can't act on it dynamically (afaik).
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 0x78146302 > tstctland switch back to regular mode can be done with:
echo -n 0x78146300 > tstctl
In the comment in the code you have 0 as return-to-regular-mode.
With my setup, enabling test mode seems to work, but when I return to regular mode, the first echo results in black display, but echoing 0 a second time will restore the display.
Hmm, actually, just echoing 0 to tstctl multiple times, it makes the screen go black and then restores it with every other echo.
- debugfs = debugfs_create_dir(dev_name(dev), NULL);
- if (!IS_ERR(debugfs)) {
debugfs_create_file_unsafe("tstctl", 0200, debugfs, tc,&tc_tstctl_fops);devm_add_action_or_reset(dev, tc_remove_debugfs, debugfs);- }
For me this creates debugfs/3-000f/tstctl. I don't think that's a clear or usable path, and could even cause a name conflict in the worst case.
Not sure what's a good solution here, but only two semi-good ones come to mind: have it in sysfs under the device's dir, or debugfs/dri/something. The latter probably needs some thought and common agreement on how to handle bridge and panel debugfs files. But that would be a good thing to have, as I'm sure there are other similar cases (at least I have a few).
Tomi
On Mon, Dec 9, 2019 at 1:38 AM Tomi Valkeinen tomi.valkeinen@ti.com wrote:
(Cc'ing Daniel for the last paragraph)
On 09/12/2019 07:08, 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 pattern" <-> "proper output" modes on-the-fly clunky and outright impossible if the driver is built into the kernel.
That's not correct, /sys/module/tc358767/parameters/test is there even if the driver is built-in.
True, I'll drop the "impossible" part of the descrption. Having to unbind and bind device to the driver to use that parameter definitely falls under "clunky" for me still, so I'll just stick to that in the description.
I think the bigger problems are that there's just one value, even if there are multiple devices, and that with kernel parameter the driver can't act on it dynamically (afaik).
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 0x78146302 > tstctland switch back to regular mode can be done with:
echo -n 0x78146300 > tstctlIn the comment in the code you have 0 as return-to-regular-mode.
Both should work, but I'll modify commit message to match the code.
With my setup, enabling test mode seems to work, but when I return to regular mode, the first echo results in black display, but echoing 0 a second time will restore the display.
Hmm, actually, just echoing 0 to tstctl multiple times, it makes the screen go black and then restores it with every other echo.
Strange, works on my setup every time. No error messages in kernel log I assume? Probably unrelated, but when you echo "0" and the screen stays black, what do you see in DP_SINK_STATUS register:
dd if=/dev/drm_dp_aux0 bs=1 skip=$((0x205)) count=1 2>/dev/null | hexdump -Cv
? Note that this needs CONFIG_DRM_DP_AUX_CHARDEV to be enabled.
debugfs = debugfs_create_dir(dev_name(dev), NULL);if (!IS_ERR(debugfs)) {debugfs_create_file_unsafe("tstctl", 0200, debugfs, tc,&tc_tstctl_fops);devm_add_action_or_reset(dev, tc_remove_debugfs, debugfs);}For me this creates debugfs/3-000f/tstctl. I don't think that's a clear or usable path, and could even cause a name conflict in the worst case.
I agree on usability aspect, but I am not sure I can see how a conflict can happen. What scenario do you have in mind that would cause that? My thinking was that the combination of I2C bus number + I2C address should always be unique on the system, but maybe I am missing something?
Not sure what's a good solution here, but only two semi-good ones come to mind: have it in sysfs under the device's dir,
I'm fine with this option if it is the only path forward, but, given a choice, I would _really_ rather not go the sysfs route.
Thanks, Andrey Smirnov
On 09/12/2019 16:38, Andrey Smirnov wrote:
On Mon, Dec 9, 2019 at 1:38 AM Tomi Valkeinen tomi.valkeinen@ti.com wrote:
(Cc'ing Daniel for the last paragraph)
On 09/12/2019 07:08, 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 pattern" <-> "proper output" modes on-the-fly clunky and outright impossible if the driver is built into the kernel.
That's not correct, /sys/module/tc358767/parameters/test is there even if the driver is built-in.
True, I'll drop the "impossible" part of the descrption. Having to unbind and bind device to the driver to use that parameter definitely falls under "clunky" for me still, so I'll just stick to that in the description.
You don't need to re-bind. You can change the module parameter at runtime, and if the driver happens to use the value, then it uses the new value. If I recall right, changing the module parameter and then doing a full modeset from userspace made the driver to use the test mode (I'm not 100% sure, though).
In any case, I'm not advocating for the use of module parameter here =)
Hmm, actually, just echoing 0 to tstctl multiple times, it makes the screen go black and then restores it with every other echo.
Strange, works on my setup every time. No error messages in kernel log I assume? Probably unrelated, but when you echo "0" and the screen
No errors.
stays black, what do you see in DP_SINK_STATUS register:
dd if=/dev/drm_dp_aux0 bs=1 skip=$((0x205)) count=1 2>/dev/null | hexdump -Cv
? Note that this needs CONFIG_DRM_DP_AUX_CHARDEV to be enabled.
I'll check this later, and do a few more tests.
debugfs = debugfs_create_dir(dev_name(dev), NULL);if (!IS_ERR(debugfs)) {debugfs_create_file_unsafe("tstctl", 0200, debugfs, tc,&tc_tstctl_fops);devm_add_action_or_reset(dev, tc_remove_debugfs, debugfs);}For me this creates debugfs/3-000f/tstctl. I don't think that's a clear or usable path, and could even cause a name conflict in the worst case.
I agree on usability aspect, but I am not sure I can see how a conflict can happen. What scenario do you have in mind that would cause that? My thinking was that the combination of I2C bus number + I2C address should always be unique on the system, but maybe I am missing something?
Well, the dir name doesn't have "i2c" anywhere, so at least in theory, some other bus could have "3-000f" address too.
Maybe bigger problem is that it's not at all obvious what "3-000f" means. All the other debugfs dirs make sense when you look at the name, and "3-000f" looks very odd there.
Tomi
On Mon, Dec 9, 2019 at 7:05 AM Tomi Valkeinen tomi.valkeinen@ti.com wrote:
On 09/12/2019 16:38, Andrey Smirnov wrote:
On Mon, Dec 9, 2019 at 1:38 AM Tomi Valkeinen tomi.valkeinen@ti.com wrote:
(Cc'ing Daniel for the last paragraph)
On 09/12/2019 07:08, 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 pattern" <-> "proper output" modes on-the-fly clunky and outright impossible if the driver is built into the kernel.
That's not correct, /sys/module/tc358767/parameters/test is there even if the driver is built-in.
True, I'll drop the "impossible" part of the descrption. Having to unbind and bind device to the driver to use that parameter definitely falls under "clunky" for me still, so I'll just stick to that in the description.
You don't need to re-bind. You can change the module parameter at runtime, and if the driver happens to use the value, then it uses the new value. If I recall right, changing the module parameter and then doing a full modeset from userspace made the driver to use the test mode (I'm not 100% sure, though).
In any case, I'm not advocating for the use of module parameter here =)
Hmm, actually, just echoing 0 to tstctl multiple times, it makes the screen go black and then restores it with every other echo.
Strange, works on my setup every time. No error messages in kernel log I assume? Probably unrelated, but when you echo "0" and the screen
No errors.
stays black, what do you see in DP_SINK_STATUS register:
dd if=/dev/drm_dp_aux0 bs=1 skip=$((0x205)) count=1 2>/dev/null | hexdump -Cv
? Note that this needs CONFIG_DRM_DP_AUX_CHARDEV to be enabled.
I'll check this later, and do a few more tests.
debugfs = debugfs_create_dir(dev_name(dev), NULL);if (!IS_ERR(debugfs)) {debugfs_create_file_unsafe("tstctl", 0200, debugfs, tc,&tc_tstctl_fops);devm_add_action_or_reset(dev, tc_remove_debugfs, debugfs);}For me this creates debugfs/3-000f/tstctl. I don't think that's a clear or usable path, and could even cause a name conflict in the worst case.
I agree on usability aspect, but I am not sure I can see how a conflict can happen. What scenario do you have in mind that would cause that? My thinking was that the combination of I2C bus number + I2C address should always be unique on the system, but maybe I am missing something?
Well, the dir name doesn't have "i2c" anywhere, so at least in theory, some other bus could have "3-000f" address too.
Maybe bigger problem is that it's not at all obvious what "3-000f" means. All the other debugfs dirs make sense when you look at the name, and "3-000f" looks very odd there.
Fair enough, so what if we changed the name say "tc358767-3-000f" (i. e. used "tc358767-" + dev_name(dev)), would that be a reasonable path forward?
Thanks, Andrey Smirnov
dri-devel@lists.freedesktop.org
-
Andrey Smirnov -
Tomi Valkeinen