[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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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: 159 bytes --]

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 11258 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: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-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=arm 

All errors (new ones prefixed by >>):

>> drivers/gpu//drm/sun4i/sun4i_rgb.c:62:1: error: unknown type name 'DEFINE'; did you mean 'EPIPE'?
    DEFINE SUN4I_RGB_DOTCLOCK_TOLERANCE 5
    ^~~~~~
    EPIPE
>> 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 +191 drivers/gpu//drm/sun4i/sun4i_rgb.c

29e57fab9 Maxime Ripard  2015-10-29   55  
3c3671edd Maxime Ripard  2019-02-26   56  /*
3c3671edd Maxime Ripard  2019-02-26   57   * VESA DMT defines a tolerance of 0.5% on the pixel clock, while the
3c3671edd Maxime Ripard  2019-02-26   58   * CVT spec reuses that tolerance in its examples, so it looks to be a
3c3671edd Maxime Ripard  2019-02-26   59   * good default tolerance for the EDID-based modes. Define it to 5 per
3c3671edd Maxime Ripard  2019-02-26   60   * mille to avoid floating point operations.
3c3671edd Maxime Ripard  2019-02-26   61   */
3c3671edd Maxime Ripard  2019-02-26  @62  DEFINE SUN4I_RGB_DOTCLOCK_TOLERANCE	5
3c3671edd Maxime Ripard  2019-02-26   63  
cde8b7548 Giulio Benetti 2018-03-13   64  static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc,
cde8b7548 Giulio Benetti 2018-03-13   65  						 const struct drm_display_mode *mode)
29e57fab9 Maxime Ripard  2015-10-29   66  {
cde8b7548 Giulio Benetti 2018-03-13   67  	struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(crtc);
b9c8506cb Chen-Yu Tsai   2017-02-23   68  	struct sun4i_tcon *tcon = rgb->tcon;
29e57fab9 Maxime Ripard  2015-10-29   69  	u32 hsync = mode->hsync_end - mode->hsync_start;
29e57fab9 Maxime Ripard  2015-10-29   70  	u32 vsync = mode->vsync_end - mode->vsync_start;
7659a82a3 Maxime Ripard  2019-02-26   71  	unsigned long long rate = mode->clock * 1000;
3c3671edd Maxime Ripard  2019-02-26   72  	unsigned long long lowest, highest;
7659a82a3 Maxime Ripard  2019-02-26   73  	unsigned long long rounded_rate;
29e57fab9 Maxime Ripard  2015-10-29   74  
29e57fab9 Maxime Ripard  2015-10-29   75  	DRM_DEBUG_DRIVER("Validating modes...\n");
29e57fab9 Maxime Ripard  2015-10-29   76  
29e57fab9 Maxime Ripard  2015-10-29   77  	if (hsync < 1)
29e57fab9 Maxime Ripard  2015-10-29   78  		return MODE_HSYNC_NARROW;
29e57fab9 Maxime Ripard  2015-10-29   79  
29e57fab9 Maxime Ripard  2015-10-29   80  	if (hsync > 0x3ff)
29e57fab9 Maxime Ripard  2015-10-29   81  		return MODE_HSYNC_WIDE;
29e57fab9 Maxime Ripard  2015-10-29   82  
29e57fab9 Maxime Ripard  2015-10-29   83  	if ((mode->hdisplay < 1) || (mode->htotal < 1))
29e57fab9 Maxime Ripard  2015-10-29   84  		return MODE_H_ILLEGAL;
29e57fab9 Maxime Ripard  2015-10-29   85  
29e57fab9 Maxime Ripard  2015-10-29   86  	if ((mode->hdisplay > 0x7ff) || (mode->htotal > 0xfff))
29e57fab9 Maxime Ripard  2015-10-29   87  		return MODE_BAD_HVALUE;
29e57fab9 Maxime Ripard  2015-10-29   88  
29e57fab9 Maxime Ripard  2015-10-29   89  	DRM_DEBUG_DRIVER("Horizontal parameters OK\n");
29e57fab9 Maxime Ripard  2015-10-29   90  
29e57fab9 Maxime Ripard  2015-10-29   91  	if (vsync < 1)
29e57fab9 Maxime Ripard  2015-10-29   92  		return MODE_VSYNC_NARROW;
29e57fab9 Maxime Ripard  2015-10-29   93  
29e57fab9 Maxime Ripard  2015-10-29   94  	if (vsync > 0x3ff)
29e57fab9 Maxime Ripard  2015-10-29   95  		return MODE_VSYNC_WIDE;
29e57fab9 Maxime Ripard  2015-10-29   96  
29e57fab9 Maxime Ripard  2015-10-29   97  	if ((mode->vdisplay < 1) || (mode->vtotal < 1))
29e57fab9 Maxime Ripard  2015-10-29   98  		return MODE_V_ILLEGAL;
29e57fab9 Maxime Ripard  2015-10-29   99  
29e57fab9 Maxime Ripard  2015-10-29  100  	if ((mode->vdisplay > 0x7ff) || (mode->vtotal > 0xfff))
29e57fab9 Maxime Ripard  2015-10-29  101  		return MODE_BAD_VVALUE;
29e57fab9 Maxime Ripard  2015-10-29  102  
29e57fab9 Maxime Ripard  2015-10-29  103  	DRM_DEBUG_DRIVER("Vertical parameters OK\n");
29e57fab9 Maxime Ripard  2015-10-29  104  
3c3671edd Maxime Ripard  2019-02-26  105  	/*
3c3671edd Maxime Ripard  2019-02-26  106  	 * TODO: We should use the struct display_timing if available
3c3671edd Maxime Ripard  2019-02-26  107  	 * and / or trying to stretch the timings within that
3c3671edd Maxime Ripard  2019-02-26  108  	 * tolerancy to take care of panels that we wouldn't be able
3c3671edd Maxime Ripard  2019-02-26  109  	 * to have a exact match for.
3c3671edd Maxime Ripard  2019-02-26  110  	 */
3c3671edd Maxime Ripard  2019-02-26  111  	if (rgb->panel) {
3c3671edd Maxime Ripard  2019-02-26  112  		DRM_DEBUG_DRIVER("RGB panel used, skipping clock rate checks");
3c3671edd Maxime Ripard  2019-02-26  113  		goto out;
3c3671edd Maxime Ripard  2019-02-26  114  	}
3c3671edd Maxime Ripard  2019-02-26  115  
3c3671edd Maxime Ripard  2019-02-26  116  	/*
3c3671edd Maxime Ripard  2019-02-26  117  	 * That shouldn't ever happen unless something is really wrong, but it
3c3671edd Maxime Ripard  2019-02-26  118  	 * doesn't harm to check.
3c3671edd Maxime Ripard  2019-02-26  119  	 */
3c3671edd Maxime Ripard  2019-02-26  120  	if (!rgb->bridge)
3c3671edd Maxime Ripard  2019-02-26  121  		goto out;
3c3671edd Maxime Ripard  2019-02-26  122  
5af894bd2 Maxime Ripard  2018-02-21  123  	tcon->dclk_min_div = 6;
5af894bd2 Maxime Ripard  2018-02-21  124  	tcon->dclk_max_div = 127;
bb43d40d7 Maxime Ripard  2016-04-21  125  	rounded_rate = clk_round_rate(tcon->dclk, rate);
3c3671edd Maxime Ripard  2019-02-26  126  
3c3671edd Maxime Ripard  2019-02-26  127  	lowest = rate * (1000 - SUN4I_RGB_DOTCLOCK_TOLERANCE);
3c3671edd Maxime Ripard  2019-02-26  128  	do_div(lowest, 1000);
3c3671edd Maxime Ripard  2019-02-26  129  	if (rounded_rate < lowest)
bb43d40d7 Maxime Ripard  2016-04-21  130  		return MODE_CLOCK_LOW;
bb43d40d7 Maxime Ripard  2016-04-21  131  
3c3671edd Maxime Ripard  2019-02-26  132  	highest = rate * (1000 + SUN4I_RGB_DOTCLOCK_TOLERANCE);
3c3671edd Maxime Ripard  2019-02-26  133  	do_div(highest, 1000);
3c3671edd Maxime Ripard  2019-02-26  134  	if (rounded_rate > highest)
bb43d40d7 Maxime Ripard  2016-04-21  135  		return MODE_CLOCK_HIGH;
bb43d40d7 Maxime Ripard  2016-04-21  136  
3c3671edd Maxime Ripard  2019-02-26  137  out:
bb43d40d7 Maxime Ripard  2016-04-21  138  	DRM_DEBUG_DRIVER("Clock rate OK\n");
bb43d40d7 Maxime Ripard  2016-04-21  139  
29e57fab9 Maxime Ripard  2015-10-29  140  	return MODE_OK;
29e57fab9 Maxime Ripard  2015-10-29  141  }
29e57fab9 Maxime Ripard  2015-10-29  142  
29e57fab9 Maxime Ripard  2015-10-29  143  static struct drm_connector_helper_funcs sun4i_rgb_con_helper_funcs = {
29e57fab9 Maxime Ripard  2015-10-29  144  	.get_modes	= sun4i_rgb_get_modes,
29e57fab9 Maxime Ripard  2015-10-29  145  };
29e57fab9 Maxime Ripard  2015-10-29  146  
29e57fab9 Maxime Ripard  2015-10-29  147  static void
29e57fab9 Maxime Ripard  2015-10-29  148  sun4i_rgb_connector_destroy(struct drm_connector *connector)
29e57fab9 Maxime Ripard  2015-10-29  149  {
29e57fab9 Maxime Ripard  2015-10-29  150  	struct sun4i_rgb *rgb = drm_connector_to_sun4i_rgb(connector);
29e57fab9 Maxime Ripard  2015-10-29  151  
b413dde1a Maxime Ripard  2019-02-26  152  	drm_panel_detach(rgb->panel);
29e57fab9 Maxime Ripard  2015-10-29  153  	drm_connector_cleanup(connector);
29e57fab9 Maxime Ripard  2015-10-29  154  }
29e57fab9 Maxime Ripard  2015-10-29  155  
32b4d5756 Bhumika Goyal  2017-08-08  156  static const struct drm_connector_funcs sun4i_rgb_con_funcs = {
29e57fab9 Maxime Ripard  2015-10-29  157  	.fill_modes		= drm_helper_probe_single_connector_modes,
29e57fab9 Maxime Ripard  2015-10-29  158  	.destroy		= sun4i_rgb_connector_destroy,
29e57fab9 Maxime Ripard  2015-10-29  159  	.reset			= drm_atomic_helper_connector_reset,
29e57fab9 Maxime Ripard  2015-10-29  160  	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
29e57fab9 Maxime Ripard  2015-10-29  161  	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
29e57fab9 Maxime Ripard  2015-10-29  162  };
29e57fab9 Maxime Ripard  2015-10-29  163  
29e57fab9 Maxime Ripard  2015-10-29  164  static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder)
29e57fab9 Maxime Ripard  2015-10-29  165  {
29e57fab9 Maxime Ripard  2015-10-29  166  	struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(encoder);
29e57fab9 Maxime Ripard  2015-10-29  167  
29e57fab9 Maxime Ripard  2015-10-29  168  	DRM_DEBUG_DRIVER("Enabling RGB output\n");
29e57fab9 Maxime Ripard  2015-10-29  169  
b413dde1a Maxime Ripard  2019-02-26  170  	if (rgb->panel) {
b413dde1a Maxime Ripard  2019-02-26  171  		drm_panel_prepare(rgb->panel);
b413dde1a Maxime Ripard  2019-02-26  172  		drm_panel_enable(rgb->panel);
29e57fab9 Maxime Ripard  2015-10-29  173  	}
45e88f994 Maxime Ripard  2017-10-17  174  }
29e57fab9 Maxime Ripard  2015-10-29  175  
29e57fab9 Maxime Ripard  2015-10-29  176  static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder)
29e57fab9 Maxime Ripard  2015-10-29  177  {
29e57fab9 Maxime Ripard  2015-10-29  178  	struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(encoder);
29e57fab9 Maxime Ripard  2015-10-29  179  
29e57fab9 Maxime Ripard  2015-10-29  180  	DRM_DEBUG_DRIVER("Disabling RGB output\n");
29e57fab9 Maxime Ripard  2015-10-29  181  
b413dde1a Maxime Ripard  2019-02-26  182  	if (rgb->panel) {
b413dde1a Maxime Ripard  2019-02-26  183  		drm_panel_disable(rgb->panel);
b413dde1a Maxime Ripard  2019-02-26  184  		drm_panel_unprepare(rgb->panel);
4b3095025 Jonathan Liu   2016-08-30  185  	}
45e88f994 Maxime Ripard  2017-10-17  186  }
29e57fab9 Maxime Ripard  2015-10-29  187  
29e57fab9 Maxime Ripard  2015-10-29  188  static struct drm_encoder_helper_funcs sun4i_rgb_enc_helper_funcs = {
29e57fab9 Maxime Ripard  2015-10-29  189  	.disable	= sun4i_rgb_encoder_disable,
29e57fab9 Maxime Ripard  2015-10-29  190  	.enable		= sun4i_rgb_encoder_enable,
cde8b7548 Giulio Benetti 2018-03-13 @191  	.mode_valid	= sun4i_rgb_mode_valid,
29e57fab9 Maxime Ripard  2015-10-29  192  };
29e57fab9 Maxime Ripard  2015-10-29  193  

:::::: The code at line 191 was first introduced by commit
:::::: cde8b7548272640437d2fa691ae211f940066f6b drm/sun4i: move rgb mode_valid from connector to encoder

:::::: TO: Giulio Benetti <giulio.benetti@micronovasrl.com>
:::::: CC: Maxime Ripard <maxime.ripard@bootlin.com>

---
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: 68304 bytes --]

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

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

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

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

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

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

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

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

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

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: 159 bytes --]

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