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
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 | 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;
}
-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;
+ } 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;
+}
+
static const struct drm_connector_funcs tc_connector_funcs = {
.detect = tc_connector_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
@@ -1379,6 +1443,7 @@ static const struct drm_connector_funcs tc_connector_funcs = {
.reset = drm_atomic_helper_connector_reset,
.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+ .late_register = tc_late_register,
};
static int tc_bridge_attach(struct drm_bridge *bridge)
--
2.21.0
Hi Andrey,
Thank you for the patch.
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 ? Do all bits in the register need to be controlled
from userspace ?
> 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 | 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 ?
> +#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.
> + } 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 ?
> +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 ?
> static const struct drm_connector_funcs tc_connector_funcs = {
> .detect = tc_connector_detect,
> .fill_modes = drm_helper_probe_single_connector_modes,
> @@ -1379,6 +1443,7 @@ static const struct drm_connector_funcs tc_connector_funcs = {
> .reset = drm_atomic_helper_connector_reset,
> .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> + .late_register = tc_late_register,
> };
>
> static int tc_bridge_attach(struct drm_bridge *bridge)
--
Regards,
Laurent Pinchart
On Mon, Aug 26, 2019 at 3:08 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Andrey,
>
> Thank you for the patch.
>
> 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.
> 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 <[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 | 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.
Thanks,
Andrey Smirnov
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 <[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 | 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
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 <[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 | 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
On Tue, Aug 27, 2019 at 1:06 AM Laurent Pinchart
<[email protected]> wrote:
>
> 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 ? :-)
Is this a rhetoric question or are you seriously asking? If its the
latter I can ping you off the list.
> The whole point of a
> driver is to avoid needing detailed knowledge of the device's internals
> in userspace.
>
You won't avoid needing detailed knowledge of the device's internals
if you don't have a priori knowledge in the form of a agreed upon/well
known abstraction you are exposing from the driver. There is no such
abstraction in this case. Whether you present "tstctl" that takes a
magic value or "red", "green", "blue" and "pattern" taking numbers and
special strings, as a user, you still would have to go read the driver
code in order to figure out how that stuff works.
Given how this is an obscure _debug_ feature for a niche part, I think
exposing raw register and leaving a comment in the driver source code
explaining how it works is reasonably user-friendly (for all 10 - 15
unique users that this feature would ever have).
To avoid any further back and forth of this subject, how about the
following. If this is up to me, then I'd like to move forward to v2
with the interface as is. If you feel strongly about this and insist
on your vision of the interface, please let me know what it looks like
(e.g. is what I described above good enough) and I'll rework v2 to
have that.
> > > 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 <[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 | 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.
>
Sure, I can move it to probe in v2.
Thanks,
Andrey Smirnov
On Mon, Aug 26, 2019 at 10:46 PM Tomi Valkeinen <[email protected]> wrote:
>
> 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.
>
Good point. Will do if this is the format going forward.
> > 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 | 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.
OK, will fix in v2.
>
> >
> > -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?
>
Sure, I can do that in v2.
> >
> > 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?
>
I'll move to using a dedicated entry in debugfs, which should address
all of those concerns.
Thanks,
Andrey Smirnov
On 28/08/2019 01:51, Andrey Smirnov wrote:
>> The whole point of a
>> driver is to avoid needing detailed knowledge of the device's internals
>> in userspace.
>>
>
> You won't avoid needing detailed knowledge of the device's internals
> if you don't have a priori knowledge in the form of a agreed upon/well
> known abstraction you are exposing from the driver. There is no such
> abstraction in this case. Whether you present "tstctl" that takes a
> magic value or "red", "green", "blue" and "pattern" taking numbers and
> special strings, as a user, you still would have to go read the driver
> code in order to figure out how that stuff works.
>
> Given how this is an obscure _debug_ feature for a niche part, I think
> exposing raw register and leaving a comment in the driver source code
> explaining how it works is reasonably user-friendly (for all 10 - 15
> unique users that this feature would ever have).
>
> To avoid any further back and forth of this subject, how about the
> following. If this is up to me, then I'd like to move forward to v2
> with the interface as is. If you feel strongly about this and insist
> on your vision of the interface, please let me know what it looks like
> (e.g. is what I described above good enough) and I'll rework v2 to
> have that.
I agree, I don't see a point in adding a pile of code to make a device
specific debug feature to hide the device internals. If someone is going
to use this feature, most likely he either has the datasheet or he has
been asked by someone with the datasheet to try the feature.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki