[PATCH] drm: atmel_hlcdc: Add support for get_timings

[PATCH] drm: atmel_hlcdc: Add support for get_timings

[David Dueck]

drm_panel supports querying timing ranges. If the supplied mode does
not work with the hlcdc we query the panel and try to find a suitable
mode.

Signed-off-by: David Dueck <davidcdueck@googlemail.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 118 +++++++++++++++++++----
 1 file changed, 98 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
index 9c45130..ea36c24 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
@@ -20,6 +20,7 @@
  */
 
 #include <linux/of_graph.h>
+#include <video/display_timing.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_panel.h>
@@ -78,6 +79,8 @@ drm_encoder_to_atmel_hlcdc_rgb_output(struct drm_encoder *encoder)
 struct atmel_hlcdc_panel {
 	struct atmel_hlcdc_rgb_output base;
 	struct drm_panel *panel;
+	struct display_timing *timings;
+	unsigned int num_timings;
 };
 
 static inline struct atmel_hlcdc_panel *
@@ -104,14 +107,6 @@ static void atmel_hlcdc_panel_encoder_disable(struct drm_encoder *encoder)
 	drm_panel_disable(panel->panel);
 }
 
-static bool
-atmel_hlcdc_panel_encoder_mode_fixup(struct drm_encoder *encoder,
-				     const struct drm_display_mode *mode,
-				     struct drm_display_mode *adjusted)
-{
-	return true;
-}
-
 static void
 atmel_hlcdc_rgb_encoder_mode_set(struct drm_encoder *encoder,
 				 struct drm_display_mode *mode,
@@ -142,8 +137,86 @@ atmel_hlcdc_rgb_encoder_mode_set(struct drm_encoder *encoder,
 			   cfg);
 }
 
+/**
+ * atmel_hlcdc_choose_parameter - choose timing parameter
+ * @dc_min: minimum parameter value allowed by dc
+ * @dc_max: maximum parameter value allowed by dc
+ * @te: parameter range allowed by panel
+ * @result: chosen parameter
+ *
+ * DESCRIPTION:
+ * If there is overlap in the allowed ranges, we will pick a parameter
+ * minimizing the distance to te.typ. If not, we return te.min or te.max.
+ **/
+static u32 atmel_hlcdc_choose_parameter(u32 dc_min, u32 dc_max,
+					struct timing_entry te)
+{
+	if (te.typ <= dc_max && te.typ >= dc_min)
+		return te.typ;
+	else if (te.typ > dc_max)
+		return max(dc_max, te.min);
+	else
+		return min(dc_min, te.max);
+}
+
+static void atmel_hlcdc_calculate_mode(struct display_timing *timings,
+				       struct drm_display_mode *adjusted_mode)
+{
+	u32 hsync_len, hfront_porch, hback_porch;
+	u32 vsync_len, vfront_porch, vback_porch;
+
+	hsync_len = atmel_hlcdc_choose_parameter(1, 0x40, timings->hsync_len);
+	vsync_len = atmel_hlcdc_choose_parameter(1, 0x40, timings->vsync_len);
+
+	hfront_porch = atmel_hlcdc_choose_parameter(1, 0x200,
+						    timings->hfront_porch);
+	hback_porch = atmel_hlcdc_choose_parameter(1, 0x200,
+						   timings->hback_porch);
+	vfront_porch = atmel_hlcdc_choose_parameter(1, 0x40,
+						    timings->vfront_porch);
+	vback_porch = atmel_hlcdc_choose_parameter(0, 0x40,
+						   timings->vback_porch);
+
+	adjusted_mode->hsync_start = adjusted_mode->hdisplay + hfront_porch;
+	adjusted_mode->hsync_end = adjusted_mode->hsync_start + hsync_len;
+	adjusted_mode->htotal = adjusted_mode->hsync_end + hback_porch;
+
+	adjusted_mode->vsync_start = adjusted_mode->vdisplay + vfront_porch;
+	adjusted_mode->vsync_end = adjusted_mode->vsync_start + vsync_len;
+	adjusted_mode->vtotal = adjusted_mode->vsync_end + vback_porch;
+}
+
+static int
+atmel_hlcdc_panel_encoder_atomic_check(struct drm_encoder *encoder,
+				       struct drm_crtc_state *crtc_state,
+				       struct drm_connector_state *conn_state)
+{
+	struct atmel_hlcdc_rgb_output *rgb =
+			drm_encoder_to_atmel_hlcdc_rgb_output(encoder);
+	struct atmel_hlcdc_panel *panel = atmel_hlcdc_rgb_output_to_panel(rgb);
+	struct drm_display_mode *mode = &crtc_state->mode;
+	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
+	int i;
+
+	if (atmel_hlcdc_dc_mode_valid(rgb->dc, mode) == MODE_OK)
+		return 0;
+
+	for (i = 0; i < panel->num_timings; i++) {
+		if (panel->timings->hactive.typ != mode->hdisplay ||
+		    panel->timings->vactive.typ != mode->vdisplay)
+			continue;
+
+		atmel_hlcdc_calculate_mode(panel->timings, adjusted_mode);
+		if (atmel_hlcdc_dc_mode_valid(rgb->dc, adjusted_mode) ==
+		    MODE_OK)
+			return 0;
+	}
+
+	return -EINVAL;
+}
+
 static struct drm_encoder_helper_funcs atmel_hlcdc_panel_encoder_helper_funcs = {
-	.mode_fixup = atmel_hlcdc_panel_encoder_mode_fixup,
+	.atomic_check = atmel_hlcdc_panel_encoder_atomic_check,
 	.mode_set = atmel_hlcdc_rgb_encoder_mode_set,
 	.disable = atmel_hlcdc_panel_encoder_disable,
 	.enable = atmel_hlcdc_panel_encoder_enable,
@@ -168,16 +241,6 @@ static int atmel_hlcdc_panel_get_modes(struct drm_connector *connector)
 	return panel->panel->funcs->get_modes(panel->panel);
 }
 
-static int atmel_hlcdc_rgb_mode_valid(struct drm_connector *connector,
-				      struct drm_display_mode *mode)
-{
-	struct atmel_hlcdc_rgb_output *rgb =
-			drm_connector_to_atmel_hlcdc_rgb_output(connector);
-
-	return atmel_hlcdc_dc_mode_valid(rgb->dc, mode);
-}
-
-
 
 static struct drm_encoder *
 atmel_hlcdc_rgb_best_encoder(struct drm_connector *connector)
@@ -190,7 +253,6 @@ atmel_hlcdc_rgb_best_encoder(struct drm_connector *connector)
 
 static struct drm_connector_helper_funcs atmel_hlcdc_panel_connector_helper_funcs = {
 	.get_modes = atmel_hlcdc_panel_get_modes,
-	.mode_valid = atmel_hlcdc_rgb_mode_valid,
 	.best_encoder = atmel_hlcdc_rgb_best_encoder,
 };
 
@@ -273,6 +335,22 @@ static int atmel_hlcdc_create_panel_output(struct drm_device *dev,
 	drm_panel_attach(p, &panel->base.connector);
 	panel->panel = p;
 
+	if (!panel->panel->funcs->get_timings)
+		return 0;
+
+	panel->num_timings =
+		panel->panel->funcs->get_timings(panel->panel, 0, NULL);
+	panel->timings =
+		devm_kzalloc(dev->dev,
+			     panel->num_timings * sizeof(struct display_timing),
+			     GFP_KERNEL);
+
+	if (!panel->timings)
+		return 0;
+
+	panel->panel->funcs->get_timings(panel->panel, panel->num_timings,
+					 panel->timings);
+
 	return 0;
 
 err_encoder_cleanup:
-- 
2.3.6

[PATCH] drm: atmel_hlcdc: Add support for get_timings

[Boris Brezillon]

Hi David,

On Thu, 21 May 2015 11:06:56 +0200
David Dueck <davidcdueck@googlemail.com> wrote:

> drm_panel supports querying timing ranges. If the supplied mode does
> not work with the hlcdc we query the panel and try to find a suitable
> mode.

This patch looks good to me.

Philip, Thierry, could you confirm this is the correct way of dealing
with timing ranges.

> 
> Signed-off-by: David Dueck <davidcdueck@googlemail.com>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 118 +++++++++++++++++++----
>  1 file changed, 98 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> index 9c45130..ea36c24 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> @@ -20,6 +20,7 @@
>   */
>  
>  #include <linux/of_graph.h>
> +#include <video/display_timing.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_panel.h>
> @@ -78,6 +79,8 @@ drm_encoder_to_atmel_hlcdc_rgb_output(struct drm_encoder *encoder)
>  struct atmel_hlcdc_panel {
>  	struct atmel_hlcdc_rgb_output base;
>  	struct drm_panel *panel;
> +	struct display_timing *timings;
> +	unsigned int num_timings;
>  };
>  
>  static inline struct atmel_hlcdc_panel *
> @@ -104,14 +107,6 @@ static void atmel_hlcdc_panel_encoder_disable(struct drm_encoder *encoder)
>  	drm_panel_disable(panel->panel);
>  }
>  
> -static bool
> -atmel_hlcdc_panel_encoder_mode_fixup(struct drm_encoder *encoder,
> -				     const struct drm_display_mode *mode,
> -				     struct drm_display_mode *adjusted)
> -{
> -	return true;
> -}
> -
>  static void
>  atmel_hlcdc_rgb_encoder_mode_set(struct drm_encoder *encoder,
>  				 struct drm_display_mode *mode,
> @@ -142,8 +137,86 @@ atmel_hlcdc_rgb_encoder_mode_set(struct drm_encoder *encoder,
>  			   cfg);
>  }
>  
> +/**
> + * atmel_hlcdc_choose_parameter - choose timing parameter
> + * @dc_min: minimum parameter value allowed by dc
> + * @dc_max: maximum parameter value allowed by dc
> + * @te: parameter range allowed by panel
> + * @result: chosen parameter
> + *
> + * DESCRIPTION:
> + * If there is overlap in the allowed ranges, we will pick a parameter
> + * minimizing the distance to te.typ. If not, we return te.min or te.max.
> + **/
> +static u32 atmel_hlcdc_choose_parameter(u32 dc_min, u32 dc_max,
> +					struct timing_entry te)
> +{
> +	if (te.typ <= dc_max && te.typ >= dc_min)
> +		return te.typ;
> +	else if (te.typ > dc_max)
> +		return max(dc_max, te.min);
> +	else
> +		return min(dc_min, te.max);
> +}
> +
> +static void atmel_hlcdc_calculate_mode(struct display_timing *timings,
> +				       struct drm_display_mode *adjusted_mode)
> +{
> +	u32 hsync_len, hfront_porch, hback_porch;
> +	u32 vsync_len, vfront_porch, vback_porch;
> +
> +	hsync_len = atmel_hlcdc_choose_parameter(1, 0x40, timings->hsync_len);
> +	vsync_len = atmel_hlcdc_choose_parameter(1, 0x40, timings->vsync_len);
> +
> +	hfront_porch = atmel_hlcdc_choose_parameter(1, 0x200,
> +						    timings->hfront_porch);
> +	hback_porch = atmel_hlcdc_choose_parameter(1, 0x200,
> +						   timings->hback_porch);
> +	vfront_porch = atmel_hlcdc_choose_parameter(1, 0x40,
> +						    timings->vfront_porch);
> +	vback_porch = atmel_hlcdc_choose_parameter(0, 0x40,
> +						   timings->vback_porch);
> +
> +	adjusted_mode->hsync_start = adjusted_mode->hdisplay + hfront_porch;
> +	adjusted_mode->hsync_end = adjusted_mode->hsync_start + hsync_len;
> +	adjusted_mode->htotal = adjusted_mode->hsync_end + hback_porch;
> +
> +	adjusted_mode->vsync_start = adjusted_mode->vdisplay + vfront_porch;
> +	adjusted_mode->vsync_end = adjusted_mode->vsync_start + vsync_len;
> +	adjusted_mode->vtotal = adjusted_mode->vsync_end + vback_porch;
> +}
> +
> +static int
> +atmel_hlcdc_panel_encoder_atomic_check(struct drm_encoder *encoder,
> +				       struct drm_crtc_state *crtc_state,
> +				       struct drm_connector_state *conn_state)
> +{
> +	struct atmel_hlcdc_rgb_output *rgb =
> +			drm_encoder_to_atmel_hlcdc_rgb_output(encoder);
> +	struct atmel_hlcdc_panel *panel = atmel_hlcdc_rgb_output_to_panel(rgb);
> +	struct drm_display_mode *mode = &crtc_state->mode;
> +	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> +	int i;
> +
> +	if (atmel_hlcdc_dc_mode_valid(rgb->dc, mode) == MODE_OK)
> +		return 0;
> +
> +	for (i = 0; i < panel->num_timings; i++) {
> +		if (panel->timings->hactive.typ != mode->hdisplay ||
> +		    panel->timings->vactive.typ != mode->vdisplay)
> +			continue;
> +
> +		atmel_hlcdc_calculate_mode(panel->timings, adjusted_mode);
> +		if (atmel_hlcdc_dc_mode_valid(rgb->dc, adjusted_mode) ==
> +		    MODE_OK)
> +			return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static struct drm_encoder_helper_funcs atmel_hlcdc_panel_encoder_helper_funcs = {
> -	.mode_fixup = atmel_hlcdc_panel_encoder_mode_fixup,
> +	.atomic_check = atmel_hlcdc_panel_encoder_atomic_check,
>  	.mode_set = atmel_hlcdc_rgb_encoder_mode_set,
>  	.disable = atmel_hlcdc_panel_encoder_disable,
>  	.enable = atmel_hlcdc_panel_encoder_enable,
> @@ -168,16 +241,6 @@ static int atmel_hlcdc_panel_get_modes(struct drm_connector *connector)
>  	return panel->panel->funcs->get_modes(panel->panel);
>  }
>  
> -static int atmel_hlcdc_rgb_mode_valid(struct drm_connector *connector,
> -				      struct drm_display_mode *mode)
> -{
> -	struct atmel_hlcdc_rgb_output *rgb =
> -			drm_connector_to_atmel_hlcdc_rgb_output(connector);
> -
> -	return atmel_hlcdc_dc_mode_valid(rgb->dc, mode);
> -}
> -
> -
>  
>  static struct drm_encoder *
>  atmel_hlcdc_rgb_best_encoder(struct drm_connector *connector)
> @@ -190,7 +253,6 @@ atmel_hlcdc_rgb_best_encoder(struct drm_connector *connector)
>  
>  static struct drm_connector_helper_funcs atmel_hlcdc_panel_connector_helper_funcs = {
>  	.get_modes = atmel_hlcdc_panel_get_modes,
> -	.mode_valid = atmel_hlcdc_rgb_mode_valid,
>  	.best_encoder = atmel_hlcdc_rgb_best_encoder,
>  };
>  
> @@ -273,6 +335,22 @@ static int atmel_hlcdc_create_panel_output(struct drm_device *dev,
>  	drm_panel_attach(p, &panel->base.connector);
>  	panel->panel = p;
>  
> +	if (!panel->panel->funcs->get_timings)
> +		return 0;
> +
> +	panel->num_timings =
> +		panel->panel->funcs->get_timings(panel->panel, 0, NULL);
> +	panel->timings =
> +		devm_kzalloc(dev->dev,
> +			     panel->num_timings * sizeof(struct display_timing),
> +			     GFP_KERNEL);
> +
> +	if (!panel->timings)
> +		return 0;
> +
> +	panel->panel->funcs->get_timings(panel->panel, panel->num_timings,
> +					 panel->timings);
> +
>  	return 0;
>  
>  err_encoder_cleanup:



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH] drm: atmel_hlcdc: Add support for get_timings

[Philipp Zabel]

Hi Boris,

Am Dienstag, den 26.05.2015, 11:28 +0200 schrieb Boris Brezillon:
> Hi David,
> 
> On Thu, 21 May 2015 11:06:56 +0200
> David Dueck <davidcdueck@googlemail.com> wrote:
> 
> > drm_panel supports querying timing ranges. If the supplied mode does
> > not work with the hlcdc we query the panel and try to find a suitable
> > mode.
> 
> This patch looks good to me.
> 
> Philip, Thierry, could you confirm this is the correct way of dealing
> with timing ranges.

I wonder about two things:

This implementation minimizes the sum of absolute differences between
chosen and typical values. I wonder if it would be better to try and
minimize the difference between the chosen and nominal vertical refresh
rate.

Is this something that should be done earlier, in create_panel_output,
so that the connector's modes already contain the corrected settings?

regards
Philipp

> > 
> > Signed-off-by: David Dueck <davidcdueck@googlemail.com>
> > ---
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 118 +++++++++++++++++++----
> >  1 file changed, 98 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> > index 9c45130..ea36c24 100644
> > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> > @@ -20,6 +20,7 @@
> >   */
> >  
> >  #include <linux/of_graph.h>
> > +#include <video/display_timing.h>
> >  
> >  #include <drm/drmP.h>
> >  #include <drm/drm_panel.h>
> > @@ -78,6 +79,8 @@ drm_encoder_to_atmel_hlcdc_rgb_output(struct drm_encoder *encoder)
> >  struct atmel_hlcdc_panel {
> >  	struct atmel_hlcdc_rgb_output base;
> >  	struct drm_panel *panel;
> > +	struct display_timing *timings;
> > +	unsigned int num_timings;
> >  };
> >  
> >  static inline struct atmel_hlcdc_panel *
> > @@ -104,14 +107,6 @@ static void atmel_hlcdc_panel_encoder_disable(struct drm_encoder *encoder)
> >  	drm_panel_disable(panel->panel);
> >  }
> >  
> > -static bool
> > -atmel_hlcdc_panel_encoder_mode_fixup(struct drm_encoder *encoder,
> > -				     const struct drm_display_mode *mode,
> > -				     struct drm_display_mode *adjusted)
> > -{
> > -	return true;
> > -}
> > -
> >  static void
> >  atmel_hlcdc_rgb_encoder_mode_set(struct drm_encoder *encoder,
> >  				 struct drm_display_mode *mode,
> > @@ -142,8 +137,86 @@ atmel_hlcdc_rgb_encoder_mode_set(struct drm_encoder *encoder,
> >  			   cfg);
> >  }
> >  
> > +/**
> > + * atmel_hlcdc_choose_parameter - choose timing parameter
> > + * @dc_min: minimum parameter value allowed by dc
> > + * @dc_max: maximum parameter value allowed by dc
> > + * @te: parameter range allowed by panel
> > + * @result: chosen parameter
> > + *
> > + * DESCRIPTION:
> > + * If there is overlap in the allowed ranges, we will pick a parameter
> > + * minimizing the distance to te.typ. If not, we return te.min or te.max.
> > + **/
> > +static u32 atmel_hlcdc_choose_parameter(u32 dc_min, u32 dc_max,
> > +					struct timing_entry te)
> > +{
> > +	if (te.typ <= dc_max && te.typ >= dc_min)
> > +		return te.typ;
> > +	else if (te.typ > dc_max)
> > +		return max(dc_max, te.min);
> > +	else
> > +		return min(dc_min, te.max);
> > +}
> > +
> > +static void atmel_hlcdc_calculate_mode(struct display_timing *timings,
> > +				       struct drm_display_mode *adjusted_mode)
> > +{
> > +	u32 hsync_len, hfront_porch, hback_porch;
> > +	u32 vsync_len, vfront_porch, vback_porch;
> > +
> > +	hsync_len = atmel_hlcdc_choose_parameter(1, 0x40, timings->hsync_len);
> > +	vsync_len = atmel_hlcdc_choose_parameter(1, 0x40, timings->vsync_len);
> > +
> > +	hfront_porch = atmel_hlcdc_choose_parameter(1, 0x200,
> > +						    timings->hfront_porch);
> > +	hback_porch = atmel_hlcdc_choose_parameter(1, 0x200,
> > +						   timings->hback_porch);
> > +	vfront_porch = atmel_hlcdc_choose_parameter(1, 0x40,
> > +						    timings->vfront_porch);
> > +	vback_porch = atmel_hlcdc_choose_parameter(0, 0x40,
> > +						   timings->vback_porch);
> > +
> > +	adjusted_mode->hsync_start = adjusted_mode->hdisplay + hfront_porch;
> > +	adjusted_mode->hsync_end = adjusted_mode->hsync_start + hsync_len;
> > +	adjusted_mode->htotal = adjusted_mode->hsync_end + hback_porch;
> > +
> > +	adjusted_mode->vsync_start = adjusted_mode->vdisplay + vfront_porch;
> > +	adjusted_mode->vsync_end = adjusted_mode->vsync_start + vsync_len;
> > +	adjusted_mode->vtotal = adjusted_mode->vsync_end + vback_porch;
> > +}
> > +
> > +static int
> > +atmel_hlcdc_panel_encoder_atomic_check(struct drm_encoder *encoder,
> > +				       struct drm_crtc_state *crtc_state,
> > +				       struct drm_connector_state *conn_state)
> > +{
> > +	struct atmel_hlcdc_rgb_output *rgb =
> > +			drm_encoder_to_atmel_hlcdc_rgb_output(encoder);
> > +	struct atmel_hlcdc_panel *panel = atmel_hlcdc_rgb_output_to_panel(rgb);
> > +	struct drm_display_mode *mode = &crtc_state->mode;
> > +	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> > +	int i;
> > +
> > +	if (atmel_hlcdc_dc_mode_valid(rgb->dc, mode) == MODE_OK)
> > +		return 0;
> > +
> > +	for (i = 0; i < panel->num_timings; i++) {
> > +		if (panel->timings->hactive.typ != mode->hdisplay ||
> > +		    panel->timings->vactive.typ != mode->vdisplay)
> > +			continue;
> > +
> > +		atmel_hlcdc_calculate_mode(panel->timings, adjusted_mode);
> > +		if (atmel_hlcdc_dc_mode_valid(rgb->dc, adjusted_mode) ==
> > +		    MODE_OK)
> > +			return 0;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> >  static struct drm_encoder_helper_funcs atmel_hlcdc_panel_encoder_helper_funcs = {
> > -	.mode_fixup = atmel_hlcdc_panel_encoder_mode_fixup,
> > +	.atomic_check = atmel_hlcdc_panel_encoder_atomic_check,
> >  	.mode_set = atmel_hlcdc_rgb_encoder_mode_set,
> >  	.disable = atmel_hlcdc_panel_encoder_disable,
> >  	.enable = atmel_hlcdc_panel_encoder_enable,
> > @@ -168,16 +241,6 @@ static int atmel_hlcdc_panel_get_modes(struct drm_connector *connector)
> >  	return panel->panel->funcs->get_modes(panel->panel);
> >  }
> >  
> > -static int atmel_hlcdc_rgb_mode_valid(struct drm_connector *connector,
> > -				      struct drm_display_mode *mode)
> > -{
> > -	struct atmel_hlcdc_rgb_output *rgb =
> > -			drm_connector_to_atmel_hlcdc_rgb_output(connector);
> > -
> > -	return atmel_hlcdc_dc_mode_valid(rgb->dc, mode);
> > -}
> > -
> > -
> >  
> >  static struct drm_encoder *
> >  atmel_hlcdc_rgb_best_encoder(struct drm_connector *connector)
> > @@ -190,7 +253,6 @@ atmel_hlcdc_rgb_best_encoder(struct drm_connector *connector)
> >  
> >  static struct drm_connector_helper_funcs atmel_hlcdc_panel_connector_helper_funcs = {
> >  	.get_modes = atmel_hlcdc_panel_get_modes,
> > -	.mode_valid = atmel_hlcdc_rgb_mode_valid,
> >  	.best_encoder = atmel_hlcdc_rgb_best_encoder,
> >  };
> >  
> > @@ -273,6 +335,22 @@ static int atmel_hlcdc_create_panel_output(struct drm_device *dev,
> >  	drm_panel_attach(p, &panel->base.connector);
> >  	panel->panel = p;
> >  
> > +	if (!panel->panel->funcs->get_timings)
> > +		return 0;
> > +
> > +	panel->num_timings =
> > +		panel->panel->funcs->get_timings(panel->panel, 0, NULL);
> > +	panel->timings =
> > +		devm_kzalloc(dev->dev,
> > +			     panel->num_timings * sizeof(struct display_timing),
> > +			     GFP_KERNEL);
> > +
> > +	if (!panel->timings)
> > +		return 0;
> > +
> > +	panel->panel->funcs->get_timings(panel->panel, panel->num_timings,
> > +					 panel->timings);
> > +
> >  	return 0;
> >  
> >  err_encoder_cleanup:
> 
> 
> 

[PATCH] drm: atmel_hlcdc: Add support for get_timings

[Boris Brezillon]

Hi Philip,

On Thu, 28 May 2015 13:13:28 +0200
Philipp Zabel <p.zabel@pengutronix.de> wrote:

> Hi Boris,
> 
> Am Dienstag, den 26.05.2015, 11:28 +0200 schrieb Boris Brezillon:
> > Hi David,
> > 
> > On Thu, 21 May 2015 11:06:56 +0200
> > David Dueck <davidcdueck@googlemail.com> wrote:
> > 
> > > drm_panel supports querying timing ranges. If the supplied mode does
> > > not work with the hlcdc we query the panel and try to find a suitable
> > > mode.
> > 
> > This patch looks good to me.
> > 
> > Philip, Thierry, could you confirm this is the correct way of dealing
> > with timing ranges.
> 
> I wonder about two things:
> 
> This implementation minimizes the sum of absolute differences between
> chosen and typical values. I wonder if it would be better to try and
> minimize the difference between the chosen and nominal vertical refresh
> rate.

I'm not sure to understand what you mean.
Are you suggesting that we should try keeping the vtotal (and maybe the
htotal too) value unchanged by adapting the timing values ?
Something like that:

	vfront_porch = atmel_hlcdc_choose_parameter(1, 0x40,
						    timings->vfront_porch);
	
	adjusted_mode->vsync_start = adjusted_mode->vdisplay + vfront_porch;

	vsync_len = atmel_hlcdc_choose_parameter(1, 0x40,
						 adjusted_mode->vsync_end -
						 adjusted_mode->vsync_start);
	adjusted_mode->vsync_end = adjusted_mode->vsync_start + vsync_len;
	vback_porch = atmel_hlcdc_choose_parameter(0, 0x40,
						   timings->vtotal -
						   adjusted_mode->vsync_end);
	adjusted_mode->vtotal = adjusted_mode->vsync_end + vback_porch;
	/* ... */

If that's the case, then I definitely agree.

> 
> Is this something that should be done earlier, in create_panel_output,
> so that the connector's modes already contain the corrected settings?

You mean creating our own drm_display modes (and taking the timing
constraints when creating those modes) instead of calling drm_panel's
->get_modes().
That sounds reasonable. David, what's your opinion.

Best Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH] drm: atmel_hlcdc: Add support for get_timings

[Philipp Zabel]

Am Donnerstag, den 28.05.2015, 14:45 +0200 schrieb Boris Brezillon:
> Hi Philip,
> 
> On Thu, 28 May 2015 13:13:28 +0200
> Philipp Zabel <p.zabel@pengutronix.de> wrote:
> 
> > Hi Boris,
> > 
> > Am Dienstag, den 26.05.2015, 11:28 +0200 schrieb Boris Brezillon:
> > > Hi David,
> > > 
> > > On Thu, 21 May 2015 11:06:56 +0200
> > > David Dueck <davidcdueck@googlemail.com> wrote:
> > > 
> > > > drm_panel supports querying timing ranges. If the supplied mode does
> > > > not work with the hlcdc we query the panel and try to find a suitable
> > > > mode.
> > > 
> > > This patch looks good to me.
> > > 
> > > Philip, Thierry, could you confirm this is the correct way of dealing
> > > with timing ranges.
> > 
> > I wonder about two things:
> > 
> > This implementation minimizes the sum of absolute differences between
> > chosen and typical values. I wonder if it would be better to try and
> > minimize the difference between the chosen and nominal vertical refresh
> > rate.
> 
> I'm not sure to understand what you mean.
> Are you suggesting that we should try keeping the vtotal (and maybe the
> htotal too) value unchanged by adapting the timing values ?

More or less, only that I'd first modify htotal and vtotal to get closer
to the ideal frametime (1/vrefresh) if the pixel clock can't be set
exactly to the panel's typical pixel clock rate.

> Something like that:
> 
> 	vfront_porch = atmel_hlcdc_choose_parameter(1, 0x40,
> 						    timings->vfront_porch);
> 	
> 	adjusted_mode->vsync_start = adjusted_mode->vdisplay + vfront_porch;
> 
> 	vsync_len = atmel_hlcdc_choose_parameter(1, 0x40,
> 						 adjusted_mode->vsync_end -
> 						 adjusted_mode->vsync_start);
> 	adjusted_mode->vsync_end = adjusted_mode->vsync_start + vsync_len;
> 	vback_porch = atmel_hlcdc_choose_parameter(0, 0x40,
> 						   timings->vtotal -
> 						   adjusted_mode->vsync_end);
> 	adjusted_mode->vtotal = adjusted_mode->vsync_end + vback_porch;
> 	/* ... */
> 
> If that's the case, then I definitely agree.
> 
> > 
> > Is this something that should be done earlier, in create_panel_output,
> > so that the connector's modes already contain the corrected settings?
> 
> You mean creating our own drm_display modes (and taking the timing
> constraints when creating those modes) instead of calling drm_panel's
> ->get_modes().

Yes.

regards
Philipp

[PATCH] drm: atmel_hlcdc: Add support for get_timings

[Boris Brezillon]

On Thu, 28 May 2015 15:51:44 +0200
Philipp Zabel <p.zabel@pengutronix.de> wrote:

> Am Donnerstag, den 28.05.2015, 14:45 +0200 schrieb Boris Brezillon:
> > Hi Philip,
> > 
> > On Thu, 28 May 2015 13:13:28 +0200
> > Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > 
> > > Hi Boris,
> > > 
> > > Am Dienstag, den 26.05.2015, 11:28 +0200 schrieb Boris Brezillon:
> > > > Hi David,
> > > > 
> > > > On Thu, 21 May 2015 11:06:56 +0200
> > > > David Dueck <davidcdueck@googlemail.com> wrote:
> > > > 
> > > > > drm_panel supports querying timing ranges. If the supplied mode does
> > > > > not work with the hlcdc we query the panel and try to find a suitable
> > > > > mode.
> > > > 
> > > > This patch looks good to me.
> > > > 
> > > > Philip, Thierry, could you confirm this is the correct way of dealing
> > > > with timing ranges.
> > > 
> > > I wonder about two things:
> > > 
> > > This implementation minimizes the sum of absolute differences between
> > > chosen and typical values. I wonder if it would be better to try and
> > > minimize the difference between the chosen and nominal vertical refresh
> > > rate.
> > 
> > I'm not sure to understand what you mean.
> > Are you suggesting that we should try keeping the vtotal (and maybe the
> > htotal too) value unchanged by adapting the timing values ?
> 
> More or less, only that I'd first modify htotal and vtotal to get closer
> to the ideal frametime (1/vrefresh) if the pixel clock can't be set
> exactly to the panel's typical pixel clock rate.

Okay, now I get it (I was just keeping the pixel clk rate out of the
equation).
That sounds reasonable.

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com