[PATCH 0/4] drm/sun4i: rgb: Relax the pixel clock check

[PATCH 0/4] drm/sun4i: rgb: Relax the pixel clock check

[Maxime Ripard]

Hi,

This implements roughly what we discussed as part of the ANX6345 series to
relax the pixel clock validation while still filtering out the modes we
can't reach using the bridges.

Let me know what you think,
Maxime

Maxime Ripard (4):
  drm/sun4i: Move the panel pointer from the TCON to the encoders
  drm/sun4i: rgb: Store the bridge pointer
  drm/sun4i: Move rate variables to long long
  drm/sun4i: rgb: Change the pixel clock validation check

 drivers/gpu/drm/sun4i/sun4i_lvds.c | 29 ++++--------
 drivers/gpu/drm/sun4i/sun4i_rgb.c  | 74 +++++++++++++++++++++----------
 drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 +-
 3 files changed, 64 insertions(+), 41 deletions(-)

base-commit: bfeffd155283772bbe78c6a05dec7c0128ee500c
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] drm/sun4i: Move the panel pointer from the TCON to the encoders

[Maxime Ripard]

The TCON driver used to need the panel pointer in order to configure the
tcon according to the various parameters of the panel. However, this has
evolved over time (especially to support bridges), and therefore the panel
pointer isn't needed anymore by the TCON driver.

Move that pointer to the LVDS and RGB encoders drivers.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_lvds.c | 29 ++++++++++++-----------------
 drivers/gpu/drm/sun4i/sun4i_rgb.c  | 27 ++++++++++++---------------
 drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 --
 3 files changed, 24 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c
index e7eb0d1e17be..5696f55db5cd 100644
--- a/drivers/gpu/drm/sun4i/sun4i_lvds.c
+++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
@@ -20,7 +20,7 @@ struct sun4i_lvds {
 	struct drm_connector	connector;
 	struct drm_encoder	encoder;
 
-	struct sun4i_tcon	*tcon;
+	struct drm_panel	*panel;
 };
 
 static inline struct sun4i_lvds *
@@ -41,9 +41,8 @@ static int sun4i_lvds_get_modes(struct drm_connector *connector)
 {
 	struct sun4i_lvds *lvds =
 		drm_connector_to_sun4i_lvds(connector);
-	struct sun4i_tcon *tcon = lvds->tcon;
 
-	return drm_panel_get_modes(tcon->panel);
+	return drm_panel_get_modes(lvds->panel);
 }
 
 static struct drm_connector_helper_funcs sun4i_lvds_con_helper_funcs = {
@@ -54,9 +53,8 @@ static void
 sun4i_lvds_connector_destroy(struct drm_connector *connector)
 {
 	struct sun4i_lvds *lvds = drm_connector_to_sun4i_lvds(connector);
-	struct sun4i_tcon *tcon = lvds->tcon;
 
-	drm_panel_detach(tcon->panel);
+	drm_panel_detach(lvds->panel);
 	drm_connector_cleanup(connector);
 }
 
@@ -71,26 +69,24 @@ static const struct drm_connector_funcs sun4i_lvds_con_funcs = {
 static void sun4i_lvds_encoder_enable(struct drm_encoder *encoder)
 {
 	struct sun4i_lvds *lvds = drm_encoder_to_sun4i_lvds(encoder);
-	struct sun4i_tcon *tcon = lvds->tcon;
 
 	DRM_DEBUG_DRIVER("Enabling LVDS output\n");
 
-	if (tcon->panel) {
-		drm_panel_prepare(tcon->panel);
-		drm_panel_enable(tcon->panel);
+	if (lvds->panel) {
+		drm_panel_prepare(lvds->panel);
+		drm_panel_enable(lvds->panel);
 	}
 }
 
 static void sun4i_lvds_encoder_disable(struct drm_encoder *encoder)
 {
 	struct sun4i_lvds *lvds = drm_encoder_to_sun4i_lvds(encoder);
-	struct sun4i_tcon *tcon = lvds->tcon;
 
 	DRM_DEBUG_DRIVER("Disabling LVDS output\n");
 
-	if (tcon->panel) {
-		drm_panel_disable(tcon->panel);
-		drm_panel_unprepare(tcon->panel);
+	if (lvds->panel) {
+		drm_panel_disable(lvds->panel);
+		drm_panel_unprepare(lvds->panel);
 	}
 }
 
@@ -113,11 +109,10 @@ int sun4i_lvds_init(struct drm_device *drm, struct sun4i_tcon *tcon)
 	lvds = devm_kzalloc(drm->dev, sizeof(*lvds), GFP_KERNEL);
 	if (!lvds)
 		return -ENOMEM;
-	lvds->tcon = tcon;
 	encoder = &lvds->encoder;
 
 	ret = drm_of_find_panel_or_bridge(tcon->dev->of_node, 1, 0,
-					  &tcon->panel, &bridge);
+					  &lvds->panel, &bridge);
 	if (ret) {
 		dev_info(drm->dev, "No panel or bridge found... LVDS output disabled\n");
 		return 0;
@@ -138,7 +133,7 @@ int sun4i_lvds_init(struct drm_device *drm, struct sun4i_tcon *tcon)
 	/* The LVDS encoder can only work with the TCON channel 0 */
 	lvds->encoder.possible_crtcs = drm_crtc_mask(&tcon->crtc->crtc);
 
-	if (tcon->panel) {
+	if (lvds->panel) {
 		drm_connector_helper_add(&lvds->connector,
 					 &sun4i_lvds_con_helper_funcs);
 		ret = drm_connector_init(drm, &lvds->connector,
@@ -152,7 +147,7 @@ int sun4i_lvds_init(struct drm_device *drm, struct sun4i_tcon *tcon)
 		drm_connector_attach_encoder(&lvds->connector,
 						  &lvds->encoder);
 
-		ret = drm_panel_attach(tcon->panel, &lvds->connector);
+		ret = drm_panel_attach(lvds->panel, &lvds->connector);
 		if (ret) {
 			dev_err(drm->dev, "Couldn't attach our panel\n");
 			goto err_cleanup_connector;
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index f4a22689eb54..d181ee399b03 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -27,6 +27,7 @@ struct sun4i_rgb {
 	struct drm_encoder	encoder;
 
 	struct sun4i_tcon	*tcon;
+	struct drm_panel	*panel;
 };
 
 static inline struct sun4i_rgb *
@@ -47,9 +48,8 @@ static int sun4i_rgb_get_modes(struct drm_connector *connector)
 {
 	struct sun4i_rgb *rgb =
 		drm_connector_to_sun4i_rgb(connector);
-	struct sun4i_tcon *tcon = rgb->tcon;
 
-	return drm_panel_get_modes(tcon->panel);
+	return drm_panel_get_modes(rgb->panel);
 }
 
 static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
@@ -114,9 +114,8 @@ static void
 sun4i_rgb_connector_destroy(struct drm_connector *connector)
 {
 	struct sun4i_rgb *rgb = drm_connector_to_sun4i_rgb(connector);
-	struct sun4i_tcon *tcon = rgb->tcon;
 
-	drm_panel_detach(tcon->panel);
+	drm_panel_detach(rgb->panel);
 	drm_connector_cleanup(connector);
 }
 
@@ -131,26 +130,24 @@ static const struct drm_connector_funcs sun4i_rgb_con_funcs = {
 static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
 {
 	struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(encoder);
-	struct sun4i_tcon *tcon = rgb->tcon;
 
 	DRM_DEBUG_DRIVER("Enabling RGB output\n");
 
-	if (tcon->panel) {
-		drm_panel_prepare(tcon->panel);
-		drm_panel_enable(tcon->panel);
+	if (rgb->panel) {
+		drm_panel_prepare(rgb->panel);
+		drm_panel_enable(rgb->panel);
 	}
 }
 
 static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
 {
 	struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(encoder);
-	struct sun4i_tcon *tcon = rgb->tcon;
 
 	DRM_DEBUG_DRIVER("Disabling RGB output\n");
 
-	if (tcon->panel) {
-		drm_panel_disable(tcon->panel);
-		drm_panel_unprepare(tcon->panel);
+	if (rgb->panel) {
+		drm_panel_disable(rgb->panel);
+		drm_panel_unprepare(rgb->panel);
 	}
 }
 
@@ -183,7 +180,7 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
 	encoder = &rgb->encoder;
 
 	ret = drm_of_find_panel_or_bridge(tcon->dev->of_node, 1, 0,
-					  &tcon->panel, &bridge);
+					  &rgb->panel, &bridge);
 	if (ret) {
 		dev_info(drm->dev, "No panel or bridge found... RGB output disabled\n");
 		return 0;
@@ -204,7 +201,7 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
 	/* The RGB encoder can only work with the TCON channel 0 */
 	rgb->encoder.possible_crtcs = drm_crtc_mask(&tcon->crtc->crtc);
 
-	if (tcon->panel) {
+	if (rgb->panel) {
 		drm_connector_helper_add(&rgb->connector,
 					 &sun4i_rgb_con_helper_funcs);
 		ret = drm_connector_init(drm, &rgb->connector,
@@ -218,7 +215,7 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
 		drm_connector_attach_encoder(&rgb->connector,
 						  &rgb->encoder);
 
-		ret = drm_panel_attach(tcon->panel, &rgb->connector);
+		ret = drm_panel_attach(rgb->panel, &rgb->connector);
 		if (ret) {
 			dev_err(drm->dev, "Couldn't attach our panel\n");
 			goto err_cleanup_connector;
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index b5214d71610f..84cfb1952ff7 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -257,8 +257,6 @@ struct sun4i_tcon {
 	struct reset_control		*lcd_rst;
 	struct reset_control		*lvds_rst;
 
-	struct drm_panel		*panel;
-
 	/* Platform adjustments */
 	const struct sun4i_tcon_quirks	*quirks;
 
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] drm/sun4i: rgb: Store the bridge pointer

[Maxime Ripard]

We'll need the bridge pointer, if any, in the mode_valid callback in
addition to the init function. Store the pointer to the bridge in the
rgb private structure.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index d181ee399b03..8a43ed46af82 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -28,6 +28,7 @@ struct sun4i_rgb {
 
 	struct sun4i_tcon	*tcon;
 	struct drm_panel	*panel;
+	struct drm_bridge	*bridge;
 };
 
 static inline struct sun4i_rgb *
@@ -169,7 +170,6 @@ static struct drm_encoder_funcs sun4i_rgb_enc_funcs = {
 int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
 {
 	struct drm_encoder *encoder;
-	struct drm_bridge *bridge;
 	struct sun4i_rgb *rgb;
 	int ret;
 
@@ -180,7 +180,7 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
 	encoder = &rgb->encoder;
 
 	ret = drm_of_find_panel_or_bridge(tcon->dev->of_node, 1, 0,
-					  &rgb->panel, &bridge);
+					  &rgb->panel, &rgb->bridge);
 	if (ret) {
 		dev_info(drm->dev, "No panel or bridge found... RGB output disabled\n");
 		return 0;
@@ -222,8 +222,8 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
 		}
 	}
 
-	if (bridge) {
-		ret = drm_bridge_attach(encoder, bridge, NULL);
+	if (rgb->bridge) {
+		ret = drm_bridge_attach(encoder, rgb->bridge, NULL);
 		if (ret) {
 			dev_err(drm->dev, "Couldn't attach our bridge\n");
 			goto err_cleanup_connector;
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] drm/sun4i: Move rate variables to long long

[Maxime Ripard]

Our clock rate variables are getting pretty close to the LONG_MAX / ULONG_MAX
limit, especially since we will start doing arithmetic on it. Move those
types to unsigned long long to be sure we don't overflow their type.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index 8a43ed46af82..893b6e6d4d85 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -60,8 +60,8 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
 	struct sun4i_tcon *tcon = rgb->tcon;
 	u32 hsync = mode->hsync_end - mode->hsync_start;
 	u32 vsync = mode->vsync_end - mode->vsync_start;
-	unsigned long rate = mode->clock * 1000;
-	long rounded_rate;
+	unsigned long long rate = mode->clock * 1000;
+	unsigned long long rounded_rate;
 
 	DRM_DEBUG_DRIVER("Validating modes...\n");
 
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] drm/sun4i: rgb: Change the pixel clock validation check

[Maxime Ripard]

The current code, since commit bb43d40d7c83 ("drm/sun4i: rgb: Validate the
clock rate"), perform some validation on the pixel clock to filter out the
EDID modes provided by monitors (through bridges) that we wouldn't be able
to reach. For the usual modes, we're able to generate a perfect clock rate,
so a strict check was enough.

However, this had the side effect of preventing displays that would work
otherwise to operate properly, since we would pretty much never be able to
generate an exact rate for those displays, even though we would fall within
that panel tolerance.

This was also shown to happen for unusual modes exposed through EDIDs, for
example on eDP panels.

We can work around this by simplifying a bit the problem: no panels we've
encountered so far actually needed that check. All of them are tied to a
particular board when it is produced, and made to work with the Allwinner
BSP. That pretty much guarantees that we never have a pixel clock out of
reach.

On the other hand, the EDIDs modes that needed to be validated have always
been exposed through bridges.

Let's just use that metric to instead of validating all modes, only
validate modes when we have a bridge attached. It should be good enough for
now, while we still have room for improvements or refinements using the
display_timings structure for example for panels.

We also add a tolerance for EDID-based modes instead of doing a strict
check. This tolerance is of 0.5% which is the one advertised in the VESA
DVT and CVT specs. If that needed to be extended in the future, we can add
a custom module parameter to relax it a bit.

Fixes: bb43d40d7c83 ("drm/sun4i: rgb: Validate the clock rate")
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 37 ++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index 893b6e6d4d85..05beefe93989 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -53,6 +53,14 @@ static int sun4i_rgb_get_modes(struct drm_connector *connector)
 	return drm_panel_get_modes(rgb->panel);
 }
 
+/*
+ * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
+ * CVT spec reuses that tolerance in its examples, so it looks to be a
+ * good default tolerance for the EDID-based modes. Define it to 5 per
+ * mille to avoid floating point operations.
+ */
+DEFINE SUN4I_RGB_DOTCLOCK_TOLERANCE	5
+
 static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
 						 const struct drm_display_mode *mode)
 {
@@ -61,6 +69,7 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
 	u32 hsync = mode->hsync_end - mode->hsync_start;
 	u32 vsync = mode->vsync_end - mode->vsync_start;
 	unsigned long long rate = mode->clock * 1000;
+	unsigned long long lowest, highest;
 	unsigned long long rounded_rate;
 
 	DRM_DEBUG_DRIVER("Validating modes...\n");
@@ -93,15 +102,39 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
 
 	DRM_DEBUG_DRIVER("Vertical parameters OK\n");
 
+	/*
+	 * TODO: We should use the struct display_timing if available
+	 * and / or trying to stretch the timings within that
+	 * tolerancy to take care of panels that we wouldn't be able
+	 * to have a exact match for.
+	 */
+	if (rgb->panel) {
+		DRM_DEBUG_DRIVER("RGB panel used, skipping clock rate checks");
+		goto out;
+	}
+
+	/*
+	 * That shouldn't ever happen unless something is really wrong, but it
+	 * doesn't harm to check.
+	 */
+	if (!rgb->bridge)
+		goto out;
+
 	tcon->dclk_min_div = 6;
 	tcon->dclk_max_div = 127;
 	rounded_rate = clk_round_rate(tcon->dclk, rate);
-	if (rounded_rate < rate)
+
+	lowest = rate * (1000 - SUN4I_RGB_DOTCLOCK_TOLERANCE);
+	do_div(lowest, 1000);
+	if (rounded_rate < lowest)
 		return MODE_CLOCK_LOW;
 
-	if (rounded_rate > rate)
+	highest = rate * (1000 + SUN4I_RGB_DOTCLOCK_TOLERANCE);
+	do_div(highest, 1000);
+	if (rounded_rate > highest)
 		return MODE_CLOCK_HIGH;
 
+out:
 	DRM_DEBUG_DRIVER("Clock rate OK\n");
 
 	return MODE_OK;
-- 
git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/4] drm/sun4i: rgb: Change the pixel clock validation check

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

Hi Maxime,

I love your patch! Yet something to improve:

[auto build test ERROR on ]

url:    https://github.com/0day-ci/linux/commits/Maxime-Ripard/drm-sun4i-rgb-Relax-the-pixel-clock-check/20190227-012757
base:    
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.2.0 make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

>> drivers/gpu//drm/sun4i/sun4i_rgb.c:62:1: error: unknown type name 'DEFINE'; did you mean 'EMFILE'?
    DEFINE SUN4I_RGB_DOTCLOCK_TOLERANCE 5
    ^~~~~~
    EMFILE
   drivers/gpu//drm/sun4i/sun4i_rgb.c:62:37: error: expected '=', ',', ';', 'asm' or '__attribute__' before numeric constant
    DEFINE SUN4I_RGB_DOTCLOCK_TOLERANCE 5
                                        ^
   drivers/gpu//drm/sun4i/sun4i_rgb.c:191:16: error: 'sun4i_rgb_mode_valid' undeclared here (not in a function); did you mean 'sun4i_rgb_con_funcs'?
     .mode_valid = sun4i_rgb_mode_valid,
                   ^~~~~~~~~~~~~~~~~~~~
                   sun4i_rgb_con_funcs

vim +62 drivers/gpu//drm/sun4i/sun4i_rgb.c

    55	
    56	/*
    57	 * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
    58	 * CVT spec reuses that tolerance in its examples, so it looks to be a
    59	 * good default tolerance for the EDID-based modes. Define it to 5 per
    60	 * mille to avoid floating point operations.
    61	 */
  > 62	DEFINE SUN4I_RGB_DOTCLOCK_TOLERANCE	5
    63	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 63059 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] drm/sun4i: rgb: Relax the pixel clock check

[Vasily Khoruzhick]

On Tue, Feb 26, 2019 at 6:25 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> Hi,
>
> This implements roughly what we discussed as part of the ANX6345 series to
> relax the pixel clock validation while still filtering out the modes we
> can't reach using the bridges.
>
> Let me know what you think,
> Maxime

With last patch fixed (s/DEFINE/#define), for whole series:

Tested-by: Vasily Khoruzhick <anarsoul@gmail.com> # tested on pinebook

> Maxime Ripard (4):
>   drm/sun4i: Move the panel pointer from the TCON to the encoders
>   drm/sun4i: rgb: Store the bridge pointer
>   drm/sun4i: Move rate variables to long long
>   drm/sun4i: rgb: Change the pixel clock validation check
>
>  drivers/gpu/drm/sun4i/sun4i_lvds.c | 29 ++++--------
>  drivers/gpu/drm/sun4i/sun4i_rgb.c  | 74 +++++++++++++++++++++----------
>  drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 +-
>  3 files changed, 64 insertions(+), 41 deletions(-)
>
> base-commit: bfeffd155283772bbe78c6a05dec7c0128ee500c
> --
> git-series 0.9.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] drm/sun4i: Move the panel pointer from the TCON to the encoders

[Paul Kocialkowski]

Hi,

On Tue, 2019-02-26 at 15:25 +0100, Maxime Ripard wrote:
> The TCON driver used to need the panel pointer in order to configure the
> tcon according to the various parameters of the panel. However, this has
> evolved over time (especially to support bridges), and therefore the panel
> pointer isn't needed anymore by the TCON driver.
> 
> Move that pointer to the LVDS and RGB encoders drivers.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

That definitely makes good sense to me!

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> ---
>  drivers/gpu/drm/sun4i/sun4i_lvds.c | 29 ++++++++++++-----------------
>  drivers/gpu/drm/sun4i/sun4i_rgb.c  | 27 ++++++++++++---------------
>  drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 --
>  3 files changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c b/drivers/gpu/drm/sun4i/sun4i_lvds.c
> index e7eb0d1e17be..5696f55db5cd 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_lvds.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c
> @@ -20,7 +20,7 @@ struct sun4i_lvds {
>  	struct drm_connector	connector;
>  	struct drm_encoder	encoder;
>  
> -	struct sun4i_tcon	*tcon;
> +	struct drm_panel	*panel;
>  };
>  
>  static inline struct sun4i_lvds *
> @@ -41,9 +41,8 @@ static int sun4i_lvds_get_modes(struct drm_connector *connector)
>  {
>  	struct sun4i_lvds *lvds =
>  		drm_connector_to_sun4i_lvds(connector);
> -	struct sun4i_tcon *tcon = lvds->tcon;
>  
> -	return drm_panel_get_modes(tcon->panel);
> +	return drm_panel_get_modes(lvds->panel);
>  }
>  
>  static struct drm_connector_helper_funcs sun4i_lvds_con_helper_funcs = {
> @@ -54,9 +53,8 @@ static void
>  sun4i_lvds_connector_destroy(struct drm_connector *connector)
>  {
>  	struct sun4i_lvds *lvds = drm_connector_to_sun4i_lvds(connector);
> -	struct sun4i_tcon *tcon = lvds->tcon;
>  
> -	drm_panel_detach(tcon->panel);
> +	drm_panel_detach(lvds->panel);
>  	drm_connector_cleanup(connector);
>  }
>  
> @@ -71,26 +69,24 @@ static const struct drm_connector_funcs sun4i_lvds_con_funcs = {
>  static void sun4i_lvds_encoder_enable(struct drm_encoder *encoder)
>  {
>  	struct sun4i_lvds *lvds = drm_encoder_to_sun4i_lvds(encoder);
> -	struct sun4i_tcon *tcon = lvds->tcon;
>  
>  	DRM_DEBUG_DRIVER("Enabling LVDS output\n");
>  
> -	if (tcon->panel) {
> -		drm_panel_prepare(tcon->panel);
> -		drm_panel_enable(tcon->panel);
> +	if (lvds->panel) {
> +		drm_panel_prepare(lvds->panel);
> +		drm_panel_enable(lvds->panel);
>  	}
>  }
>  
>  static void sun4i_lvds_encoder_disable(struct drm_encoder *encoder)
>  {
>  	struct sun4i_lvds *lvds = drm_encoder_to_sun4i_lvds(encoder);
> -	struct sun4i_tcon *tcon = lvds->tcon;
>  
>  	DRM_DEBUG_DRIVER("Disabling LVDS output\n");
>  
> -	if (tcon->panel) {
> -		drm_panel_disable(tcon->panel);
> -		drm_panel_unprepare(tcon->panel);
> +	if (lvds->panel) {
> +		drm_panel_disable(lvds->panel);
> +		drm_panel_unprepare(lvds->panel);
>  	}
>  }
>  
> @@ -113,11 +109,10 @@ int sun4i_lvds_init(struct drm_device *drm, struct sun4i_tcon *tcon)
>  	lvds = devm_kzalloc(drm->dev, sizeof(*lvds), GFP_KERNEL);
>  	if (!lvds)
>  		return -ENOMEM;
> -	lvds->tcon = tcon;
>  	encoder = &lvds->encoder;
>  
>  	ret = drm_of_find_panel_or_bridge(tcon->dev->of_node, 1, 0,
> -					  &tcon->panel, &bridge);
> +					  &lvds->panel, &bridge);
>  	if (ret) {
>  		dev_info(drm->dev, "No panel or bridge found... LVDS output disabled\n");
>  		return 0;
> @@ -138,7 +133,7 @@ int sun4i_lvds_init(struct drm_device *drm, struct sun4i_tcon *tcon)
>  	/* The LVDS encoder can only work with the TCON channel 0 */
>  	lvds->encoder.possible_crtcs = drm_crtc_mask(&tcon->crtc->crtc);
>  
> -	if (tcon->panel) {
> +	if (lvds->panel) {
>  		drm_connector_helper_add(&lvds->connector,
>  					 &sun4i_lvds_con_helper_funcs);
>  		ret = drm_connector_init(drm, &lvds->connector,
> @@ -152,7 +147,7 @@ int sun4i_lvds_init(struct drm_device *drm, struct sun4i_tcon *tcon)
>  		drm_connector_attach_encoder(&lvds->connector,
>  						  &lvds->encoder);
>  
> -		ret = drm_panel_attach(tcon->panel, &lvds->connector);
> +		ret = drm_panel_attach(lvds->panel, &lvds->connector);
>  		if (ret) {
>  			dev_err(drm->dev, "Couldn't attach our panel\n");
>  			goto err_cleanup_connector;
> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> index f4a22689eb54..d181ee399b03 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> @@ -27,6 +27,7 @@ struct sun4i_rgb {
>  	struct drm_encoder	encoder;
>  
>  	struct sun4i_tcon	*tcon;
> +	struct drm_panel	*panel;
>  };
>  
>  static inline struct sun4i_rgb *
> @@ -47,9 +48,8 @@ static int sun4i_rgb_get_modes(struct drm_connector *connector)
>  {
>  	struct sun4i_rgb *rgb =
>  		drm_connector_to_sun4i_rgb(connector);
> -	struct sun4i_tcon *tcon = rgb->tcon;
>  
> -	return drm_panel_get_modes(tcon->panel);
> +	return drm_panel_get_modes(rgb->panel);
>  }
>  
>  static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
> @@ -114,9 +114,8 @@ static void
>  sun4i_rgb_connector_destroy(struct drm_connector *connector)
>  {
>  	struct sun4i_rgb *rgb = drm_connector_to_sun4i_rgb(connector);
> -	struct sun4i_tcon *tcon = rgb->tcon;
>  
> -	drm_panel_detach(tcon->panel);
> +	drm_panel_detach(rgb->panel);
>  	drm_connector_cleanup(connector);
>  }
>  
> @@ -131,26 +130,24 @@ static const struct drm_connector_funcs sun4i_rgb_con_funcs = {
>  static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
>  {
>  	struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(encoder);
> -	struct sun4i_tcon *tcon = rgb->tcon;
>  
>  	DRM_DEBUG_DRIVER("Enabling RGB output\n");
>  
> -	if (tcon->panel) {
> -		drm_panel_prepare(tcon->panel);
> -		drm_panel_enable(tcon->panel);
> +	if (rgb->panel) {
> +		drm_panel_prepare(rgb->panel);
> +		drm_panel_enable(rgb->panel);
>  	}
>  }
>  
>  static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
>  {
>  	struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(encoder);
> -	struct sun4i_tcon *tcon = rgb->tcon;
>  
>  	DRM_DEBUG_DRIVER("Disabling RGB output\n");
>  
> -	if (tcon->panel) {
> -		drm_panel_disable(tcon->panel);
> -		drm_panel_unprepare(tcon->panel);
> +	if (rgb->panel) {
> +		drm_panel_disable(rgb->panel);
> +		drm_panel_unprepare(rgb->panel);
>  	}
>  }
>  
> @@ -183,7 +180,7 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
>  	encoder = &rgb->encoder;
>  
>  	ret = drm_of_find_panel_or_bridge(tcon->dev->of_node, 1, 0,
> -					  &tcon->panel, &bridge);
> +					  &rgb->panel, &bridge);
>  	if (ret) {
>  		dev_info(drm->dev, "No panel or bridge found... RGB output disabled\n");
>  		return 0;
> @@ -204,7 +201,7 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
>  	/* The RGB encoder can only work with the TCON channel 0 */
>  	rgb->encoder.possible_crtcs = drm_crtc_mask(&tcon->crtc->crtc);
>  
> -	if (tcon->panel) {
> +	if (rgb->panel) {
>  		drm_connector_helper_add(&rgb->connector,
>  					 &sun4i_rgb_con_helper_funcs);
>  		ret = drm_connector_init(drm, &rgb->connector,
> @@ -218,7 +215,7 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
>  		drm_connector_attach_encoder(&rgb->connector,
>  						  &rgb->encoder);
>  
> -		ret = drm_panel_attach(tcon->panel, &rgb->connector);
> +		ret = drm_panel_attach(rgb->panel, &rgb->connector);
>  		if (ret) {
>  			dev_err(drm->dev, "Couldn't attach our panel\n");
>  			goto err_cleanup_connector;
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> index b5214d71610f..84cfb1952ff7 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> @@ -257,8 +257,6 @@ struct sun4i_tcon {
>  	struct reset_control		*lcd_rst;
>  	struct reset_control		*lvds_rst;
>  
> -	struct drm_panel		*panel;
> -
>  	/* Platform adjustments */
>  	const struct sun4i_tcon_quirks	*quirks;
>  
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] drm/sun4i: rgb: Store the bridge pointer

[Paul Kocialkowski]

Hi,

On Tue, 2019-02-26 at 15:25 +0100, Maxime Ripard wrote:
> We'll need the bridge pointer, if any, in the mode_valid callback in
> addition to the init function. Store the pointer to the bridge in the
> rgb private structure.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> ---
>  drivers/gpu/drm/sun4i/sun4i_rgb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> index d181ee399b03..8a43ed46af82 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> @@ -28,6 +28,7 @@ struct sun4i_rgb {
>  
>  	struct sun4i_tcon	*tcon;
>  	struct drm_panel	*panel;
> +	struct drm_bridge	*bridge;
>  };
>  
>  static inline struct sun4i_rgb *
> @@ -169,7 +170,6 @@ static struct drm_encoder_funcs sun4i_rgb_enc_funcs = {
>  int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
>  {
>  	struct drm_encoder *encoder;
> -	struct drm_bridge *bridge;
>  	struct sun4i_rgb *rgb;
>  	int ret;
>  
> @@ -180,7 +180,7 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
>  	encoder = &rgb->encoder;
>  
>  	ret = drm_of_find_panel_or_bridge(tcon->dev->of_node, 1, 0,
> -					  &rgb->panel, &bridge);
> +					  &rgb->panel, &rgb->bridge);
>  	if (ret) {
>  		dev_info(drm->dev, "No panel or bridge found... RGB output disabled\n");
>  		return 0;
> @@ -222,8 +222,8 @@ int sun4i_rgb_init(struct drm_device *drm, struct sun4i_tcon *tcon)
>  		}
>  	}
>  
> -	if (bridge) {
> -		ret = drm_bridge_attach(encoder, bridge, NULL);
> +	if (rgb->bridge) {
> +		ret = drm_bridge_attach(encoder, rgb->bridge, NULL);
>  		if (ret) {
>  			dev_err(drm->dev, "Couldn't attach our bridge\n");
>  			goto err_cleanup_connector;
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] drm/sun4i: Move rate variables to long long

[Paul Kocialkowski]

Hi,

On Tue, 2019-02-26 at 15:25 +0100, Maxime Ripard wrote:
> Our clock rate variables are getting pretty close to the LONG_MAX / ULONG_MAX
> limit, especially since we will start doing arithmetic on it. Move those
> types to unsigned long long to be sure we don't overflow their type.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> ---
>  drivers/gpu/drm/sun4i/sun4i_rgb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> index 8a43ed46af82..893b6e6d4d85 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> @@ -60,8 +60,8 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
>  	struct sun4i_tcon *tcon = rgb->tcon;
>  	u32 hsync = mode->hsync_end - mode->hsync_start;
>  	u32 vsync = mode->vsync_end - mode->vsync_start;
> -	unsigned long rate = mode->clock * 1000;
> -	long rounded_rate;
> +	unsigned long long rate = mode->clock * 1000;
> +	unsigned long long rounded_rate;
>  
>  	DRM_DEBUG_DRIVER("Validating modes...\n");
>  
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/4] drm/sun4i: rgb: Change the pixel clock validation check

[Paul Kocialkowski]

Hi,

On Tue, 2019-02-26 at 15:25 +0100, Maxime Ripard wrote:
> The current code, since commit bb43d40d7c83 ("drm/sun4i: rgb: Validate the
> clock rate"), perform some validation on the pixel clock to filter out the
> EDID modes provided by monitors (through bridges) that we wouldn't be able
> to reach. For the usual modes, we're able to generate a perfect clock rate,
> so a strict check was enough.
> 
> However, this had the side effect of preventing displays that would work
> otherwise to operate properly, since we would pretty much never be able to
> generate an exact rate for those displays, even though we would fall within
> that panel tolerance.
> 
> This was also shown to happen for unusual modes exposed through EDIDs, for
> example on eDP panels.
> 
> We can work around this by simplifying a bit the problem: no panels we've
> encountered so far actually needed that check. All of them are tied to a
> particular board when it is produced, and made to work with the Allwinner
> BSP. That pretty much guarantees that we never have a pixel clock out of
> reach.
> 
> On the other hand, the EDIDs modes that needed to be validated have always
> been exposed through bridges.
> 
> Let's just use that metric to instead of validating all modes, only
> validate modes when we have a bridge attached. It should be good enough for
> now, while we still have room for improvements or refinements using the
> display_timings structure for example for panels.
> 
> We also add a tolerance for EDID-based modes instead of doing a strict
> check. This tolerance is of 0.5% which is the one advertised in the VESA
> DVT and CVT specs. If that needed to be extended in the future, we can add
> a custom module parameter to relax it a bit.
> 
> Fixes: bb43d40d7c83 ("drm/sun4i: rgb: Validate the clock rate")
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

With the define fixed and given the comment below, this is:

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

> ---
>  drivers/gpu/drm/sun4i/sun4i_rgb.c | 37 ++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> index 893b6e6d4d85..05beefe93989 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
> @@ -53,6 +53,14 @@ static int sun4i_rgb_get_modes(struct drm_connector *connector)
>  	return drm_panel_get_modes(rgb->panel);
>  }
>  
> +/*
> + * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
> + * CVT spec reuses that tolerance in its examples, so it looks to be a
> + * good default tolerance for the EDID-based modes. Define it to 5 per
> + * mille to avoid floating point operations.
> + */
> +DEFINE SUN4I_RGB_DOTCLOCK_TOLERANCE	5

It could be nice to have some kind of unit made explicit in the define.
Maybe something like SUN4I_RGB_DOTCLOCK_TOLERANCE_PER_THOUSAND.

That's just a suggestion, feel free to take it or leave it :)

Cheers,

Paul

> +
>  static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
>  						 const struct drm_display_mode *mode)
>  {
> @@ -61,6 +69,7 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
>  	u32 hsync = mode->hsync_end - mode->hsync_start;
>  	u32 vsync = mode->vsync_end - mode->vsync_start;
>  	unsigned long long rate = mode->clock * 1000;
> +	unsigned long long lowest, highest;
>  	unsigned long long rounded_rate;
>  
>  	DRM_DEBUG_DRIVER("Validating modes...\n");
> @@ -93,15 +102,39 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
>  
>  	DRM_DEBUG_DRIVER("Vertical parameters OK\n");
>  
> +	/*
> +	 * TODO: We should use the struct display_timing if available
> +	 * and / or trying to stretch the timings within that
> +	 * tolerancy to take care of panels that we wouldn't be able
> +	 * to have a exact match for.
> +	 */
> +	if (rgb->panel) {
> +		DRM_DEBUG_DRIVER("RGB panel used, skipping clock rate checks");
> +		goto out;
> +	}
> +
> +	/*
> +	 * That shouldn't ever happen unless something is really wrong, but it
> +	 * doesn't harm to check.
> +	 */
> +	if (!rgb->bridge)
> +		goto out;
> +
>  	tcon->dclk_min_div = 6;
>  	tcon->dclk_max_div = 127;
>  	rounded_rate = clk_round_rate(tcon->dclk, rate);
> -	if (rounded_rate < rate)
> +
> +	lowest = rate * (1000 - SUN4I_RGB_DOTCLOCK_TOLERANCE);
> +	do_div(lowest, 1000);
> +	if (rounded_rate < lowest)
>  		return MODE_CLOCK_LOW;
>  
> -	if (rounded_rate > rate)
> +	highest = rate * (1000 + SUN4I_RGB_DOTCLOCK_TOLERANCE);
> +	do_div(highest, 1000);
> +	if (rounded_rate > highest)
>  		return MODE_CLOCK_HIGH;
>  
> +out:
>  	DRM_DEBUG_DRIVER("Clock rate OK\n");
>  
>  	return MODE_OK;
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] drm/sun4i: rgb: Relax the pixel clock check

[Maxime Ripard]

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

On Tue, Feb 26, 2019 at 03:25:45PM +0100, Maxime Ripard wrote:
> Hi,
> 
> This implements roughly what we discussed as part of the ANX6345 series to
> relax the pixel clock validation while still filtering out the modes we
> can't reach using the bridges.
> 
> Let me know what you think,
> Maxime

Applied with Paul comments taken into account, thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel