[PATCH v3 0/6] drm/panel: simple: Add mode support to devicetree

[PATCH v3 0/6] drm/panel: simple: Add mode support to devicetree

[Sean Paul]

Hello!

Here's v3 of the set. I've reintroduced the single display timing node
in the dt binding from v1, and left the improved docs and patch split
from v2. I've also added a patch to clarify that a single display timing
node should always be named panel-timing, which was feedback from v1.

Cover letter from v1 (was the same in v2) is here:
https://lists.freedesktop.org/archives/dri-devel/2018-February/164886.html

Thanks for the reviews so far,

Sean


Sean Paul (6):
  dt-bindings: Clarify timing subnode use as panel-timing
  dt-bindings: Add headings to simple-panel bindings
  dt-bindings: Add panel-timing subnode to simple-panel
  drm/panel: simple: Add ability to override typical timing
  drm/panel: simple: Use display_timing for lq123p1jx31
  arm64: dts: rockchip: Specify override mode for kevin panel

 .../bindings/display/panel/display-timing.txt      |  5 ++
 .../bindings/display/panel/simple-panel.txt        | 34 ++++++++
 arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts  | 14 ++++
 drivers/gpu/drm/panel/panel-simple.c               | 94 ++++++++++++++++++----
 4 files changed, 132 insertions(+), 15 deletions(-)

-- 
2.16.0.rc1.238.g530d649a79-goog

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/6] dt-bindings: Clarify timing subnode use as panel-timing

[Sean Paul]

Add a note in the documentation explaining when it's appropriate to use
the display-timings subnode on its own, as well as the preferred name to
use (panel-timing).

Changes in v3:
 - Added

Cc: Doug Anderson <dianders@chromium.org>
Cc: Eric Anholt <eric@anholt.net>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stéphane Marchesin <marcheu@chromium.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: devicetree@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-rockchip@lists.infradead.org
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 Documentation/devicetree/bindings/display/panel/display-timing.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/display-timing.txt b/Documentation/devicetree/bindings/display/panel/display-timing.txt
index 58fa3e48481d..78222ced1874 100644
--- a/Documentation/devicetree/bindings/display/panel/display-timing.txt
+++ b/Documentation/devicetree/bindings/display/panel/display-timing.txt
@@ -80,6 +80,11 @@ The parameters are defined as:
   |          |        v                            |          |       |
   +----------+-------------------------------------+----------+-------+
 
+Note: In addition to being used as subnode(s) of display-timings, the timing
+      subnode may also be used on its own. This is appropriate if only one mode
+      need be conveyed. In this case, the node should be named 'panel-timing'.
+
+
 Example:
 
 	display-timings {
-- 
2.16.0.rc1.238.g530d649a79-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/6] dt-bindings: Add headings to simple-panel bindings

[Sean Paul]

In preparation for a new subnode section in a follow-on patch, add
explicit headings to the existings sections for simple-panel.

Changes in v2:
 - Added
Changes in v3:
 - None

Cc: Doug Anderson <dianders@chromium.org>
Cc: Eric Anholt <eric@anholt.net>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stéphane Marchesin <marcheu@chromium.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: devicetree@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-rockchip@lists.infradead.org
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 Documentation/devicetree/bindings/display/panel/simple-panel.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
index 16d8ff088b7d..45a457ad38f0 100644
--- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
+++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
@@ -1,4 +1,8 @@
 Simple display panel
+====================
+
+panel node
+----------
 
 Required properties:
 - power-supply: See panel-common.txt
-- 
2.16.0.rc1.238.g530d649a79-goog


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 3/6] dt-bindings: Add panel-timing subnode to simple-panel

[Sean Paul]

This patch adds a new subnode to simple-panel allowing us to override
the typical timing expressed in the panel's display_timing.

Changes in v2:
 - Split out the binding into a new patch (Rob)
 - display-timings is a new section (Rob)
 - Use the full display-timings subnode instead of picking the timing
   out (Rob/Thierry)
Changes in v3:
 - Go back to using the timing subnode directly, but rename to
   panel-timing (Rob)

Cc: Doug Anderson <dianders@chromium.org>
Cc: Eric Anholt <eric@anholt.net>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stéphane Marchesin <marcheu@chromium.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: devicetree@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-rockchip@lists.infradead.org
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 .../bindings/display/panel/simple-panel.txt        | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
index 45a457ad38f0..7788b9ce160b 100644
--- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
+++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
@@ -12,6 +12,24 @@ Optional properties:
 - enable-gpios: GPIO pin to enable or disable the panel
 - backlight: phandle of the backlight device attached to the panel
 
+panel-timing subnode
+--------------------
+
+This optional subnode is for devices which require a mode differing from the
+panel's "typical" display timing as programmed in the simple-panel driver.
+Overriding the driver mode must only be done in the following scenario:
+ - The restrictions motivating the override cannot be applied to the platform's
+   display driver (ie: it must be specific to the device not the platform)
+ - The panel must not have a fixed mode attributed to it in the driver
+ - The panel must provide at list one display_timing range by which the override
+   mode can be validated against
+ - The override mode will use the 'typ' values from the panel-timings subnode
+ - You must provide all required properties for the panel-timing subnode
+
+Format information on the panel-timing subnode can be found in
+display-timing.txt.
+
+
 Example:
 
 	panel: panel {
@@ -22,4 +40,16 @@ Example:
 		enable-gpios = <&gpio 90 0>;
 
 		backlight = <&backlight>;
+
+		panel-timing {
+			clock-frequency = <266604720>;
+			hactive = <2400>;
+			hfront-porch = <48>;
+			hback-porch = <84>;
+			hsync-len = <32>;
+			vactive = <1600>;
+			vfront-porch = <3>;
+			vback-porch = <120>;
+			vsync-len = <10>;
+		};
 	};
-- 
2.16.0.rc1.238.g530d649a79-goog


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 4/6] drm/panel: simple: Add ability to override typical timing

[Sean Paul]

This patch adds the ability to override the typical display timing for a
given panel. This is useful for devices which have timing constraints
that do not apply across the entire display driver (eg: to avoid
crosstalk between panel and digitizer on certain laptops). The rules are
as follows:

- panel must not specify fixed mode (since the override mode will
  either be the same as the fixed mode, or we'll be unable to
  check the bounds of the overried)
- panel must specify at least one display_timing range which will be
  used to ensure the override mode fits within its bounds

Changes in v2:
 - Parse the full display-timings node (using the native-mode) (Rob)
Changes in v3:
 - No longer parse display-timings subnode, use panel-timing (Rob)

Cc: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Cc: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
Cc: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Stéphane Marchesin <marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/gpu/drm/panel/panel-simple.c | 67 +++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 5591984a392b..87488392bca1 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -34,6 +34,7 @@
 #include <drm/drm_panel.h>
 
 #include <video/display_timing.h>
+#include <video/of_display_timing.h>
 #include <video/videomode.h>
 
 struct panel_desc {
@@ -87,6 +88,8 @@ struct panel_simple {
 	struct i2c_adapter *ddc;
 
 	struct gpio_desc *enable_gpio;
+
+	struct drm_display_mode override_mode;
 };
 
 static inline struct panel_simple *to_panel_simple(struct drm_panel *panel)
@@ -99,11 +102,22 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel)
 	struct drm_connector *connector = panel->base.connector;
 	struct drm_device *drm = panel->base.drm;
 	struct drm_display_mode *mode;
+	bool has_override = panel->override_mode.type;
 	unsigned int i, num = 0;
 
 	if (!panel->desc)
 		return 0;
 
+	if (has_override) {
+		mode = drm_mode_duplicate(drm, &panel->override_mode);
+		if (mode) {
+			drm_mode_probed_add(connector, mode);
+			num++;
+		} else {
+			dev_err(drm->dev, "failed to add override mode\n");
+		}
+	}
+
 	for (i = 0; i < panel->desc->num_timings; i++) {
 		const struct display_timing *dt = &panel->desc->timings[i];
 		struct videomode vm;
@@ -120,7 +134,7 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel)
 
 		mode->type |= DRM_MODE_TYPE_DRIVER;
 
-		if (panel->desc->num_timings == 1)
+		if (panel->desc->num_timings == 1 && !has_override)
 			mode->type |= DRM_MODE_TYPE_PREFERRED;
 
 		drm_mode_probed_add(connector, mode);
@@ -291,10 +305,58 @@ static const struct drm_panel_funcs panel_simple_funcs = {
 	.get_timings = panel_simple_get_timings,
 };
 
+#define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \
+	(to_check->field.typ >= bounds->field.min && \
+	 to_check->field.typ <= bounds->field.max)
+static void panel_simple_add_override_mode(struct device *dev,
+					   struct panel_simple *panel,
+					   const struct display_timing *ot)
+{
+	const struct panel_desc *desc = panel->desc;
+	struct videomode vm;
+	int i;
+
+	if (desc->num_modes) {
+		dev_err(dev, "Reject override mode: panel has a fixed mode\n");
+		return;
+	}
+	if (!desc->num_timings) {
+		dev_err(dev, "Reject override mode: no timings specified\n");
+		return;
+	}
+
+	for (i = 0; i < panel->desc->num_timings; i++) {
+		const struct display_timing *dt = &panel->desc->timings[i];
+
+		if (!PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hactive) ||
+		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hfront_porch) ||
+		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hback_porch) ||
+		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hsync_len) ||
+		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vactive) ||
+		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vfront_porch) ||
+		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vback_porch) ||
+		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vsync_len))
+			continue;
+
+		if (ot->flags != dt->flags)
+			continue;
+
+		videomode_from_timing(ot, &vm);
+		drm_display_mode_from_videomode(&vm, &panel->override_mode);
+		panel->override_mode.type |= DRM_MODE_TYPE_DRIVER |
+					     DRM_MODE_TYPE_PREFERRED;
+		return;
+	}
+
+	dev_err(dev, "Reject override mode: No display_timing found\n");
+	return;
+}
+
 static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 {
 	struct device_node *backlight, *ddc;
 	struct panel_simple *panel;
+	struct display_timing dt;
 	int err;
 
 	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
@@ -338,6 +400,9 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 		}
 	}
 
+	if (!of_get_display_timing(dev->of_node, "panel-timing", &dt))
+		panel_simple_add_override_mode(dev, panel, &dt);
+
 	drm_panel_init(&panel->base);
 	panel->base.dev = dev;
 	panel->base.funcs = &panel_simple_funcs;
-- 
2.16.0.rc1.238.g530d649a79-goog

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 5/6] drm/panel: simple: Use display_timing for lq123p1jx31

[Sean Paul]

Convert the sharp lq123p1jx31 from using a fixed mode to specifying a
display timing with min/typ/max values. This allows us to capture the
timings set forth in the datasheet as well as the additional values that
we've cleared with the display vendor to avoid interference with the
digitizer on the Samsung Chromebook Plus (kevin).

A follow-on patch will specify the override mode for kevin devices.

Changes in v2:
 - None
Changes in v3:
 - None

Cc: Doug Anderson <dianders@chromium.org>
Cc: Eric Anholt <eric@anholt.net>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stéphane Marchesin <marcheu@chromium.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: devicetree@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-rockchip@lists.infradead.org
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/panel-simple.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 87488392bca1..6de9c39bc182 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1806,23 +1806,22 @@ static const struct panel_desc sharp_lq101k1ly04 = {
 	.bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
 };
 
-static const struct drm_display_mode sharp_lq123p1jx31_mode = {
-	.clock = 252750,
-	.hdisplay = 2400,
-	.hsync_start = 2400 + 48,
-	.hsync_end = 2400 + 48 + 32,
-	.htotal = 2400 + 48 + 32 + 80,
-	.vdisplay = 1600,
-	.vsync_start = 1600 + 3,
-	.vsync_end = 1600 + 3 + 10,
-	.vtotal = 1600 + 3 + 10 + 33,
-	.vrefresh = 60,
-	.flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+static const struct display_timing sharp_lq123p1jx31_timing = {
+	.pixelclock = { 252750000, 252750000, 266604720 },
+	.hactive = { 2400, 2400, 2400 },
+	.hfront_porch = { 48, 48, 48 },
+	.hback_porch = { 80, 80, 84 },
+	.hsync_len = { 32, 32, 32 },
+	.vactive = { 1600, 1600, 1600 },
+	.vfront_porch = { 3, 3, 3 },
+	.vback_porch = { 33, 33, 120 },
+	.vsync_len = { 10, 10, 10 },
+	.flags = DISPLAY_FLAGS_VSYNC_LOW | DISPLAY_FLAGS_HSYNC_LOW,
 };
 
 static const struct panel_desc sharp_lq123p1jx31 = {
-	.modes = &sharp_lq123p1jx31_mode,
-	.num_modes = 1,
+	.timings = &sharp_lq123p1jx31_timing,
+	.num_timings = 1,
 	.bpc = 8,
 	.size = {
 		.width = 259,
-- 
2.16.0.rc1.238.g530d649a79-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 6/6] arm64: dts: rockchip: Specify override mode for kevin panel

[Sean Paul]

This patch adds an override mode for kevin devices. The mode increases
both back porches to allow a pixel clock of 26666kHz as opposed to the
'typical' value of 252750kHz. This is needed to avoid interference with
the touch digitizer on these laptops.

Changes in v2:
 - Wrap the timing in display-timings node to match binding (Rob/Thierry)
Changes in v3:
 - Unwrap the timing from display-timings and rename panel-timing (Rob)

Cc: Doug Anderson <dianders@chromium.org>
Cc: Eric Anholt <eric@anholt.net>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stéphane Marchesin <marcheu@chromium.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: devicetree@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-rockchip@lists.infradead.org
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
index 191a6bcb1704..658411ce37ea 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
@@ -98,6 +98,20 @@
 		backlight = <&backlight>;
 		power-supply = <&pp3300_disp>;
 
+		panel-timing {
+			clock-frequency = <266604720>;
+			hactive = <2400>;
+			hfront-porch = <48>;
+			hback-porch = <84>;
+			hsync-len = <32>;
+			hsync-active = <0>;
+			vactive = <1600>;
+			vfront-porch = <3>;
+			vback-porch = <120>;
+			vsync-len = <10>;
+			vsync-active = <0>;
+		};
+
 		ports {
 			panel_in_edp: endpoint {
 				remote-endpoint = <&edp_out_panel>;
-- 
2.16.0.rc1.238.g530d649a79-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] dt-bindings: Clarify timing subnode use as panel-timing

[Rob Herring]

On Thu, Feb 8, 2018 at 11:48 AM, Sean Paul <seanpaul@chromium.org> wrote:
> Add a note in the documentation explaining when it's appropriate to use
> the display-timings subnode on its own, as well as the preferred name to
> use (panel-timing).
>
> Changes in v3:
>  - Added
>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-rockchip@lists.infradead.org
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  Documentation/devicetree/bindings/display/panel/display-timing.txt | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/6] dt-bindings: Add headings to simple-panel bindings

[Rob Herring]

On Thu, Feb 8, 2018 at 11:48 AM, Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> In preparation for a new subnode section in a follow-on patch, add
> explicit headings to the existings sections for simple-panel.
>
> Changes in v2:
>  - Added
> Changes in v3:
>  - None
>
> Cc: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> Cc: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> Cc: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Stéphane Marchesin <marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> Cc: linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/display/panel/simple-panel.txt | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/6] dt-bindings: Add panel-timing subnode to simple-panel

[Rob Herring]

On Thu, Feb 8, 2018 at 11:48 AM, Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> This patch adds a new subnode to simple-panel allowing us to override
> the typical timing expressed in the panel's display_timing.
>
> Changes in v2:
>  - Split out the binding into a new patch (Rob)
>  - display-timings is a new section (Rob)
>  - Use the full display-timings subnode instead of picking the timing
>    out (Rob/Thierry)
> Changes in v3:
>  - Go back to using the timing subnode directly, but rename to
>    panel-timing (Rob)
>
> Cc: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> Cc: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> Cc: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Stéphane Marchesin <marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> Cc: linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  .../bindings/display/panel/simple-panel.txt        | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)

Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/6] drm/panel: simple: Add ability to override typical timing

[Enric Balletbo Serra]

Hi,

2018-02-08 18:48 GMT+01:00 Sean Paul <seanpaul@chromium.org>:
> This patch adds the ability to override the typical display timing for a
> given panel. This is useful for devices which have timing constraints
> that do not apply across the entire display driver (eg: to avoid
> crosstalk between panel and digitizer on certain laptops). The rules are
> as follows:
>
> - panel must not specify fixed mode (since the override mode will
>   either be the same as the fixed mode, or we'll be unable to
>   check the bounds of the overried)
> - panel must specify at least one display_timing range which will be
>   used to ensure the override mode fits within its bounds
>
> Changes in v2:
>  - Parse the full display-timings node (using the native-mode) (Rob)
> Changes in v3:
>  - No longer parse display-timings subnode, use panel-timing (Rob)
>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 67 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 5591984a392b..87488392bca1 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -34,6 +34,7 @@
>  #include <drm/drm_panel.h>
>
>  #include <video/display_timing.h>
> +#include <video/of_display_timing.h>
>  #include <video/videomode.h>
>
>  struct panel_desc {
> @@ -87,6 +88,8 @@ struct panel_simple {
>         struct i2c_adapter *ddc;
>
>         struct gpio_desc *enable_gpio;
> +
> +       struct drm_display_mode override_mode;
>  };
>
>  static inline struct panel_simple *to_panel_simple(struct drm_panel *panel)
> @@ -99,11 +102,22 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel)
>         struct drm_connector *connector = panel->base.connector;
>         struct drm_device *drm = panel->base.drm;
>         struct drm_display_mode *mode;
> +       bool has_override = panel->override_mode.type;
>         unsigned int i, num = 0;
>
>         if (!panel->desc)
>                 return 0;
>
> +       if (has_override) {
> +               mode = drm_mode_duplicate(drm, &panel->override_mode);
> +               if (mode) {
> +                       drm_mode_probed_add(connector, mode);
> +                       num++;
> +               } else {
> +                       dev_err(drm->dev, "failed to add override mode\n");
> +               }
> +       }
> +
>         for (i = 0; i < panel->desc->num_timings; i++) {
>                 const struct display_timing *dt = &panel->desc->timings[i];
>                 struct videomode vm;
> @@ -120,7 +134,7 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel)
>
>                 mode->type |= DRM_MODE_TYPE_DRIVER;
>
> -               if (panel->desc->num_timings == 1)
> +               if (panel->desc->num_timings == 1 && !has_override)
>                         mode->type |= DRM_MODE_TYPE_PREFERRED;
>
>                 drm_mode_probed_add(connector, mode);
> @@ -291,10 +305,58 @@ static const struct drm_panel_funcs panel_simple_funcs = {
>         .get_timings = panel_simple_get_timings,
>  };
>
> +#define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \
> +       (to_check->field.typ >= bounds->field.min && \
> +        to_check->field.typ <= bounds->field.max)
> +static void panel_simple_add_override_mode(struct device *dev,
> +                                          struct panel_simple *panel,
> +                                          const struct display_timing *ot)
> +{
> +       const struct panel_desc *desc = panel->desc;
> +       struct videomode vm;
> +       int i;
> +
> +       if (desc->num_modes) {
> +               dev_err(dev, "Reject override mode: panel has a fixed mode\n");
> +               return;
> +       }
> +       if (!desc->num_timings) {
> +               dev_err(dev, "Reject override mode: no timings specified\n");
> +               return;
> +       }
> +
> +       for (i = 0; i < panel->desc->num_timings; i++) {
> +               const struct display_timing *dt = &panel->desc->timings[i];
> +
> +               if (!PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hactive) ||
> +                   !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hfront_porch) ||
> +                   !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hback_porch) ||
> +                   !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hsync_len) ||
> +                   !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vactive) ||
> +                   !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vfront_porch) ||
> +                   !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vback_porch) ||
> +                   !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vsync_len))
> +                       continue;
> +
> +               if (ot->flags != dt->flags)
> +                       continue;
> +
> +               videomode_from_timing(ot, &vm);
> +               drm_display_mode_from_videomode(&vm, &panel->override_mode);
> +               panel->override_mode.type |= DRM_MODE_TYPE_DRIVER |
> +                                            DRM_MODE_TYPE_PREFERRED;
> +               return;
> +       }
> +
> +       dev_err(dev, "Reject override mode: No display_timing found\n");
> +       return;
> +}
> +
>  static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  {
>         struct device_node *backlight, *ddc;
>         struct panel_simple *panel;
> +       struct display_timing dt;
>         int err;
>
>         panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
> @@ -338,6 +400,9 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>                 }
>         }
>
> +       if (!of_get_display_timing(dev->of_node, "panel-timing", &dt))
> +               panel_simple_add_override_mode(dev, panel, &dt);
> +
>         drm_panel_init(&panel->base);
>         panel->base.dev = dev;
>         panel->base.funcs = &panel_simple_funcs;
> --

Tested on top of linux-next on a Samsung Chromebook Plus.

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>



> 2.16.0.rc1.238.g530d649a79-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 5/6] drm/panel: simple: Use display_timing for lq123p1jx31

[Enric Balletbo Serra]

Hi,

2018-02-08 18:48 GMT+01:00 Sean Paul <seanpaul@chromium.org>:
> Convert the sharp lq123p1jx31 from using a fixed mode to specifying a
> display timing with min/typ/max values. This allows us to capture the
> timings set forth in the datasheet as well as the additional values that
> we've cleared with the display vendor to avoid interference with the
> digitizer on the Samsung Chromebook Plus (kevin).
>
> A follow-on patch will specify the override mode for kevin devices.
>
> Changes in v2:
>  - None
> Changes in v3:
>  - None
>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-rockchip@lists.infradead.org
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 87488392bca1..6de9c39bc182 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1806,23 +1806,22 @@ static const struct panel_desc sharp_lq101k1ly04 = {
>         .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA,
>  };
>
> -static const struct drm_display_mode sharp_lq123p1jx31_mode = {
> -       .clock = 252750,
> -       .hdisplay = 2400,
> -       .hsync_start = 2400 + 48,
> -       .hsync_end = 2400 + 48 + 32,
> -       .htotal = 2400 + 48 + 32 + 80,
> -       .vdisplay = 1600,
> -       .vsync_start = 1600 + 3,
> -       .vsync_end = 1600 + 3 + 10,
> -       .vtotal = 1600 + 3 + 10 + 33,
> -       .vrefresh = 60,
> -       .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> +static const struct display_timing sharp_lq123p1jx31_timing = {
> +       .pixelclock = { 252750000, 252750000, 266604720 },
> +       .hactive = { 2400, 2400, 2400 },
> +       .hfront_porch = { 48, 48, 48 },
> +       .hback_porch = { 80, 80, 84 },
> +       .hsync_len = { 32, 32, 32 },
> +       .vactive = { 1600, 1600, 1600 },
> +       .vfront_porch = { 3, 3, 3 },
> +       .vback_porch = { 33, 33, 120 },
> +       .vsync_len = { 10, 10, 10 },
> +       .flags = DISPLAY_FLAGS_VSYNC_LOW | DISPLAY_FLAGS_HSYNC_LOW,
>  };
>
>  static const struct panel_desc sharp_lq123p1jx31 = {
> -       .modes = &sharp_lq123p1jx31_mode,
> -       .num_modes = 1,
> +       .timings = &sharp_lq123p1jx31_timing,
> +       .num_timings = 1,
>         .bpc = 8,
>         .size = {
>                 .width = 259,
> --


Tested on top of linux-next on a Samsung Chromebook Plus.

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>


> 2.16.0.rc1.238.g530d649a79-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] arm64: dts: rockchip: Specify override mode for kevin panel

[Enric Balletbo Serra]

2018-02-08 18:48 GMT+01:00 Sean Paul <seanpaul@chromium.org>:
> This patch adds an override mode for kevin devices. The mode increases
> both back porches to allow a pixel clock of 26666kHz as opposed to the
> 'typical' value of 252750kHz. This is needed to avoid interference with
> the touch digitizer on these laptops.
>
> Changes in v2:
>  - Wrap the timing in display-timings node to match binding (Rob/Thierry)
> Changes in v3:
>  - Unwrap the timing from display-timings and rename panel-timing (Rob)
>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-rockchip@lists.infradead.org
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> index 191a6bcb1704..658411ce37ea 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> @@ -98,6 +98,20 @@
>                 backlight = <&backlight>;
>                 power-supply = <&pp3300_disp>;
>
> +               panel-timing {
> +                       clock-frequency = <266604720>;
> +                       hactive = <2400>;
> +                       hfront-porch = <48>;
> +                       hback-porch = <84>;
> +                       hsync-len = <32>;
> +                       hsync-active = <0>;
> +                       vactive = <1600>;
> +                       vfront-porch = <3>;
> +                       vback-porch = <120>;
> +                       vsync-len = <10>;
> +                       vsync-active = <0>;
> +               };
> +
>                 ports {
>                         panel_in_edp: endpoint {
>                                 remote-endpoint = <&edp_out_panel>;
> --


Tested on top of linux-next on a Samsung Chromebook Plus.

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>


> 2.16.0.rc1.238.g530d649a79-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] dt-bindings: Clarify timing subnode use as panel-timing

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 361 bytes --]

On Thu, Feb 08, 2018 at 12:48:48PM -0500, Sean Paul wrote:
> Add a note in the documentation explaining when it's appropriate to use
> the display-timings subnode on its own, as well as the preferred name to
> use (panel-timing).

panel-timing seems rather redundant, but I guess that's the de facto
standard, so:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/6] dt-bindings: Add panel-timing subnode to simple-panel

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3522 bytes --]

On Thu, Feb 08, 2018 at 12:48:50PM -0500, Sean Paul wrote:
> This patch adds a new subnode to simple-panel allowing us to override
> the typical timing expressed in the panel's display_timing.
> 
> Changes in v2:
>  - Split out the binding into a new patch (Rob)
>  - display-timings is a new section (Rob)
>  - Use the full display-timings subnode instead of picking the timing
>    out (Rob/Thierry)
> Changes in v3:
>  - Go back to using the timing subnode directly, but rename to
>    panel-timing (Rob)
> 
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-rockchip@lists.infradead.org
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  .../bindings/display/panel/simple-panel.txt        | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> index 45a457ad38f0..7788b9ce160b 100644
> --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> @@ -12,6 +12,24 @@ Optional properties:
>  - enable-gpios: GPIO pin to enable or disable the panel
>  - backlight: phandle of the backlight device attached to the panel
>  
> +panel-timing subnode
> +--------------------
> +
> +This optional subnode is for devices which require a mode differing from the
> +panel's "typical" display timing as programmed in the simple-panel driver.
> +Overriding the driver mode must only be done in the following scenario:
> + - The restrictions motivating the override cannot be applied to the platform's
> +   display driver (ie: it must be specific to the device not the platform)
> + - The panel must not have a fixed mode attributed to it in the driver

I think this is somewhat ambiguous. One could argue that panels only
have a fixed mode. What you means if obviously the fixed DRM display
mode, but that's not something that should go into the binding. I'd
simply drop this requirement.

> + - The panel must provide at list one display_timing range by which the override
> +   mode can be validated against

This is the important bit and kind of obsoletes the above anyway. Oh,
and you probably meant "... provide at _least_ one..."

> + - The override mode will use the 'typ' values from the panel-timings subnode

The properties for display timings in DT don't list anything other than
the 'typ' values, right? I'm not sure I understand this requirement.

> + - You must provide all required properties for the panel-timing subnode

Isn't this redundant? required properties are always required.

Other than the unclarities, the above does reflect what we had discussed
on IRC. I wonder, though, how much of this really belongs in the device
tree bindings. The above are almost all restrictions that apply to the
driver, and are very specific to Linux. Perhaps this is something that
belongs in the GPU documentation rather than DT bindings. They are more
like guidelines for what panel drivers should look like rather than what
the DT should look like.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/6] drm/panel: simple: Add ability to override typical timing

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 6775 bytes --]

On Thu, Feb 08, 2018 at 12:48:51PM -0500, Sean Paul wrote:
> This patch adds the ability to override the typical display timing for a
> given panel. This is useful for devices which have timing constraints
> that do not apply across the entire display driver (eg: to avoid
> crosstalk between panel and digitizer on certain laptops). The rules are
> as follows:
> 
> - panel must not specify fixed mode (since the override mode will
>   either be the same as the fixed mode, or we'll be unable to
>   check the bounds of the overried)
> - panel must specify at least one display_timing range which will be
>   used to ensure the override mode fits within its bounds
> 
> Changes in v2:
>  - Parse the full display-timings node (using the native-mode) (Rob)
> Changes in v3:
>  - No longer parse display-timings subnode, use panel-timing (Rob)
> 
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 67 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 5591984a392b..87488392bca1 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -34,6 +34,7 @@
>  #include <drm/drm_panel.h>
>  
>  #include <video/display_timing.h>
> +#include <video/of_display_timing.h>
>  #include <video/videomode.h>
>  
>  struct panel_desc {
> @@ -87,6 +88,8 @@ struct panel_simple {
>  	struct i2c_adapter *ddc;
>  
>  	struct gpio_desc *enable_gpio;
> +
> +	struct drm_display_mode override_mode;
>  };
>  
>  static inline struct panel_simple *to_panel_simple(struct drm_panel *panel)
> @@ -99,11 +102,22 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel)
>  	struct drm_connector *connector = panel->base.connector;
>  	struct drm_device *drm = panel->base.drm;
>  	struct drm_display_mode *mode;
> +	bool has_override = panel->override_mode.type;
>  	unsigned int i, num = 0;
>  
>  	if (!panel->desc)
>  		return 0;
>  
> +	if (has_override) {
> +		mode = drm_mode_duplicate(drm, &panel->override_mode);
> +		if (mode) {
> +			drm_mode_probed_add(connector, mode);
> +			num++;
> +		} else {
> +			dev_err(drm->dev, "failed to add override mode\n");
> +		}
> +	}

Do we really want to continue after this? Shouldn't the override mode
simply override everything else? That is, if users do override the mode,
do we want to give them additional modes to potentially choose from?
Presumably the reason why the user had to override is because the fixed
one didn't work.

Actually, we should only ever have either the display timings specified
or a fixed mode. Anything else is rather bogus.

> +
>  	for (i = 0; i < panel->desc->num_timings; i++) {
>  		const struct display_timing *dt = &panel->desc->timings[i];
>  		struct videomode vm;
> @@ -120,7 +134,7 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel)
>  
>  		mode->type |= DRM_MODE_TYPE_DRIVER;
>  
> -		if (panel->desc->num_timings == 1)
> +		if (panel->desc->num_timings == 1 && !has_override)
>  			mode->type |= DRM_MODE_TYPE_PREFERRED;
>  
>  		drm_mode_probed_add(connector, mode);
> @@ -291,10 +305,58 @@ static const struct drm_panel_funcs panel_simple_funcs = {
>  	.get_timings = panel_simple_get_timings,
>  };
>  
> +#define PANEL_SIMPLE_BOUNDS_CHECK(to_check, bounds, field) \
> +	(to_check->field.typ >= bounds->field.min && \
> +	 to_check->field.typ <= bounds->field.max)
> +static void panel_simple_add_override_mode(struct device *dev,
> +					   struct panel_simple *panel,
> +					   const struct display_timing *ot)
> +{
> +	const struct panel_desc *desc = panel->desc;
> +	struct videomode vm;
> +	int i;

unsigned int, please.

> +
> +	if (desc->num_modes) {
> +		dev_err(dev, "Reject override mode: panel has a fixed mode\n");
> +		return;
> +	}
> +	if (!desc->num_timings) {
> +		dev_err(dev, "Reject override mode: no timings specified\n");
> +		return;
> +	}

Perhaps these should be WARN_ON() to be annoying, but let the driver
continue with the override mode? Again, this is something that we should
catch during review (when a new compatible is added, or a new driver, we
should make sure that it always comes with a timing).

WARN_ON() is probably enough to let people know when they test that they
forgot something.

> +
> +	for (i = 0; i < panel->desc->num_timings; i++) {
> +		const struct display_timing *dt = &panel->desc->timings[i];
> +
> +		if (!PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hactive) ||
> +		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hfront_porch) ||
> +		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hback_porch) ||
> +		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, hsync_len) ||
> +		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vactive) ||
> +		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vfront_porch) ||
> +		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vback_porch) ||
> +		    !PANEL_SIMPLE_BOUNDS_CHECK(ot, dt, vsync_len))
> +			continue;
> +
> +		if (ot->flags != dt->flags)
> +			continue;
> +
> +		videomode_from_timing(ot, &vm);
> +		drm_display_mode_from_videomode(&vm, &panel->override_mode);
> +		panel->override_mode.type |= DRM_MODE_TYPE_DRIVER |
> +					     DRM_MODE_TYPE_PREFERRED;
> +		return;
> +	}
> +
> +	dev_err(dev, "Reject override mode: No display_timing found\n");

Perhaps something like this, to simplify the code flow:

	if (!panel->override_mode.type)
		dev_err(dev, ...);

Then turn the above return into a break and leave out the below return.

> +	return;
> +}
> +
>  static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  {
>  	struct device_node *backlight, *ddc;
>  	struct panel_simple *panel;
> +	struct display_timing dt;
>  	int err;
>  
>  	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
> @@ -338,6 +400,9 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
>  		}
>  	}
>  
> +	if (!of_get_display_timing(dev->of_node, "panel-timing", &dt))
> +		panel_simple_add_override_mode(dev, panel, &dt);

Perahps "panel_simple_parse_override_mode()"? To clarify what exactly
this does. This doesn't actually "add" the mode yet, that's only done
in panel_simple_get_fixed_modes().

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] arm64: dts: rockchip: Specify override mode for kevin panel

[Doug Anderson]

Hi,

On Thu, Feb 8, 2018 at 9:48 AM, Sean Paul <seanpaul@chromium.org> wrote:
> This patch adds an override mode for kevin devices. The mode increases
> both back porches to allow a pixel clock of 26666kHz as opposed to the
> 'typical' value of 252750kHz. This is needed to avoid interference with
> the touch digitizer on these laptops.
>
> Changes in v2:
>  - Wrap the timing in display-timings node to match binding (Rob/Thierry)
> Changes in v3:
>  - Unwrap the timing from display-timings and rename panel-timing (Rob)
>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-rockchip@lists.infradead.org
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> index 191a6bcb1704..658411ce37ea 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> @@ -98,6 +98,20 @@
>                 backlight = <&backlight>;
>                 power-supply = <&pp3300_disp>;
>
> +               panel-timing {
> +                       clock-frequency = <266604720>;
> +                       hactive = <2400>;
> +                       hfront-porch = <48>;
> +                       hback-porch = <84>;
> +                       hsync-len = <32>;
> +                       hsync-active = <0>;
> +                       vactive = <1600>;
> +                       vfront-porch = <3>;
> +                       vback-porch = <120>;
> +                       vsync-len = <10>;
> +                       vsync-active = <0>;
> +               };
> +
>                 ports {
>                         panel_in_edp: endpoint {
>                                 remote-endpoint = <&edp_out_panel>;

Kristian brought an old bug to my attention
<https://bugs.chromium.org/p/chromium/issues/detail?id=750354> and it
made me think.  Should we somehow adjust the bindings here to account
for the fact that a board may source several different panels?

AKA: on some boards an ODM may want to second source (or third source,
or ...) the panel.  They'll randomly connect several different panels
to the board and ship the boards out.  The panels are all compatible
electrically (same power sequencing) but might need slightly different
timings.  In this particular case there's no board-level strappings
for the different panels--it's assumed that the EDID on the panels can
be used to distinguish them.

In that case it seems like it would be nice to allow specifying more
than one "panel-timing" nodes.  Maybe keyed off some type of ID that's
present in the EDID?


Is that something we'd want to account for before we land this series?
 It seems like it would just be adding an extra level of nodes?


-Doug
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/6] dt-bindings: Add panel-timing subnode to simple-panel

[Laurent Pinchart]

Hi Sean,

Thank you for the patch.

On Thursday, 8 February 2018 19:48:50 EET Sean Paul wrote:
> This patch adds a new subnode to simple-panel allowing us to override
> the typical timing expressed in the panel's display_timing.
> 
> Changes in v2:
>  - Split out the binding into a new patch (Rob)
>  - display-timings is a new section (Rob)
>  - Use the full display-timings subnode instead of picking the timing
>    out (Rob/Thierry)
> Changes in v3:
>  - Go back to using the timing subnode directly, but rename to
>    panel-timing (Rob)
> 
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: devicetree@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-rockchip@lists.infradead.org
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  .../bindings/display/panel/simple-panel.txt        | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> b/Documentation/devicetree/bindings/display/panel/simple-panel.txt index
> 45a457ad38f0..7788b9ce160b 100644
> --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> @@ -12,6 +12,24 @@ Optional properties:
>  - enable-gpios: GPIO pin to enable or disable the panel
>  - backlight: phandle of the backlight device attached to the panel
> 
> +panel-timing subnode
> +--------------------
> +
> +This optional subnode is for devices which require a mode differing from
> the
> +panel's "typical" display timing as programmed in the simple-panel driver.

Please don't refer to particular drivers in DT bindings.

> +Overriding the driver mode must only be done in the following scenario:
> + - The restrictions motivating the override cannot be applied to the
> platform's
> +   display driver (ie: it must be specific to the device not the platform)

I find this quite ambiguous. I assume that "not the platform" refers to the 
platform's display controller. "platform" without a qualifier is too vague, as 
I expect the reason to override the mode will come from the platform's 
specific interactions between the panel, the display controller, the other 
display-related devices if any, and the PCB.

> + - The panel must not have a fixed mode attributed to it in the driver

I'm not sure to follow you here.

> + - The panel must provide at list one display_timing range by which the
> override
> +   mode can be validated against
> + - The override mode will use the 'typ' values from the panel-timings
> subnode

The panel-timings subnode has no "typ" values. Do you mean that the 
display_timing for the panel will have its "typ" values overridden by the 
values contained in the panel-timing node ?

> + - You must provide all required properties for the panel-timing subnode
> +
> +Format information on the panel-timing subnode can be found in
> +display-timing.txt.
> +
> +
>  Example:
> 
>  	panel: panel {
> @@ -22,4 +40,16 @@ Example:
>  		enable-gpios = <&gpio 90 0>;
> 
>  		backlight = <&backlight>;
> +
> +		panel-timing {
> +			clock-frequency = <266604720>;
> +			hactive = <2400>;
> +			hfront-porch = <48>;
> +			hback-porch = <84>;
> +			hsync-len = <32>;
> +			vactive = <1600>;
> +			vfront-porch = <3>;
> +			vback-porch = <120>;
> +			vsync-len = <10>;
> +		};
>  	};

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/6] drm/panel: simple: Add mode support to devicetree

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1417 bytes --]

On Thu, Feb 08, 2018 at 12:48:47PM -0500, Sean Paul wrote:
> Hello!
> 
> Here's v3 of the set. I've reintroduced the single display timing node
> in the dt binding from v1, and left the improved docs and patch split
> from v2. I've also added a patch to clarify that a single display timing
> node should always be named panel-timing, which was feedback from v1.
> 
> Cover letter from v1 (was the same in v2) is here:
> https://lists.freedesktop.org/archives/dri-devel/2018-February/164886.html
> 
> Thanks for the reviews so far,
> 
> Sean
> 
> 
> Sean Paul (6):
>   dt-bindings: Clarify timing subnode use as panel-timing
>   dt-bindings: Add headings to simple-panel bindings
>   dt-bindings: Add panel-timing subnode to simple-panel
>   drm/panel: simple: Add ability to override typical timing
>   drm/panel: simple: Use display_timing for lq123p1jx31
>   arm64: dts: rockchip: Specify override mode for kevin panel
> 
>  .../bindings/display/panel/display-timing.txt      |  5 ++
>  .../bindings/display/panel/simple-panel.txt        | 34 ++++++++
>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts  | 14 ++++
>  drivers/gpu/drm/panel/panel-simple.c               | 94 ++++++++++++++++++----
>  4 files changed, 132 insertions(+), 15 deletions(-)

I've applied patches 1, 2 and 5 of the series, 3 and 4 had comments and
6 will need to go through the Rockchip tree.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] arm64: dts: rockchip: Specify override mode for kevin panel

[Ezequiel Garcia]

Hi Doug, Sean:

I would like to move this forward.

On 26 February 2018 at 15:23, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Thu, Feb 8, 2018 at 9:48 AM, Sean Paul <seanpaul@chromium.org> wrote:
>> This patch adds an override mode for kevin devices. The mode increases
>> both back porches to allow a pixel clock of 26666kHz as opposed to the
>> 'typical' value of 252750kHz. This is needed to avoid interference with
>> the touch digitizer on these laptops.
>>
>> Changes in v2:
>>  - Wrap the timing in display-timings node to match binding (Rob/Thierry)
>> Changes in v3:
>>  - Unwrap the timing from display-timings and rename panel-timing (Rob)
>>
>> Cc: Doug Anderson <dianders@chromium.org>
>> Cc: Eric Anholt <eric@anholt.net>
>> Cc: Heiko Stuebner <heiko@sntech.de>
>> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Stéphane Marchesin <marcheu@chromium.org>
>> Cc: Thierry Reding <thierry.reding@gmail.com>
>> Cc: devicetree@vger.kernel.org
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linux-rockchip@lists.infradead.org
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> ---
>>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>> index 191a6bcb1704..658411ce37ea 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>> @@ -98,6 +98,20 @@
>>                 backlight = <&backlight>;
>>                 power-supply = <&pp3300_disp>;
>>
>> +               panel-timing {
>> +                       clock-frequency = <266604720>;
>> +                       hactive = <2400>;
>> +                       hfront-porch = <48>;
>> +                       hback-porch = <84>;
>> +                       hsync-len = <32>;
>> +                       hsync-active = <0>;
>> +                       vactive = <1600>;
>> +                       vfront-porch = <3>;
>> +                       vback-porch = <120>;
>> +                       vsync-len = <10>;
>> +                       vsync-active = <0>;
>> +               };
>> +
>>                 ports {
>>                         panel_in_edp: endpoint {
>>                                 remote-endpoint = <&edp_out_panel>;
>
> Kristian brought an old bug to my attention
> <https://bugs.chromium.org/p/chromium/issues/detail?id=750354> and it
> made me think.  Should we somehow adjust the bindings here to account
> for the fact that a board may source several different panels?
>
> AKA: on some boards an ODM may want to second source (or third source,
> or ...) the panel.  They'll randomly connect several different panels
> to the board and ship the boards out.  The panels are all compatible
> electrically (same power sequencing) but might need slightly different
> timings.  In this particular case there's no board-level strappings
> for the different panels--it's assumed that the EDID on the panels can
> be used to distinguish them.
>
> In that case it seems like it would be nice to allow specifying more
> than one "panel-timing" nodes.  Maybe keyed off some type of ID that's
> present in the EDID?
>
>
> Is that something we'd want to account for before we land this series?
>  It seems like it would just be adding an extra level of nodes?
>

AFAICS, there is no EDID without a DDC bus, which we don't
seem to have on gru platforms, do we?

Regards,
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] arm64: dts: rockchip: Specify override mode for kevin panel

[Stéphane Marchesin]

On Tue, Apr 24, 2018 at 7:31 AM, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> Hi Doug, Sean:
>
> I would like to move this forward.
>
> On 26 February 2018 at 15:23, Doug Anderson <dianders@chromium.org> wrote:
>> Hi,
>>
>> On Thu, Feb 8, 2018 at 9:48 AM, Sean Paul <seanpaul@chromium.org> wrote:
>>> This patch adds an override mode for kevin devices. The mode increases
>>> both back porches to allow a pixel clock of 26666kHz as opposed to the
>>> 'typical' value of 252750kHz. This is needed to avoid interference with
>>> the touch digitizer on these laptops.
>>>
>>> Changes in v2:
>>>  - Wrap the timing in display-timings node to match binding (Rob/Thierry)
>>> Changes in v3:
>>>  - Unwrap the timing from display-timings and rename panel-timing (Rob)
>>>
>>> Cc: Doug Anderson <dianders@chromium.org>
>>> Cc: Eric Anholt <eric@anholt.net>
>>> Cc: Heiko Stuebner <heiko@sntech.de>
>>> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Stéphane Marchesin <marcheu@chromium.org>
>>> Cc: Thierry Reding <thierry.reding@gmail.com>
>>> Cc: devicetree@vger.kernel.org
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: linux-rockchip@lists.infradead.org
>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>> index 191a6bcb1704..658411ce37ea 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>> @@ -98,6 +98,20 @@
>>>                 backlight = <&backlight>;
>>>                 power-supply = <&pp3300_disp>;
>>>
>>> +               panel-timing {
>>> +                       clock-frequency = <266604720>;
>>> +                       hactive = <2400>;
>>> +                       hfront-porch = <48>;
>>> +                       hback-porch = <84>;
>>> +                       hsync-len = <32>;
>>> +                       hsync-active = <0>;
>>> +                       vactive = <1600>;
>>> +                       vfront-porch = <3>;
>>> +                       vback-porch = <120>;
>>> +                       vsync-len = <10>;
>>> +                       vsync-active = <0>;
>>> +               };
>>> +
>>>                 ports {
>>>                         panel_in_edp: endpoint {
>>>                                 remote-endpoint = <&edp_out_panel>;
>>
>> Kristian brought an old bug to my attention
>> <https://bugs.chromium.org/p/chromium/issues/detail?id=750354> and it
>> made me think.  Should we somehow adjust the bindings here to account
>> for the fact that a board may source several different panels?
>>
>> AKA: on some boards an ODM may want to second source (or third source,
>> or ...) the panel.  They'll randomly connect several different panels
>> to the board and ship the boards out.  The panels are all compatible
>> electrically (same power sequencing) but might need slightly different
>> timings.  In this particular case there's no board-level strappings
>> for the different panels--it's assumed that the EDID on the panels can
>> be used to distinguish them.
>>
>> In that case it seems like it would be nice to allow specifying more
>> than one "panel-timing" nodes.  Maybe keyed off some type of ID that's
>> present in the EDID?
>>
>>
>> Is that something we'd want to account for before we land this series?
>>  It seems like it would just be adding an extra level of nodes?
>>
>
> AFAICS, there is no EDID without a DDC bus, which we don't
> seem to have on gru platforms, do we?

IIRC historically we've only done second sourcing when
doable because either:
1. the timings between all the panels we use are compatible, or
2. we have a working DDC to figure it out for us.

In other words, we haven't handled the case where timings are not
compatible and we can't address this by reading EDIDs.

Stéphane

>
> Regards,
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] arm64: dts: rockchip: Specify override mode for kevin panel

[Doug Anderson]

Hi,

On Tue, Apr 24, 2018 at 7:31 AM, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> Hi Doug, Sean:
>
> I would like to move this forward.
>
> On 26 February 2018 at 15:23, Doug Anderson <dianders@chromium.org> wrote:
>> Hi,
>>
>> On Thu, Feb 8, 2018 at 9:48 AM, Sean Paul <seanpaul@chromium.org> wrote:
>>> This patch adds an override mode for kevin devices. The mode increases
>>> both back porches to allow a pixel clock of 26666kHz as opposed to the
>>> 'typical' value of 252750kHz. This is needed to avoid interference with
>>> the touch digitizer on these laptops.
>>>
>>> Changes in v2:
>>>  - Wrap the timing in display-timings node to match binding (Rob/Thierry)
>>> Changes in v3:
>>>  - Unwrap the timing from display-timings and rename panel-timing (Rob)
>>>
>>> Cc: Doug Anderson <dianders@chromium.org>
>>> Cc: Eric Anholt <eric@anholt.net>
>>> Cc: Heiko Stuebner <heiko@sntech.de>
>>> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Stéphane Marchesin <marcheu@chromium.org>
>>> Cc: Thierry Reding <thierry.reding@gmail.com>
>>> Cc: devicetree@vger.kernel.org
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: linux-rockchip@lists.infradead.org
>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>> index 191a6bcb1704..658411ce37ea 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>> @@ -98,6 +98,20 @@
>>>                 backlight = <&backlight>;
>>>                 power-supply = <&pp3300_disp>;
>>>
>>> +               panel-timing {
>>> +                       clock-frequency = <266604720>;
>>> +                       hactive = <2400>;
>>> +                       hfront-porch = <48>;
>>> +                       hback-porch = <84>;
>>> +                       hsync-len = <32>;
>>> +                       hsync-active = <0>;
>>> +                       vactive = <1600>;
>>> +                       vfront-porch = <3>;
>>> +                       vback-porch = <120>;
>>> +                       vsync-len = <10>;
>>> +                       vsync-active = <0>;
>>> +               };
>>> +
>>>                 ports {
>>>                         panel_in_edp: endpoint {
>>>                                 remote-endpoint = <&edp_out_panel>;
>>
>> Kristian brought an old bug to my attention
>> <https://bugs.chromium.org/p/chromium/issues/detail?id=750354> and it
>> made me think.  Should we somehow adjust the bindings here to account
>> for the fact that a board may source several different panels?
>>
>> AKA: on some boards an ODM may want to second source (or third source,
>> or ...) the panel.  They'll randomly connect several different panels
>> to the board and ship the boards out.  The panels are all compatible
>> electrically (same power sequencing) but might need slightly different
>> timings.  In this particular case there's no board-level strappings
>> for the different panels--it's assumed that the EDID on the panels can
>> be used to distinguish them.
>>
>> In that case it seems like it would be nice to allow specifying more
>> than one "panel-timing" nodes.  Maybe keyed off some type of ID that's
>> present in the EDID?
>>
>>
>> Is that something we'd want to account for before we land this series?
>>  It seems like it would just be adding an extra level of nodes?
>>
>
> AFAICS, there is no EDID without a DDC bus, which we don't
> seem to have on gru platforms, do we?

I'm fairly certain we can read the EDID on gru devices.  I see the
"aux" channel connected on the schematics.  That'll do it, right?
...and I could have sworn that at some point in time I could read the
EDID in the software.  My kevin devices are not connected for
debugging so I can't check right now, but I'm pretty sure I was able
to read the EDID in the past...

-Doug
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] arm64: dts: rockchip: Specify override mode for kevin panel

[Ezequiel Garcia]

On 25 April 2018 at 01:29, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Tue, Apr 24, 2018 at 7:31 AM, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
>> Hi Doug, Sean:
>>
>> I would like to move this forward.
>>
>> On 26 February 2018 at 15:23, Doug Anderson <dianders@chromium.org> wrote:
>>> Hi,
>>>
>>> On Thu, Feb 8, 2018 at 9:48 AM, Sean Paul <seanpaul@chromium.org> wrote:
>>>> This patch adds an override mode for kevin devices. The mode increases
>>>> both back porches to allow a pixel clock of 26666kHz as opposed to the
>>>> 'typical' value of 252750kHz. This is needed to avoid interference with
>>>> the touch digitizer on these laptops.
>>>>
>>>> Changes in v2:
>>>>  - Wrap the timing in display-timings node to match binding (Rob/Thierry)
>>>> Changes in v3:
>>>>  - Unwrap the timing from display-timings and rename panel-timing (Rob)
>>>>
>>>> Cc: Doug Anderson <dianders@chromium.org>
>>>> Cc: Eric Anholt <eric@anholt.net>
>>>> Cc: Heiko Stuebner <heiko@sntech.de>
>>>> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Stéphane Marchesin <marcheu@chromium.org>
>>>> Cc: Thierry Reding <thierry.reding@gmail.com>
>>>> Cc: devicetree@vger.kernel.org
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Cc: linux-rockchip@lists.infradead.org
>>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>>> ---
>>>>  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 14 ++++++++++++++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>>> index 191a6bcb1704..658411ce37ea 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
>>>> @@ -98,6 +98,20 @@
>>>>                 backlight = <&backlight>;
>>>>                 power-supply = <&pp3300_disp>;
>>>>
>>>> +               panel-timing {
>>>> +                       clock-frequency = <266604720>;
>>>> +                       hactive = <2400>;
>>>> +                       hfront-porch = <48>;
>>>> +                       hback-porch = <84>;
>>>> +                       hsync-len = <32>;
>>>> +                       hsync-active = <0>;
>>>> +                       vactive = <1600>;
>>>> +                       vfront-porch = <3>;
>>>> +                       vback-porch = <120>;
>>>> +                       vsync-len = <10>;
>>>> +                       vsync-active = <0>;
>>>> +               };
>>>> +
>>>>                 ports {
>>>>                         panel_in_edp: endpoint {
>>>>                                 remote-endpoint = <&edp_out_panel>;
>>>
>>> Kristian brought an old bug to my attention
>>> <https://bugs.chromium.org/p/chromium/issues/detail?id=750354> and it
>>> made me think.  Should we somehow adjust the bindings here to account
>>> for the fact that a board may source several different panels?
>>>
>>> AKA: on some boards an ODM may want to second source (or third source,
>>> or ...) the panel.  They'll randomly connect several different panels
>>> to the board and ship the boards out.  The panels are all compatible
>>> electrically (same power sequencing) but might need slightly different
>>> timings.  In this particular case there's no board-level strappings
>>> for the different panels--it's assumed that the EDID on the panels can
>>> be used to distinguish them.
>>>
>>> In that case it seems like it would be nice to allow specifying more
>>> than one "panel-timing" nodes.  Maybe keyed off some type of ID that's
>>> present in the EDID?
>>>
>>>
>>> Is that something we'd want to account for before we land this series?
>>>  It seems like it would just be adding an extra level of nodes?
>>>
>>
>> AFAICS, there is no EDID without a DDC bus, which we don't
>> seem to have on gru platforms, do we?
>
> I'm fairly certain we can read the EDID on gru devices.  I see the
> "aux" channel connected on the schematics.  That'll do it, right?
> ...and I could have sworn that at some point in time I could read the
> EDID in the software.  My kevin devices are not connected for
> debugging so I can't check right now, but I'm pretty sure I was able
> to read the EDID in the past...
>

Right.

Well, I've been trying to get some EDID without much success.
Perhaps I am missing something? The I2C aux bus seems empty:

root@linaro-developer:/sys/class/i2c-adapter/i2c-10# cat name
DP-AUX
root@linaro-developer:/sys/class/i2c-adapter/i2c-10# i2cdetect -y 10
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --

In any case, perhaps it's better to not stall this series
upstream - and work on second sourcing as follow up
patches?

Thanks,
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] arm64: dts: rockchip: Specify override mode for kevin panel

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3494 bytes --]

On Mon, Feb 26, 2018 at 10:23:00AM -0800, Doug Anderson wrote:
> Hi,
> 
> On Thu, Feb 8, 2018 at 9:48 AM, Sean Paul <seanpaul@chromium.org> wrote:
> > This patch adds an override mode for kevin devices. The mode increases
> > both back porches to allow a pixel clock of 26666kHz as opposed to the
> > 'typical' value of 252750kHz. This is needed to avoid interference with
> > the touch digitizer on these laptops.
> >
> > Changes in v2:
> >  - Wrap the timing in display-timings node to match binding (Rob/Thierry)
> > Changes in v3:
> >  - Unwrap the timing from display-timings and rename panel-timing (Rob)
> >
> > Cc: Doug Anderson <dianders@chromium.org>
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Heiko Stuebner <heiko@sntech.de>
> > Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Stéphane Marchesin <marcheu@chromium.org>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: devicetree@vger.kernel.org
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-rockchip@lists.infradead.org
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> > index 191a6bcb1704..658411ce37ea 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts
> > @@ -98,6 +98,20 @@
> >                 backlight = <&backlight>;
> >                 power-supply = <&pp3300_disp>;
> >
> > +               panel-timing {
> > +                       clock-frequency = <266604720>;
> > +                       hactive = <2400>;
> > +                       hfront-porch = <48>;
> > +                       hback-porch = <84>;
> > +                       hsync-len = <32>;
> > +                       hsync-active = <0>;
> > +                       vactive = <1600>;
> > +                       vfront-porch = <3>;
> > +                       vback-porch = <120>;
> > +                       vsync-len = <10>;
> > +                       vsync-active = <0>;
> > +               };
> > +
> >                 ports {
> >                         panel_in_edp: endpoint {
> >                                 remote-endpoint = <&edp_out_panel>;
> 
> Kristian brought an old bug to my attention
> <https://bugs.chromium.org/p/chromium/issues/detail?id=750354> and it
> made me think.  Should we somehow adjust the bindings here to account
> for the fact that a board may source several different panels?
> 
> AKA: on some boards an ODM may want to second source (or third source,
> or ...) the panel.  They'll randomly connect several different panels
> to the board and ship the boards out.  The panels are all compatible
> electrically (same power sequencing) but might need slightly different
> timings.  In this particular case there's no board-level strappings
> for the different panels--it's assumed that the EDID on the panels can
> be used to distinguish them.
> 
> In that case it seems like it would be nice to allow specifying more
> than one "panel-timing" nodes.  Maybe keyed off some type of ID that's
> present in the EDID?

If you've got an EDID you should be relying on the EDID to provide the
timings. No need to have any timings in the DT in that case.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] arm64: dts: rockchip: Specify override mode for kevin panel

[Doug Anderson]

Hi,

On Thu, Apr 26, 2018 at 5:05 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> If you've got an EDID you should be relying on the EDID to provide the
> timings. No need to have any timings in the DT in that case.

The problem that's specifically trying to be solved by Sean's series
is when we have to use timings other than the one suggested by the
EDID.  Specifically:

* On many Rockchip SoCs there is only one "extra" PLL available.  This
extra PLL can only be used by one of the two displays (eDP for
internal panel or HDMI/DP for external display).  The other display
has to use one of the "shared" PLLs in the system.  These PLLs have
their rate set at boot and aren't changed.

* In order to provide maximum flexibility to connect external
displays, we always want the "extra" PLL dedicated to the external
display port.  Then we can make lots of pixel clocks.

* We work with device and display manufacturers to figure out a pixel
clock that is achievable with the shared PLLs and that has valid
timings.  This is the the pixel clock that was used when testing EMI,
etc.  It is the one that should be used.

* Panel manufacturers agreed that this pixel clock was good to use,
but we didn't get an updated EDID that suggested this mode.  It's my
understanding that panels were already available and it didn't really
make sense to program in an EDID to work around a certain board.



-Doug
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/6] drm/panel: simple: Add mode support to devicetree

[Doug Anderson]

Hi,

On Mon, Mar 12, 2018 at 1:35 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Thu, Feb 08, 2018 at 12:48:47PM -0500, Sean Paul wrote:
> > Hello!
> >
> > Here's v3 of the set. I've reintroduced the single display timing node
> > in the dt binding from v1, and left the improved docs and patch split
> > from v2. I've also added a patch to clarify that a single display timing
> > node should always be named panel-timing, which was feedback from v1.
> >
> > Cover letter from v1 (was the same in v2) is here:
> > https://lists.freedesktop.org/archives/dri-devel/2018-February/164886.html
> >
> > Thanks for the reviews so far,
> >
> > Sean
> >
> >
> > Sean Paul (6):
> >   dt-bindings: Clarify timing subnode use as panel-timing
> >   dt-bindings: Add headings to simple-panel bindings
> >   dt-bindings: Add panel-timing subnode to simple-panel
> >   drm/panel: simple: Add ability to override typical timing
> >   drm/panel: simple: Use display_timing for lq123p1jx31
> >   arm64: dts: rockchip: Specify override mode for kevin panel
> >
> >  .../bindings/display/panel/display-timing.txt      |  5 ++
> >  .../bindings/display/panel/simple-panel.txt        | 34 ++++++++
> >  arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts  | 14 ++++
> >  drivers/gpu/drm/panel/panel-simple.c               | 94 ++++++++++++++++++----
> >  4 files changed, 132 insertions(+), 15 deletions(-)
>
> I've applied patches 1, 2 and 5 of the series, 3 and 4 had comments and
> 6 will need to go through the Rockchip tree.

Breadcrumbs for anyone who stumbles upon this v3 and didn't notice
that v4 was posted by me instead of Sean Paul.  To find it, head on
over to:

https://lkml.kernel.org/r/20190328171710.31949-1-dianders@chromium.org

-Doug
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel