Re: [Intel-gfx] [PATCH 1/4] drm/i915/edp: Allow alternate fixed mode for eDP if available.

2017-05-08 Thread Jim Bride 
On Mon, May 08, 2017 at 11:54:15AM +0300, Jani Nikula wrote:
> On Sat, 06 May 2017, Jim Bride  wrote:
> > Some fixed resolution panels actually support more than one mode,
> > with the only thing different being the refresh rate.  Having this
> > alternate mode available to us is desirable, because it allows us to
> > test PSR on panels whose setup time at the preferred mode is too long.
> > With this patch we allow the use of the alternate mode if it's
> > available and it was specifically requested.
> 
> All in all this feels like a hack. The generic solution would be to
> allow any mode to be set again.

To an extent, I agree with you.  I did things this way in an attempt
to change the current behavior as little as possible.  Personally,
I'd rather see us allow any supported mode to be set.

> A few specific comments inline.
> 
> BR,
> Jani.
> 
> > Cc: Rodrigo Vivi 
> > Cc: Paulo Zanoni 
> > Signed-off-by: Jim Bride 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c| 34 +-
> >  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
> >  drivers/gpu/drm/i915/intel_dsi.c   |  2 +-
> >  drivers/gpu/drm/i915/intel_dvo.c   |  2 +-
> >  drivers/gpu/drm/i915/intel_lvds.c  |  3 ++-
> >  drivers/gpu/drm/i915/intel_panel.c |  2 ++
> >  6 files changed, 37 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 08834f7..d46f72d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1637,6 +1637,19 @@ static int intel_dp_compute_bpp(struct intel_dp 
> > *intel_dp,
> > return bpp;
> >  }
> >  
> > +static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
> > +  struct drm_display_mode *m2)
> > +{
> > +   return (m1->hdisplay == m2->hdisplay &&
> > +   m1->hsync_start == m2->hsync_start &&
> > +   m1->hsync_end == m2->hsync_end &&
> > +   m1->htotal == m2->htotal &&
> > +   m1->vdisplay == m2->vdisplay &&
> > +   m1->vsync_start == m2->vsync_start &&
> > +   m1->vsync_end == m2->vsync_end &&
> > +   m1->vtotal == m2->vtotal);
> > +}
> 
> See drm_mode_equal and friends.

I did.  The problem is that I needed a comparison without vscan being
involved (see drm_mode_equal_no_clocks_no_stereo(), where the actual
comparison happens.)  This seemed like the least disruptive way to do
that comparison.

> 
> > +
> >  bool
> >  intel_dp_compute_config(struct intel_encoder *encoder,
> > struct intel_crtc_state *pipe_config,
> > @@ -1674,8 +1687,16 @@ intel_dp_compute_config(struct intel_encoder 
> > *encoder,
> > pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
> >  
> > if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> > -   intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
> > -  adjusted_mode);
> > +   struct drm_display_mode *panel_mode =
> > +   intel_connector->panel.alt_fixed_mode;
> > +   struct drm_display_mode *req_mode = _config->base.mode;
> > +
> > +   if (!intel_edp_compare_alt_mode(req_mode, panel_mode))
> > +   panel_mode = intel_connector->panel.fixed_mode;
> > +
> > +   drm_mode_debug_printmodeline(panel_mode);
> > +
> > +   intel_fixed_panel_mode(panel_mode, adjusted_mode);
> >  
> > if (INTEL_GEN(dev_priv) >= 9) {
> > int ret;
> > @@ -5829,6 +5850,7 @@ static bool intel_edp_init_connector(struct intel_dp 
> > *intel_dp,
> > struct drm_device *dev = intel_encoder->base.dev;
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > struct drm_display_mode *fixed_mode = NULL;
> > +   struct drm_display_mode *alt_fixed_mode = NULL;
> > struct drm_display_mode *downclock_mode = NULL;
> > bool has_dpcd;
> > struct drm_display_mode *scan;
> > @@ -5884,13 +5906,14 @@ static bool intel_edp_init_connector(struct 
> > intel_dp *intel_dp,
> > }
> > intel_connector->edid = edid;
> >  
> > -   /* prefer fixed mode from EDID if available */
> > +   /* prefer fixed mode from EDID if available, save an alt mode also */
> > list_for_each_entry(scan, >probed_modes, head) {
> > if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
> > fixed_mode = drm_mode_duplicate(dev, scan);
> > downclock_mode = intel_dp_drrs_init(
> > intel_connector, fixed_mode);
> > -   break;
> > +   } else {
> > +   alt_fixed_mode = drm_mode_duplicate(dev, scan);
> > }
> 
> This changes the fixed mode if there are more than one preferred
> mode. This will also leak all but the two modes.

I wasn't aware there can be more than one

Re: [Intel-gfx] [PATCH 1/4] drm/i915/edp: Allow alternate fixed mode for eDP if available.

2017-05-08 Thread Jani Nikula 
On Sat, 06 May 2017, Jim Bride  wrote:
> Some fixed resolution panels actually support more than one mode,
> with the only thing different being the refresh rate.  Having this
> alternate mode available to us is desirable, because it allows us to
> test PSR on panels whose setup time at the preferred mode is too long.
> With this patch we allow the use of the alternate mode if it's
> available and it was specifically requested.

All in all this feels like a hack. The generic solution would be to
allow any mode to be set again.

A few specific comments inline.

BR,
Jani.

> Cc: Rodrigo Vivi 
> Cc: Paulo Zanoni 
> Signed-off-by: Jim Bride 
> ---
>  drivers/gpu/drm/i915/intel_dp.c| 34 +-
>  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
>  drivers/gpu/drm/i915/intel_dsi.c   |  2 +-
>  drivers/gpu/drm/i915/intel_dvo.c   |  2 +-
>  drivers/gpu/drm/i915/intel_lvds.c  |  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c |  2 ++
>  6 files changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 08834f7..d46f72d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1637,6 +1637,19 @@ static int intel_dp_compute_bpp(struct intel_dp 
> *intel_dp,
>   return bpp;
>  }
>  
> +static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
> +struct drm_display_mode *m2)
> +{
> + return (m1->hdisplay == m2->hdisplay &&
> + m1->hsync_start == m2->hsync_start &&
> + m1->hsync_end == m2->hsync_end &&
> + m1->htotal == m2->htotal &&
> + m1->vdisplay == m2->vdisplay &&
> + m1->vsync_start == m2->vsync_start &&
> + m1->vsync_end == m2->vsync_end &&
> + m1->vtotal == m2->vtotal);
> +}

See drm_mode_equal and friends.

> +
>  bool
>  intel_dp_compute_config(struct intel_encoder *encoder,
>   struct intel_crtc_state *pipe_config,
> @@ -1674,8 +1687,16 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>   pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
>  
>   if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> - intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
> -adjusted_mode);
> + struct drm_display_mode *panel_mode =
> + intel_connector->panel.alt_fixed_mode;
> + struct drm_display_mode *req_mode = _config->base.mode;
> +
> + if (!intel_edp_compare_alt_mode(req_mode, panel_mode))
> + panel_mode = intel_connector->panel.fixed_mode;
> +
> + drm_mode_debug_printmodeline(panel_mode);
> +
> + intel_fixed_panel_mode(panel_mode, adjusted_mode);
>  
>   if (INTEL_GEN(dev_priv) >= 9) {
>   int ret;
> @@ -5829,6 +5850,7 @@ static bool intel_edp_init_connector(struct intel_dp 
> *intel_dp,
>   struct drm_device *dev = intel_encoder->base.dev;
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   struct drm_display_mode *fixed_mode = NULL;
> + struct drm_display_mode *alt_fixed_mode = NULL;
>   struct drm_display_mode *downclock_mode = NULL;
>   bool has_dpcd;
>   struct drm_display_mode *scan;
> @@ -5884,13 +5906,14 @@ static bool intel_edp_init_connector(struct intel_dp 
> *intel_dp,
>   }
>   intel_connector->edid = edid;
>  
> - /* prefer fixed mode from EDID if available */
> + /* prefer fixed mode from EDID if available, save an alt mode also */
>   list_for_each_entry(scan, >probed_modes, head) {
>   if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
>   fixed_mode = drm_mode_duplicate(dev, scan);
>   downclock_mode = intel_dp_drrs_init(
>   intel_connector, fixed_mode);
> - break;
> + } else {
> + alt_fixed_mode = drm_mode_duplicate(dev, scan);
>   }

This changes the fixed mode if there are more than one preferred
mode. This will also leak all but the two modes.

>   }
>  
> @@ -5927,7 +5950,8 @@ static bool intel_edp_init_connector(struct intel_dp 
> *intel_dp,
> pipe_name(pipe));
>   }
>  
> - intel_panel_init(_connector->panel, fixed_mode, downclock_mode);
> + intel_panel_init(_connector->panel, fixed_mode, alt_fixed_mode,
> +  downclock_mode);
>   intel_connector->panel.backlight.power = intel_edp_backlight_power;
>   intel_panel_setup_backlight(connector, pipe);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 54f3ff8..19d0c8f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++

[Intel-gfx] [PATCH 1/4] drm/i915/edp: Allow alternate fixed mode for eDP if available.

2017-05-05 Thread Jim Bride 
Some fixed resolution panels actually support more than one mode,
with the only thing different being the refresh rate.  Having this
alternate mode available to us is desirable, because it allows us to
test PSR on panels whose setup time at the preferred mode is too long.
With this patch we allow the use of the alternate mode if it's
available and it was specifically requested.

Cc: Rodrigo Vivi 
Cc: Paulo Zanoni 
Signed-off-by: Jim Bride 
---
 drivers/gpu/drm/i915/intel_dp.c| 34 +-
 drivers/gpu/drm/i915/intel_drv.h   |  2 ++
 drivers/gpu/drm/i915/intel_dsi.c   |  2 +-
 drivers/gpu/drm/i915/intel_dvo.c   |  2 +-
 drivers/gpu/drm/i915/intel_lvds.c  |  3 ++-
 drivers/gpu/drm/i915/intel_panel.c |  2 ++
 6 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 08834f7..d46f72d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1637,6 +1637,19 @@ static int intel_dp_compute_bpp(struct intel_dp 
*intel_dp,
return bpp;
 }
 
+static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
+  struct drm_display_mode *m2)
+{
+   return (m1->hdisplay == m2->hdisplay &&
+   m1->hsync_start == m2->hsync_start &&
+   m1->hsync_end == m2->hsync_end &&
+   m1->htotal == m2->htotal &&
+   m1->vdisplay == m2->vdisplay &&
+   m1->vsync_start == m2->vsync_start &&
+   m1->vsync_end == m2->vsync_end &&
+   m1->vtotal == m2->vtotal);
+}
+
 bool
 intel_dp_compute_config(struct intel_encoder *encoder,
struct intel_crtc_state *pipe_config,
@@ -1674,8 +1687,16 @@ intel_dp_compute_config(struct intel_encoder *encoder,
pipe_config->has_audio = intel_dp->has_audio && port != PORT_A;
 
if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
-   intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
-  adjusted_mode);
+   struct drm_display_mode *panel_mode =
+   intel_connector->panel.alt_fixed_mode;
+   struct drm_display_mode *req_mode = _config->base.mode;
+
+   if (!intel_edp_compare_alt_mode(req_mode, panel_mode))
+   panel_mode = intel_connector->panel.fixed_mode;
+
+   drm_mode_debug_printmodeline(panel_mode);
+
+   intel_fixed_panel_mode(panel_mode, adjusted_mode);
 
if (INTEL_GEN(dev_priv) >= 9) {
int ret;
@@ -5829,6 +5850,7 @@ static bool intel_edp_init_connector(struct intel_dp 
*intel_dp,
struct drm_device *dev = intel_encoder->base.dev;
struct drm_i915_private *dev_priv = to_i915(dev);
struct drm_display_mode *fixed_mode = NULL;
+   struct drm_display_mode *alt_fixed_mode = NULL;
struct drm_display_mode *downclock_mode = NULL;
bool has_dpcd;
struct drm_display_mode *scan;
@@ -5884,13 +5906,14 @@ static bool intel_edp_init_connector(struct intel_dp 
*intel_dp,
}
intel_connector->edid = edid;
 
-   /* prefer fixed mode from EDID if available */
+   /* prefer fixed mode from EDID if available, save an alt mode also */
list_for_each_entry(scan, >probed_modes, head) {
if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
fixed_mode = drm_mode_duplicate(dev, scan);
downclock_mode = intel_dp_drrs_init(
intel_connector, fixed_mode);
-   break;
+   } else {
+   alt_fixed_mode = drm_mode_duplicate(dev, scan);
}
}
 
@@ -5927,7 +5950,8 @@ static bool intel_edp_init_connector(struct intel_dp 
*intel_dp,
  pipe_name(pipe));
}
 
-   intel_panel_init(_connector->panel, fixed_mode, downclock_mode);
+   intel_panel_init(_connector->panel, fixed_mode, alt_fixed_mode,
+downclock_mode);
intel_connector->panel.backlight.power = intel_edp_backlight_power;
intel_panel_setup_backlight(connector, pipe);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 54f3ff8..19d0c8f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -265,6 +265,7 @@ struct intel_encoder {
 
 struct intel_panel {
struct drm_display_mode *fixed_mode;
+   struct drm_display_mode *alt_fixed_mode;
struct drm_display_mode *downclock_mode;
int fitting_mode;
 
@@ -1676,6 +1677,7 @@ void intel_overlay_reset(struct drm_i915_private 
*dev_priv);
 /* intel_panel.c */
 int intel_panel_init(struct intel_panel *panel,
 struct