Re: [Intel-gfx] [PATCH 1/4] drm/i915/edp: Allow alternate fixed mode for eDP if available.
On Mon, May 08, 2017 at 11:54:15AM +0300, Jani Nikula wrote: > On Sat, 06 May 2017, Jim Bridewrote: > > 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.
On Sat, 06 May 2017, Jim Bridewrote: > 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.
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 ViviCc: 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