On Mon, May 08, 2017 at 11:54:15AM +0300, Jani Nikula wrote:
> On Sat, 06 May 2017, Jim Bride <jim.bride@xxxxxxxxxxxxxxx> 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 <rodrigo.vivi@xxxxxxxxx>
> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > Signed-off-by: Jim Bride <jim.bride@xxxxxxxxxxxxxxx>
> > ---
> > 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 = &pipe_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, &connector->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 preferred mode. I thought
the whole idea was to have a way of designating one mode of many
that would provide (at least in the opinion of the panel's makers)
the mode that will provide the best user experience. FWIW, on the
panels I've tested with I haven't seen more than two modes supported
natively in any case. If we follow your earlier suggestion about
just letting mode sets happen on all supported modes then all of this
stuff goes away.
Jim
>
> > }
> >
> > @@ -5927,7 +5950,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> > pipe_name(pipe));
> > }
> >
> > - intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> > + intel_panel_init(&intel_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 drm_display_mode *fixed_mode,
> > + struct drm_display_mode *alt_fixed_mode,
> > struct drm_display_mode *downclock_mode);
> > void intel_panel_fini(struct intel_panel *panel);
> > void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index fc0ef49..a938f08 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -1852,7 +1852,7 @@ void intel_dsi_init(struct drm_i915_private *dev_priv)
> > connector->display_info.width_mm = fixed_mode->width_mm;
> > connector->display_info.height_mm = fixed_mode->height_mm;
> >
> > - intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
> > + intel_panel_init(&intel_connector->panel, fixed_mode, NULL, NULL);
> > intel_panel_setup_backlight(connector, INVALID_PIPE);
> >
> > intel_dsi_add_properties(intel_connector);
> > diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> > index c1544a5..39fd4f3 100644
> > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > @@ -554,7 +554,7 @@ void intel_dvo_init(struct drm_i915_private *dev_priv)
> > */
> > intel_panel_init(&intel_connector->panel,
> > intel_dvo_get_current_mode(connector),
> > - NULL);
> > + NULL, NULL);
> > intel_dvo->panel_wants_dither = true;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 8b942ef..b1e6743 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -1181,7 +1181,8 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
> > out:
> > mutex_unlock(&dev->mode_config.mutex);
> >
> > - intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> > + intel_panel_init(&intel_connector->panel, fixed_mode, NULL,
> > + downclock_mode);
> > intel_panel_setup_backlight(connector, INVALID_PIPE);
> >
> > lvds_encoder->is_dual_link = compute_is_dual_link_lvds(lvds_encoder);
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index cb50c52..41eb7b0 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -1801,11 +1801,13 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
> >
> > int intel_panel_init(struct intel_panel *panel,
> > struct drm_display_mode *fixed_mode,
> > + struct drm_display_mode *alt_fixed_mode,
> > struct drm_display_mode *downclock_mode)
> > {
> > intel_panel_init_backlight_funcs(panel);
> >
> > panel->fixed_mode = fixed_mode;
> > + panel->alt_fixed_mode = alt_fixed_mode;
> > panel->downclock_mode = downclock_mode;
> >
> > return 0;
>
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
