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 <[email protected]>
Cc: Andrzej Hajda <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Tomi Valkeinen <[email protected]>
Cc: Cory Tusar <[email protected]>
Cc: Chris Healy <[email protected]>
Cc: Lucas Stach <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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;
}
--
2.21.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 > tstctl
>
> and 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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Mon, Dec 9, 2019 at 1:38 AM Tomi Valkeinen <[email protected]> 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 > tstctl
> >
> > and 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.
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 <[email protected]> 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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Mon, Dec 9, 2019 at 7:05 AM Tomi Valkeinen <[email protected]> wrote:
>
> On 09/12/2019 16:38, Andrey Smirnov wrote:
> > On Mon, Dec 9, 2019 at 1:38 AM Tomi Valkeinen <[email protected]> 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