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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

Hi,

On 26/08/2019 21:25, Andrey Smirnov wrote:
> Presently, the driver code artificially limits test pattern mode to a
> single pattern with fixed color selection. It being a kernel module
> parameter makes switching "test patter" <-> "proper output" modes
> on-the-fly clunky and outright impossible if the driver is built into
> the kernel.
> 
> To improve the situation a bit, convert current test pattern code to
> use debugfs instead by exposing "TestCtl" register. This way old
> "tc_test_pattern=1" functionality can be emulated via:
> 
>      echo -n 0x78146312 > tstctl
> 
> and switch back to regular mode can be done with:
> 
>      echo -n 0x78146310 > tstctl

It might be worth explaining the format in the commit msg or in a 
comment in the driver.

> Note that switching to any of the test patterns, will NOT trigger link
> re-establishment whereas switching to normal operation WILL. This is
> done so:
> 
> a) we can isolate and verify (e)DP link functionality by switching to
>     one of the test patters
> 
> b) trigger a link re-establishment by switching back to normal mode
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Cory Tusar <cory.tusar@zii.aero>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   drivers/gpu/drm/bridge/tc358767.c | 137 ++++++++++++++++++++++--------
>   1 file changed, 101 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 6308d93ad91d..7a795b613ed0 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -17,6 +17,7 @@
>   
>   #include <linux/bitfield.h>
>   #include <linux/clk.h>
> +#include <linux/debugfs.h>
>   #include <linux/device.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/i2c.h>
> @@ -222,11 +223,10 @@
>   #define COLOR_B			GENMASK(15, 8)
>   #define ENI2CFILTER		BIT(4)
>   #define COLOR_BAR_MODE		GENMASK(1, 0)
> +#define COLOR_BAR_MODE_NORMAL	0
>   #define COLOR_BAR_MODE_BARS	2
> -#define PLL_DBG			0x0a04
>   
> -static bool tc_test_pattern;
> -module_param_named(test, tc_test_pattern, bool, 0644);
> +#define PLL_DBG			0x0a04
>   
>   struct tc_edp_link {
>   	struct drm_dp_link	base;
> @@ -789,16 +789,6 @@ static int tc_set_video_mode(struct tc_data *tc,
>   	if (ret)
>   		return ret;
>   
> -	/* Test pattern settings */
> -	ret = regmap_write(tc->regmap, TSTCTL,
> -			   FIELD_PREP(COLOR_R, 120) |
> -			   FIELD_PREP(COLOR_G, 20) |
> -			   FIELD_PREP(COLOR_B, 99) |
> -			   ENI2CFILTER |
> -			   FIELD_PREP(COLOR_BAR_MODE, COLOR_BAR_MODE_BARS));
> -	if (ret)
> -		return ret;
> -
>   	/* DP Main Stream Attributes */
>   	vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
>   	ret = regmap_write(tc->regmap, DP0_VIDSYNCDELAY,
> @@ -1150,14 +1140,6 @@ static int tc_stream_enable(struct tc_data *tc)
>   
>   	dev_dbg(tc->dev, "enable video stream\n");
>   
> -	/* PXL PLL setup */
> -	if (tc_test_pattern) {
> -		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
> -				    1000 * tc->mode.clock);
> -		if (ret)
> -			return ret;
> -	}
> -
>   	ret = tc_set_video_mode(tc, &tc->mode);
>   	if (ret)
>   		return ret;
> @@ -1186,12 +1168,8 @@ static int tc_stream_enable(struct tc_data *tc)
>   	if (ret)
>   		return ret;
>   	/* Set input interface */
> -	value = DP0_AUDSRC_NO_INPUT;
> -	if (tc_test_pattern)
> -		value |= DP0_VIDSRC_COLOR_BAR;
> -	else
> -		value |= DP0_VIDSRC_DPI_RX;
> -	ret = regmap_write(tc->regmap, SYSCTRL, value);
> +	ret = regmap_write(tc->regmap, SYSCTRL,
> +			   DP0_AUDSRC_NO_INPUT | DP0_VIDSRC_DPI_RX);
>   	if (ret)
>   		return ret;
>   
> @@ -1220,39 +1198,44 @@ static void tc_bridge_pre_enable(struct drm_bridge *bridge)
>   	drm_panel_prepare(tc->panel);
>   }
>   
> -static void tc_bridge_enable(struct drm_bridge *bridge)
> +static int __tc_bridge_enable(struct tc_data *tc)
>   {
> -	struct tc_data *tc = bridge_to_tc(bridge);
>   	int ret;
>   
>   	ret = tc_get_display_props(tc);
>   	if (ret < 0) {
>   		dev_err(tc->dev, "failed to read display props: %d\n", ret);
> -		return;
> +		return ret;
>   	}
>   
>   	ret = tc_main_link_enable(tc);
>   	if (ret < 0) {
>   		dev_err(tc->dev, "main link enable error: %d\n", ret);
> -		return;
> +		return ret;
>   	}
>   
>   	ret = tc_stream_enable(tc);
>   	if (ret < 0) {
>   		dev_err(tc->dev, "main link stream start error: %d\n", ret);
>   		tc_main_link_disable(tc);
> -		return;
>   	}
>   
> -	drm_panel_enable(tc->panel);
> +	return ret;
>   }

Maybe it's just me, but I rather have the last if() block do a "return 
ret"; and have "return 0;" at the end of the function to keep all the if 
blocks consistent.

>   
> -static void tc_bridge_disable(struct drm_bridge *bridge)
> +static void tc_bridge_enable(struct drm_bridge *bridge)
>   {
>   	struct tc_data *tc = bridge_to_tc(bridge);
> -	int ret;
>   
> -	drm_panel_disable(tc->panel);
> +	if (__tc_bridge_enable(tc) < 0)
> +		return;
> +
> +	drm_panel_enable(tc->panel);
> +}
> +
> +static int __tc_bridge_disable(struct tc_data *tc)
> +{
> +	int ret;
>   
>   	ret = tc_stream_disable(tc);
>   	if (ret < 0)
> @@ -1261,6 +1244,16 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
>   	ret = tc_main_link_disable(tc);
>   	if (ret < 0)
>   		dev_err(tc->dev, "main link disable error: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static void tc_bridge_disable(struct drm_bridge *bridge)
> +{
> +	struct tc_data *tc = bridge_to_tc(bridge);
> +
> +	drm_panel_disable(tc->panel);
> +	__tc_bridge_disable(tc);
>   }

Maybe have this enable/disable change as a separate patch?

>   
>   static void tc_bridge_post_disable(struct drm_bridge *bridge)
> @@ -1372,6 +1365,77 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
>   		return connector_status_disconnected;
>   }
>   
> +static int tc_tstctl_set(void *data, u64 val)
> +{
> +	struct tc_data *tc = data;
> +	int ret;
> +
> +	if (FIELD_GET(COLOR_BAR_MODE, val) == COLOR_BAR_MODE_NORMAL) {
> +		ret = regmap_write(tc->regmap, SYSCTRL, DP0_VIDSRC_DPI_RX);
> +		if (ret) {
> +			dev_err(tc->dev,
> +				"failed to select dpi video stream\n");
> +			return ret;
> +		}
> +
> +		ret = regmap_write(tc->regmap, TSTCTL, val | ENI2CFILTER);
> +		if (ret) {
> +			dev_err(tc->dev, "failed to set TSTCTL\n");
> +			return ret;
> +		}
> +
> +		ret = tc_pxl_pll_dis(tc);
> +		if (ret) {
> +			dev_err(tc->dev, "failed to disable PLL\n");
> +			return ret;
> +		}
> +
> +		/*
> +		 * Re-establish DP link
> +		 */
> +		ret = __tc_bridge_disable(tc);
> +		if (ret)
> +			return ret;
> +
> +		ret = __tc_bridge_enable(tc);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
> +				    1000 * tc->mode.clock);
> +		if (ret) {
> +			dev_err(tc->dev, "failed to enable PLL\n");
> +			return ret;
> +		}
> +
> +		ret = regmap_write(tc->regmap, TSTCTL, val | ENI2CFILTER);
> +		if (ret) {
> +			dev_err(tc->dev, "failed to set TSTCTL\n");
> +			return ret;
> +		}
> +
> +		ret = regmap_write(tc->regmap, SYSCTRL, DP0_VIDSRC_COLOR_BAR);
> +		if (ret) {
> +			dev_err(tc->dev, "failed to color bar video stream\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(tc_tstctl_fops, NULL, tc_tstctl_set, "%llu\n");
> +
> +static int tc_late_register(struct drm_connector *connector)
> +{
> +	if (connector->debugfs_entry)
> +		debugfs_create_file_unsafe("tstctl", 0644,
> +					   connector->debugfs_entry,
> +					   connector_to_tc(connector),
> +					   &tc_tstctl_fops);
> +	return 0;
> +}

I very recently wanted to add quick debugfs functionality to a bridge 
too, but as there didn't seem an easy way to do it, I just lazily added 
the debugfs files to the debugfs root...

I'm not sure if adding bridge's debugfs files to connector's directory 
is a good idea. What if we have two bridges, both have the same debugfs 
file? Or even if there's no conflict, there's also no way to know which 
bridge a particular debugfs file belongs to.

Maybe we need some DRM infrastructure for bridge debugfs?

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki