* Re: [PATCH] Revert "drm/i915/edp: Allow alternate fixed mode for eDP if available."
@ 2018-05-17 7:33 ` Jani Nikula
0 siblings, 0 replies; 14+ messages in thread
From: Jani Nikula @ 2018-05-17 7:33 UTC (permalink / raw)
To: dhinakaran.pandiyan, intel-gfx; +Cc: Paulo Zanoni, # v4 . 14+, Rodrigo Vivi
On Thu, 17 May 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
>> On Wed, 2018-05-16 at 11:01 +0300, Jani Nikula wrote:
>>> This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e.
>>>
>>> Per the report, no matter what display mode you select with xrandr,
>>> the
>>> i915 driver will always select the alternate fixed mode. For the
>>> reporter this means that the display will always run at 40Hz which is
>>> quite annoying. This may be due to the mode comparison.
>>>
>>> But there are some other potential issues. The choice of
>>> alt_fixed_mode
>>> seems dubious. It's the first non-preferred mode, but there are no
>>> guarantees that the only difference would be refresh rate. Similarly,
>>> there may be more than one preferred mode in the probed modes list,
>>> and
>>> the commit changes the preferred mode selection to choose the last
>>> one
>>> on the list instead of the first.
>>>
>>> (Note that the probed modes list is the raw, unfiltered, unsorted
>>> list
>>> of modes from drm_add_edid_modes(), not the pretty result after a
>>> drm_helper_probe_single_connector_modes() call.)
>>>
>>> Finally, we already have eerily similar code in place to find the
>>> downclock mode for DRRS that seems like could be reused here.
>>>
>>> Back to the drawing board.
>>>
>>> Note: This is a hand-crafted revert due to conflicts. If it fails to
>>> backport, please just try reverting the original commit directly.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469
>>> Reported-by: Rune Petersen <rune@megahurts.dk>
>>> Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net>
>>> Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode for
>>> eDP if available.")
>>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
>>> Cc: David Weinehall <david.weinehall@linux.intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Jim Bride <jim.bride@linux.intel.com>
>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: intel-gfx@lists.freedesktop.org
>>> Cc: <stable@vger.kernel.org> # v4.14+
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_dp.c | 38 +++++-----------------------
>>> ----------
>>> 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 | 6 ------
>>> 6 files changed, 8 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index dde92e4af5d3..8320f0e8e3be 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -1679,23 +1679,6 @@ 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)
>>> -{
>>> - bool bres = false;
>>> -
>>> - if (m1 && m2)
>>> - bres = (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);
>>> - return bres;
>>> -}
>>> -
>>> /* Adjust link config limits based on compliance test requests. */
>>> static void
>>> intel_dp_adjust_compliance_config(struct intel_dp *intel_dp,
>>> @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct intel_encoder
>>> *encoder,
>>> pipe_config->has_audio = intel_conn_state-
>>> >force_audio == HDMI_AUDIO_ON;
>>>
>>> if (intel_dp_is_edp(intel_dp) && intel_connector-
>>> >panel.fixed_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);
>>> + intel_fixed_panel_mode(intel_connector-
>>> >panel.fixed_mode,
>>> + adjusted_mode);
>>>
>>> if (INTEL_GEN(dev_priv) >= 9) {
>>> int ret;
>>> @@ -6159,7 +6134,6 @@ static bool intel_edp_init_connector(struct
>>> intel_dp *intel_dp,
>>> struct drm_i915_private *dev_priv = to_i915(dev);
>>> struct drm_connector *connector = &intel_connector->base;
>>> 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;
>>> @@ -6214,14 +6188,13 @@ static bool intel_edp_init_connector(struct
>>> intel_dp *intel_dp,
>>> }
>>> intel_connector->edid = edid;
>>>
>>> - /* prefer fixed mode from EDID if available, save an alt
>>> mode also */
>>> + /* prefer fixed mode from EDID if available */
>>> 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);
>>> - } else if (!alt_fixed_mode) {
>>> - alt_fixed_mode = drm_mode_duplicate(dev,
>>> scan);
>>> + break;
>>
>> If multiple preferred modes are present, we'll now end up calling
>> drrs_init() only for the first. I see that this is what the original
>> code did but this revert does more than removing support for alternate
>> modes.
>
> It boils down to which preferred mode is *the* preferred mode. I think
> the original code was, uh, preferrable. Note that drrs init scans the
> entire list of modes again to find the same size mode with the lowest
> refresh rate.
Moreover, as you can see, the original alt mode commit had more subtle
changes than catches the eye, it caused regressions, and I feel pretty
strongly about getting back to the drawing board and starting over with
a clean slate than trying to tweak it when we are quite frankly way
overdue with the revert. If after that you think the drrs/downclock
selection needs tweaking, let's do that.
BR,
Jani.
>
> BR,
> Jani.
>
>>
>>> }
>>> }
>>>
>>> @@ -6258,8 +6231,7 @@ static bool intel_edp_init_connector(struct
>>> intel_dp *intel_dp,
>>> pipe_name(pipe));
>>> }
>>>
>>> - intel_panel_init(&intel_connector->panel, fixed_mode,
>>> alt_fixed_mode,
>>> - downclock_mode);
>>> + intel_panel_init(&intel_connector->panel, 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 d7dbca1aabff..0361130500a6 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -277,7 +277,6 @@ 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;
>>>
>>> /* backlight */
>>> @@ -1850,7 +1849,6 @@ 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 51a1d6868b1e..cf39ca90d887 100644
>>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>>> @@ -1846,7 +1846,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,
>>> NULL);
>>> + intel_panel_init(&intel_connector->panel, fixed_mode, 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 eb0c559b2715..a70d767313aa 100644
>>> --- a/drivers/gpu/drm/i915/intel_dvo.c
>>> +++ b/drivers/gpu/drm/i915/intel_dvo.c
>>> @@ -536,7 +536,7 @@ void intel_dvo_init(struct drm_i915_private
>>> *dev_priv)
>>> */
>>> intel_panel_init(&intel_connector->panel,
>>> intel_dvo_get_current_mode(
>>> intel_encoder),
>>> - 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 8691c86f579c..d8ece907ff54 100644
>>> --- a/drivers/gpu/drm/i915/intel_lvds.c
>>> +++ b/drivers/gpu/drm/i915/intel_lvds.c
>>> @@ -1140,8 +1140,7 @@ 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, NULL,
>>> - downclock_mode);
>>> + intel_panel_init(&intel_connector->panel, fixed_mode,
>>> 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 41d00b1603e3..b443278e569c 100644
>>> --- a/drivers/gpu/drm/i915/intel_panel.c
>>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>>> @@ -1928,13 +1928,11 @@ 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;
>>> @@ -1948,10 +1946,6 @@ void intel_panel_fini(struct intel_panel
>>> *panel)
>>> if (panel->fixed_mode)
>>> drm_mode_destroy(intel_connector->base.dev, panel-
>>> >fixed_mode);
>>>
>>> - if (panel->alt_fixed_mode)
>>> - drm_mode_destroy(intel_connector->base.dev,
>>> - panel->alt_fixed_mode);
>>> -
>>> if (panel->downclock_mode)
>>> drm_mode_destroy(intel_connector->base.dev,
>>> panel->downclock_mode);
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Intel-gfx] [PATCH] Revert "drm/i915/edp: Allow alternate fixed mode for eDP if available."
2018-05-17 7:33 ` Jani Nikula
(?)
@ 2018-05-17 19:44 ` Dhinakaran Pandiyan
2018-05-18 19:53 ` Dhinakaran Pandiyan
-1 siblings, 1 reply; 14+ messages in thread
From: Dhinakaran Pandiyan @ 2018-05-17 19:44 UTC (permalink / raw)
To: Jani Nikula, intel-gfx; +Cc: Paulo Zanoni, # v4 . 14+, Rodrigo Vivi
On Thu, 2018-05-17 at 10:33 +0300, Jani Nikula wrote:
> On Thu, 17 May 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> >
> > On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel
> > .com> wrote:
> > >
> > > On Wed, 2018-05-16 at 11:01 +0300, Jani Nikula wrote:
> > > >
> > > > This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e.
> > > >
> > > > Per the report, no matter what display mode you select with
> > > > xrandr,
> > > > the
> > > > i915 driver will always select the alternate fixed mode. For
> > > > the
> > > > reporter this means that the display will always run at 40Hz
> > > > which is
> > > > quite annoying. This may be due to the mode comparison.
> > > >
> > > > But there are some other potential issues. The choice of
> > > > alt_fixed_mode
> > > > seems dubious. It's the first non-preferred mode, but there are
> > > > no
> > > > guarantees that the only difference would be refresh rate.
> > > > Similarly,
> > > > there may be more than one preferred mode in the probed modes
> > > > list,
> > > > and
> > > > the commit changes the preferred mode selection to choose the
> > > > last
> > > > one
> > > > on the list instead of the first.
> > > >
> > > > (Note that the probed modes list is the raw, unfiltered,
> > > > unsorted
> > > > list
> > > > of modes from drm_add_edid_modes(), not the pretty result after
> > > > a
> > > > drm_helper_probe_single_connector_modes() call.)
> > > >
> > > > Finally, we already have eerily similar code in place to find
> > > > the
> > > > downclock mode for DRRS that seems like could be reused here.
> > > >
> > > > Back to the drawing board.
> > > >
> > > > Note: This is a hand-crafted revert due to conflicts. If it
> > > > fails to
> > > > backport, please just try reverting the original commit
> > > > directly.
> > > >
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469
> > > > Reported-by: Rune Petersen <rune@megahurts.dk>
> > > > Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net>
> > > > Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode
> > > > for
> > > > eDP if available.")
> > > > Cc: Clint Taylor <clinton.a.taylor@intel.com>
> > > > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Jim Bride <jim.bride@linux.intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Cc: intel-gfx@lists.freedesktop.org
> > > > Cc: <stable@vger.kernel.org> # v4.14+
> > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_dp.c | 38 +++++-----------------
> > > > ------
> > > > ----------
> > > > 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 | 6 ------
> > > > 6 files changed, 8 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index dde92e4af5d3..8320f0e8e3be 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1679,23 +1679,6 @@ 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)
> > > > -{
> > > > - bool bres = false;
> > > > -
> > > > - if (m1 && m2)
> > > > - bres = (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);
> > > > - return bres;
> > > > -}
> > > > -
> > > > /* Adjust link config limits based on compliance test
> > > > requests. */
> > > > static void
> > > > intel_dp_adjust_compliance_config(struct intel_dp *intel_dp,
> > > > @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct
> > > > intel_encoder
> > > > *encoder,
> > > > pipe_config->has_audio = intel_conn_state-
> > > > >
> > > > > force_audio == HDMI_AUDIO_ON;
> > > >
> > > > if (intel_dp_is_edp(intel_dp) && intel_connector-
> > > > >
> > > > > panel.fixed_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);
> > > > + intel_fixed_panel_mode(intel_connector-
> > > > >
> > > > > panel.fixed_mode,
> > > > + adjusted_mode);
> > > >
> > > > if (INTEL_GEN(dev_priv) >= 9) {
> > > > int ret;
> > > > @@ -6159,7 +6134,6 @@ static bool
> > > > intel_edp_init_connector(struct
> > > > intel_dp *intel_dp,
> > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > struct drm_connector *connector = &intel_connector-
> > > > >base;
> > > > 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;
> > > > @@ -6214,14 +6188,13 @@ static bool
> > > > intel_edp_init_connector(struct
> > > > intel_dp *intel_dp,
> > > > }
> > > > intel_connector->edid = edid;
> > > >
> > > > - /* prefer fixed mode from EDID if available, save an
> > > > alt
> > > > mode also */
> > > > + /* prefer fixed mode from EDID if available */
> > > > 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_connecto
> > > > r,
> > > > fixed_mode);
> > > > - } else if (!alt_fixed_mode) {
> > > > - alt_fixed_mode =
> > > > drm_mode_duplicate(dev,
> > > > scan);
> > > > + break;
> > > If multiple preferred modes are present, we'll now end up calling
> > > drrs_init() only for the first. I see that this is what the
> > > original
> > > code did but this revert does more than removing support for
> > > alternate
> > > modes.
> > It boils down to which preferred mode is *the* preferred mode. I
> > think
> > the original code was, uh, preferrable. Note that drrs init scans
> > the
> > entire list of modes again to find the same size mode with the
> > lowest
> > refresh rate.
> Moreover, as you can see, the original alt mode commit had more
> subtle
> changes than catches the eye, it caused regressions, and I feel
> pretty
> strongly about getting back to the drawing board and starting over
> with
> a clean slate than trying to tweak it when we are quite frankly way
> overdue with the revert. If after that you think the drrs/downclock
> selection needs tweaking, let's do that.
Yeah, agreed on starting with a clean slate.
The offending commit claims alternate mode was intended to be used for
PSR testing. I don't see any psr_basic failures in BAT or any other PSR
sub-tests failing on shards, must have been a non-CI machine then.
The revert itself looks correct,
Reviewed-by: Dhinaakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>
> BR,
> Jani.
>
>
> >
> >
> > BR,
> > Jani.
> >
> > >
> > >
> > > >
> > > > }
> > > > }
> > > >
> > > > @@ -6258,8 +6231,7 @@ static bool
> > > > intel_edp_init_connector(struct
> > > > intel_dp *intel_dp,
> > > > pipe_name(pipe));
> > > > }
> > > >
> > > > - intel_panel_init(&intel_connector->panel, fixed_mode,
> > > > alt_fixed_mode,
> > > > - downclock_mode);
> > > > + intel_panel_init(&intel_connector->panel, 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 d7dbca1aabff..0361130500a6 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -277,7 +277,6 @@ 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;
> > > >
> > > > /* backlight */
> > > > @@ -1850,7 +1849,6 @@ 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 51a1d6868b1e..cf39ca90d887 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > > > @@ -1846,7 +1846,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,
> > > > NULL);
> > > > + intel_panel_init(&intel_connector->panel, fixed_mode,
> > > > 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 eb0c559b2715..a70d767313aa 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > > > @@ -536,7 +536,7 @@ void intel_dvo_init(struct drm_i915_private
> > > > *dev_priv)
> > > > */
> > > > intel_panel_init(&intel_connector-
> > > > >panel,
> > > > intel_dvo_get_current
> > > > _mode(
> > > > intel_encoder),
> > > > - 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 8691c86f579c..d8ece907ff54 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > > > @@ -1140,8 +1140,7 @@ 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,
> > > > NULL,
> > > > - downclock_mode);
> > > > + intel_panel_init(&intel_connector->panel, fixed_mode,
> > > > 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 41d00b1603e3..b443278e569c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_panel.c
> > > > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > > > @@ -1928,13 +1928,11 @@ 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;
> > > > @@ -1948,10 +1946,6 @@ void intel_panel_fini(struct intel_panel
> > > > *panel)
> > > > if (panel->fixed_mode)
> > > > drm_mode_destroy(intel_connector->base.dev,
> > > > panel-
> > > > >
> > > > > fixed_mode);
> > > >
> > > > - if (panel->alt_fixed_mode)
> > > > - drm_mode_destroy(intel_connector->base.dev,
> > > > - panel->alt_fixed_mode);
> > > > -
> > > > if (panel->downclock_mode)
> > > > drm_mode_destroy(intel_connector->base.dev,
> > > > panel->downclock_mode);
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Revert "drm/i915/edp: Allow alternate fixed mode for eDP if available."
2018-05-17 19:44 ` [Intel-gfx] " Dhinakaran Pandiyan
@ 2018-05-18 19:53 ` Dhinakaran Pandiyan
0 siblings, 0 replies; 14+ messages in thread
From: Dhinakaran Pandiyan @ 2018-05-18 19:53 UTC (permalink / raw)
To: intel-gfx, dhinakaran.pandiyan; +Cc: Jani Nikula
On Thursday, May 17, 2018 12:44:30 PM PDT Dhinakaran Pandiyan wrote:
> On Thu, 2018-05-17 at 10:33 +0300, Jani Nikula wrote:
> > On Thu, 17 May 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> > > On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel
> > >
> > > .com> wrote:
> > > > On Wed, 2018-05-16 at 11:01 +0300, Jani Nikula wrote:
> > > > > This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e.
> > > > >
> > > > > Per the report, no matter what display mode you select with
> > > > > xrandr,
> > > > > the
> > > > > i915 driver will always select the alternate fixed mode. For
> > > > > the
> > > > > reporter this means that the display will always run at 40Hz
> > > > > which is
> > > > > quite annoying. This may be due to the mode comparison.
> > > > >
> > > > > But there are some other potential issues. The choice of
> > > > > alt_fixed_mode
> > > > > seems dubious. It's the first non-preferred mode, but there are
> > > > > no
> > > > > guarantees that the only difference would be refresh rate.
> > > > > Similarly,
> > > > > there may be more than one preferred mode in the probed modes
> > > > > list,
> > > > > and
> > > > > the commit changes the preferred mode selection to choose the
> > > > > last
> > > > > one
> > > > > on the list instead of the first.
> > > > >
> > > > > (Note that the probed modes list is the raw, unfiltered,
> > > > > unsorted
> > > > > list
> > > > > of modes from drm_add_edid_modes(), not the pretty result after
> > > > > a
> > > > > drm_helper_probe_single_connector_modes() call.)
> > > > >
> > > > > Finally, we already have eerily similar code in place to find
> > > > > the
> > > > > downclock mode for DRRS that seems like could be reused here.
> > > > >
> > > > > Back to the drawing board.
> > > > >
> > > > > Note: This is a hand-crafted revert due to conflicts. If it
> > > > > fails to
> > > > > backport, please just try reverting the original commit
> > > > > directly.
> > > > >
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469
> > > > > Reported-by: Rune Petersen <rune@megahurts.dk>
> > > > > Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net>
> > > > > Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode
> > > > > for
> > > > > eDP if available.")
> > > > > Cc: Clint Taylor <clinton.a.taylor@intel.com>
> > > > > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Jim Bride <jim.bride@linux.intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > > Cc: intel-gfx@lists.freedesktop.org
> > > > > Cc: <stable@vger.kernel.org> # v4.14+
> > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_dp.c | 38 +++++-----------------
> > > > > ------
> > > > > ----------
> > > > > 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 | 6 ------
> > > > > 6 files changed, 8 insertions(+), 45 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index dde92e4af5d3..8320f0e8e3be 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -1679,23 +1679,6 @@ 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)
> > > > > -{
> > > > > - bool bres = false;
> > > > > -
> > > > > - if (m1 && m2)
> > > > > - bres = (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);
> > > > > - return bres;
> > > > > -}
> > > > > -
> > > > > /* Adjust link config limits based on compliance test
> > > > > requests. */
> > > > > static void
> > > > > intel_dp_adjust_compliance_config(struct intel_dp *intel_dp,
> > > > > @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct
> > > > > intel_encoder
> > > > > *encoder,
> > > > > pipe_config->has_audio = intel_conn_state-
> > > > >
> > > > > > force_audio == HDMI_AUDIO_ON;
> > > > >
> > > > >
> > > > > if (intel_dp_is_edp(intel_dp) && intel_connector-
> > > > >
> > > > > > panel.fixed_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);
> > > > > + intel_fixed_panel_mode(intel_connector-
> > > > >
> > > > > > panel.fixed_mode,
> > > > >
> > > > > + adjusted_mode);
> > > > >
> > > > > if (INTEL_GEN(dev_priv) >= 9) {
> > > > > int ret;
> > > > > @@ -6159,7 +6134,6 @@ static bool
> > > > > intel_edp_init_connector(struct
> > > > > intel_dp *intel_dp,
> > > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > > struct drm_connector *connector = &intel_connector-
> > > > >
> > > > > >base;
> > > > >
> > > > > 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;
> > > > > @@ -6214,14 +6188,13 @@ static bool
> > > > > intel_edp_init_connector(struct
> > > > > intel_dp *intel_dp,
> > > > > }
> > > > > intel_connector->edid = edid;
> > > > >
> > > > > - /* prefer fixed mode from EDID if available, save an
> > > > > alt
> > > > > mode also */
> > > > > + /* prefer fixed mode from EDID if available */
> > > > > 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_connecto
> > > > > r,
> > > > > fixed_mode);
> > > > > - } else if (!alt_fixed_mode) {
> > > > > - alt_fixed_mode =
> > > > > drm_mode_duplicate(dev,
> > > > > scan);
> > > > > + break;
> > > >
> > > > If multiple preferred modes are present, we'll now end up calling
> > > > drrs_init() only for the first. I see that this is what the
> > > > original
> > > > code did but this revert does more than removing support for
> > > > alternate
> > > > modes.
> > >
> > > It boils down to which preferred mode is *the* preferred mode. I
> > > think
> > > the original code was, uh, preferrable. Note that drrs init scans
> > > the
> > > entire list of modes again to find the same size mode with the
> > > lowest
> > > refresh rate.
> >
> > Moreover, as you can see, the original alt mode commit had more
> > subtle
> > changes than catches the eye, it caused regressions, and I feel
> > pretty
> > strongly about getting back to the drawing board and starting over
> > with
> > a clean slate than trying to tweak it when we are quite frankly way
> > overdue with the revert. If after that you think the drrs/downclock
> > selection needs tweaking, let's do that.
>
> Yeah, agreed on starting with a clean slate.
>
> The offending commit claims alternate mode was intended to be used for
> PSR testing. I don't see any psr_basic failures in BAT or any other PSR
> sub-tests failing on shards, must have been a non-CI machine then.
>
> The revert itself looks correct,
> Reviewed-by: Dhinaakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Hit Send too early and Cancel too late. The signature was misspelt, hope it
doesn't confuse dim.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Intel-gfx] [PATCH] Revert "drm/i915/edp: Allow alternate fixed mode for eDP if available."
2018-05-17 7:33 ` Jani Nikula
@ 2018-05-17 19:44 ` Dhinakaran Pandiyan
-1 siblings, 0 replies; 14+ messages in thread
From: Dhinakaran Pandiyan @ 2018-05-17 19:44 UTC (permalink / raw)
To: Jani Nikula, intel-gfx; +Cc: Paulo Zanoni, # v4 . 14+, Rodrigo Vivi
On Thu, 2018-05-17 at 10:33 +0300, Jani Nikula wrote:
> On Thu, 17 May 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> >
> > On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel
> > .com> wrote:
> > >
> > > On Wed, 2018-05-16 at 11:01 +0300, Jani Nikula wrote:
> > > >
> > > > This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e.
> > > >
> > > > Per the report, no matter what display mode you select with
> > > > xrandr,
> > > > the
> > > > i915 driver will always select the alternate fixed mode. For
> > > > the
> > > > reporter this means that the display will always run at 40Hz
> > > > which is
> > > > quite annoying. This may be due to the mode comparison.
> > > >
> > > > But there are some other potential issues. The choice of
> > > > alt_fixed_mode
> > > > seems dubious. It's the first non-preferred mode, but there are
> > > > no
> > > > guarantees that the only difference would be refresh rate.
> > > > Similarly,
> > > > there may be more than one preferred mode in the probed modes
> > > > list,
> > > > and
> > > > the commit changes the preferred mode selection to choose the
> > > > last
> > > > one
> > > > on the list instead of the first.
> > > >
> > > > (Note that the probed modes list is the raw, unfiltered,
> > > > unsorted
> > > > list
> > > > of modes from drm_add_edid_modes(), not the pretty result after
> > > > a
> > > > drm_helper_probe_single_connector_modes() call.)
> > > >
> > > > Finally, we already have eerily similar code in place to find
> > > > the
> > > > downclock mode for DRRS that seems like could be reused here.
> > > >
> > > > Back to the drawing board.
> > > >
> > > > Note: This is a hand-crafted revert due to conflicts. If it
> > > > fails to
> > > > backport, please just try reverting the original commit
> > > > directly.
> > > >
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469
> > > > Reported-by: Rune Petersen <rune@megahurts.dk>
> > > > Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net>
> > > > Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode
> > > > for
> > > > eDP if available.")
> > > > Cc: Clint Taylor <clinton.a.taylor@intel.com>
> > > > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Jim Bride <jim.bride@linux.intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Cc: intel-gfx@lists.freedesktop.org
> > > > Cc: <stable@vger.kernel.org> # v4.14+
> > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_dp.c | 38 +++++-----------------
> > > > ------
> > > > ----------
> > > > 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 | 6 ------
> > > > 6 files changed, 8 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index dde92e4af5d3..8320f0e8e3be 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1679,23 +1679,6 @@ 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)
> > > > -{
> > > > - bool bres = false;
> > > > -
> > > > - if (m1 && m2)
> > > > - bres = (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);
> > > > - return bres;
> > > > -}
> > > > -
> > > > /* Adjust link config limits based on compliance test
> > > > requests. */
> > > > static void
> > > > intel_dp_adjust_compliance_config(struct intel_dp *intel_dp,
> > > > @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct
> > > > intel_encoder
> > > > *encoder,
> > > > pipe_config->has_audio = intel_conn_state-
> > > > >
> > > > > force_audio == HDMI_AUDIO_ON;
> > > >
> > > > if (intel_dp_is_edp(intel_dp) && intel_connector-
> > > > >
> > > > > panel.fixed_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);
> > > > + intel_fixed_panel_mode(intel_connector-
> > > > >
> > > > > panel.fixed_mode,
> > > > + adjusted_mode);
> > > >
> > > > if (INTEL_GEN(dev_priv) >= 9) {
> > > > int ret;
> > > > @@ -6159,7 +6134,6 @@ static bool
> > > > intel_edp_init_connector(struct
> > > > intel_dp *intel_dp,
> > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > struct drm_connector *connector = &intel_connector-
> > > > >base;
> > > > 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;
> > > > @@ -6214,14 +6188,13 @@ static bool
> > > > intel_edp_init_connector(struct
> > > > intel_dp *intel_dp,
> > > > }
> > > > intel_connector->edid = edid;
> > > >
> > > > - /* prefer fixed mode from EDID if available, save an
> > > > alt
> > > > mode also */
> > > > + /* prefer fixed mode from EDID if available */
> > > > 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_connecto
> > > > r,
> > > > fixed_mode);
> > > > - } else if (!alt_fixed_mode) {
> > > > - alt_fixed_mode =
> > > > drm_mode_duplicate(dev,
> > > > scan);
> > > > + break;
> > > If multiple preferred modes are present, we'll now end up calling
> > > drrs_init() only for the first. I see that this is what the
> > > original
> > > code did but this revert does more than removing support for
> > > alternate
> > > modes.
> > It boils down to which preferred mode is *the* preferred mode. I
> > think
> > the original code was, uh, preferrable. Note that drrs init scans
> > the
> > entire list of modes again to find the same size mode with the
> > lowest
> > refresh rate.
> Moreover, as you can see, the original alt mode commit had more
> subtle
> changes than catches the eye, it caused regressions, and I feel
> pretty
> strongly about getting back to the drawing board and starting over
> with
> a clean slate than trying to tweak it when we are quite frankly way
> overdue with the revert. If after that you think the drrs/downclock
> selection needs tweaking, let's do that.
Yeah, agreed on starting with a clean slate.
The offending commit claims alternate mode was intended to be used for
PSR testing. I don't see any psr_basic failures in BAT or any other PSR
sub-tests failing on shards, must have been a non-CI machine then.
The revert itself looks correct,
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>
> BR,
> Jani.
>
>
> >
> >
> > BR,
> > Jani.
> >
> > >
> > >
> > > >
> > > > }
> > > > }
> > > >
> > > > @@ -6258,8 +6231,7 @@ static bool
> > > > intel_edp_init_connector(struct
> > > > intel_dp *intel_dp,
> > > > pipe_name(pipe));
> > > > }
> > > >
> > > > - intel_panel_init(&intel_connector->panel, fixed_mode,
> > > > alt_fixed_mode,
> > > > - downclock_mode);
> > > > + intel_panel_init(&intel_connector->panel, 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 d7dbca1aabff..0361130500a6 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -277,7 +277,6 @@ 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;
> > > >
> > > > /* backlight */
> > > > @@ -1850,7 +1849,6 @@ 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 51a1d6868b1e..cf39ca90d887 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > > > @@ -1846,7 +1846,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,
> > > > NULL);
> > > > + intel_panel_init(&intel_connector->panel, fixed_mode,
> > > > 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 eb0c559b2715..a70d767313aa 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > > > @@ -536,7 +536,7 @@ void intel_dvo_init(struct drm_i915_private
> > > > *dev_priv)
> > > > */
> > > > intel_panel_init(&intel_connector-
> > > > >panel,
> > > > intel_dvo_get_current
> > > > _mode(
> > > > intel_encoder),
> > > > - 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 8691c86f579c..d8ece907ff54 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > > > @@ -1140,8 +1140,7 @@ 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,
> > > > NULL,
> > > > - downclock_mode);
> > > > + intel_panel_init(&intel_connector->panel, fixed_mode,
> > > > 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 41d00b1603e3..b443278e569c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_panel.c
> > > > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > > > @@ -1928,13 +1928,11 @@ 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;
> > > > @@ -1948,10 +1946,6 @@ void intel_panel_fini(struct intel_panel
> > > > *panel)
> > > > if (panel->fixed_mode)
> > > > drm_mode_destroy(intel_connector->base.dev,
> > > > panel-
> > > > >
> > > > > fixed_mode);
> > > >
> > > > - if (panel->alt_fixed_mode)
> > > > - drm_mode_destroy(intel_connector->base.dev,
> > > > - panel->alt_fixed_mode);
> > > > -
> > > > if (panel->downclock_mode)
> > > > drm_mode_destroy(intel_connector->base.dev,
> > > > panel->downclock_mode);
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] Revert "drm/i915/edp: Allow alternate fixed mode for eDP if available."
@ 2018-05-17 19:44 ` Dhinakaran Pandiyan
0 siblings, 0 replies; 14+ messages in thread
From: Dhinakaran Pandiyan @ 2018-05-17 19:44 UTC (permalink / raw)
To: Jani Nikula, intel-gfx; +Cc: Paulo Zanoni, # v4 . 14+, Rodrigo Vivi
On Thu, 2018-05-17 at 10:33 +0300, Jani Nikula wrote:
> On Thu, 17 May 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> >
> > On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel
> > .com> wrote:
> > >
> > > On Wed, 2018-05-16 at 11:01 +0300, Jani Nikula wrote:
> > > >
> > > > This reverts commit dc911f5bd8aacfcf8aabd5c26c88e04c837a938e.
> > > >
> > > > Per the report, no matter what display mode you select with
> > > > xrandr,
> > > > the
> > > > i915 driver will always select the alternate fixed mode. For
> > > > the
> > > > reporter this means that the display will always run at 40Hz
> > > > which is
> > > > quite annoying. This may be due to the mode comparison.
> > > >
> > > > But there are some other potential issues. The choice of
> > > > alt_fixed_mode
> > > > seems dubious. It's the first non-preferred mode, but there are
> > > > no
> > > > guarantees that the only difference would be refresh rate.
> > > > Similarly,
> > > > there may be more than one preferred mode in the probed modes
> > > > list,
> > > > and
> > > > the commit changes the preferred mode selection to choose the
> > > > last
> > > > one
> > > > on the list instead of the first.
> > > >
> > > > (Note that the probed modes list is the raw, unfiltered,
> > > > unsorted
> > > > list
> > > > of modes from drm_add_edid_modes(), not the pretty result after
> > > > a
> > > > drm_helper_probe_single_connector_modes() call.)
> > > >
> > > > Finally, we already have eerily similar code in place to find
> > > > the
> > > > downclock mode for DRRS that seems like could be reused here.
> > > >
> > > > Back to the drawing board.
> > > >
> > > > Note: This is a hand-crafted revert due to conflicts. If it
> > > > fails to
> > > > backport, please just try reverting the original commit
> > > > directly.
> > > >
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105469
> > > > Reported-by: Rune Petersen <rune@megahurts.dk>
> > > > Reported-by: Mark Spencer <n7u4722r35@ynzlx.anonbox.net>
> > > > Fixes: dc911f5bd8aa ("drm/i915/edp: Allow alternate fixed mode
> > > > for
> > > > eDP if available.")
> > > > Cc: Clint Taylor <clinton.a.taylor@intel.com>
> > > > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Jim Bride <jim.bride@linux.intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Cc: intel-gfx@lists.freedesktop.org
> > > > Cc: <stable@vger.kernel.org> # v4.14+
> > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_dp.c | 38 +++++-----------------
> > > > ------
> > > > ----------
> > > > 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 | 6 ------
> > > > 6 files changed, 8 insertions(+), 45 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index dde92e4af5d3..8320f0e8e3be 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -1679,23 +1679,6 @@ 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)
> > > > -{
> > > > - bool bres = false;
> > > > -
> > > > - if (m1 && m2)
> > > > - bres = (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);
> > > > - return bres;
> > > > -}
> > > > -
> > > > /* Adjust link config limits based on compliance test
> > > > requests. */
> > > > static void
> > > > intel_dp_adjust_compliance_config(struct intel_dp *intel_dp,
> > > > @@ -1860,16 +1843,8 @@ intel_dp_compute_config(struct
> > > > intel_encoder
> > > > *encoder,
> > > > pipe_config->has_audio = intel_conn_state-
> > > > >
> > > > > force_audio == HDMI_AUDIO_ON;
> > > >
> > > > if (intel_dp_is_edp(intel_dp) && intel_connector-
> > > > >
> > > > > panel.fixed_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);
> > > > + intel_fixed_panel_mode(intel_connector-
> > > > >
> > > > > panel.fixed_mode,
> > > > + adjusted_mode);
> > > >
> > > > if (INTEL_GEN(dev_priv) >= 9) {
> > > > int ret;
> > > > @@ -6159,7 +6134,6 @@ static bool
> > > > intel_edp_init_connector(struct
> > > > intel_dp *intel_dp,
> > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > struct drm_connector *connector = &intel_connector-
> > > > >base;
> > > > 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;
> > > > @@ -6214,14 +6188,13 @@ static bool
> > > > intel_edp_init_connector(struct
> > > > intel_dp *intel_dp,
> > > > }
> > > > intel_connector->edid = edid;
> > > >
> > > > - /* prefer fixed mode from EDID if available, save an
> > > > alt
> > > > mode also */
> > > > + /* prefer fixed mode from EDID if available */
> > > > 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_connecto
> > > > r,
> > > > fixed_mode);
> > > > - } else if (!alt_fixed_mode) {
> > > > - alt_fixed_mode =
> > > > drm_mode_duplicate(dev,
> > > > scan);
> > > > + break;
> > > If multiple preferred modes are present, we'll now end up calling
> > > drrs_init() only for the first. I see that this is what the
> > > original
> > > code did but this revert does more than removing support for
> > > alternate
> > > modes.
> > It boils down to which preferred mode is *the* preferred mode. I
> > think
> > the original code was, uh, preferrable. Note that drrs init scans
> > the
> > entire list of modes again to find the same size mode with the
> > lowest
> > refresh rate.
> Moreover, as you can see, the original alt mode commit had more
> subtle
> changes than catches the eye, it caused regressions, and I feel
> pretty
> strongly about getting back to the drawing board and starting over
> with
> a clean slate than trying to tweak it when we are quite frankly way
> overdue with the revert. If after that you think the drrs/downclock
> selection needs tweaking, let's do that.
Yeah, agreed on starting with a clean slate.
The offending commit claims alternate mode was intended to be used for
PSR testing. I don't see any psr_basic failures in BAT or any other PSR
sub-tests failing on shards, must have been a non-CI machine then.
The revert itself looks correct,
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>
> BR,
> Jani.
>
>
> >
> >
> > BR,
> > Jani.
> >
> > >
> > >
> > > >
> > > > }
> > > > }
> > > >
> > > > @@ -6258,8 +6231,7 @@ static bool
> > > > intel_edp_init_connector(struct
> > > > intel_dp *intel_dp,
> > > > pipe_name(pipe));
> > > > }
> > > >
> > > > - intel_panel_init(&intel_connector->panel, fixed_mode,
> > > > alt_fixed_mode,
> > > > - downclock_mode);
> > > > + intel_panel_init(&intel_connector->panel, 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 d7dbca1aabff..0361130500a6 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -277,7 +277,6 @@ 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;
> > > >
> > > > /* backlight */
> > > > @@ -1850,7 +1849,6 @@ 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 51a1d6868b1e..cf39ca90d887 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > > > @@ -1846,7 +1846,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,
> > > > NULL);
> > > > + intel_panel_init(&intel_connector->panel, fixed_mode,
> > > > 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 eb0c559b2715..a70d767313aa 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > > > @@ -536,7 +536,7 @@ void intel_dvo_init(struct drm_i915_private
> > > > *dev_priv)
> > > > */
> > > > intel_panel_init(&intel_connector-
> > > > >panel,
> > > > intel_dvo_get_current
> > > > _mode(
> > > > intel_encoder),
> > > > - 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 8691c86f579c..d8ece907ff54 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > > > @@ -1140,8 +1140,7 @@ 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,
> > > > NULL,
> > > > - downclock_mode);
> > > > + intel_panel_init(&intel_connector->panel, fixed_mode,
> > > > 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 41d00b1603e3..b443278e569c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_panel.c
> > > > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > > > @@ -1928,13 +1928,11 @@ 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;
> > > > @@ -1948,10 +1946,6 @@ void intel_panel_fini(struct intel_panel
> > > > *panel)
> > > > if (panel->fixed_mode)
> > > > drm_mode_destroy(intel_connector->base.dev,
> > > > panel-
> > > > >
> > > > > fixed_mode);
> > > >
> > > > - if (panel->alt_fixed_mode)
> > > > - drm_mode_destroy(intel_connector->base.dev,
> > > > - panel->alt_fixed_mode);
> > > > -
> > > > if (panel->downclock_mode)
> > > > drm_mode_destroy(intel_connector->base.dev,
> > > > panel->downclock_mode);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Intel-gfx] [PATCH] Revert "drm/i915/edp: Allow alternate fixed mode for eDP if available."
2018-05-17 19:44 ` Dhinakaran Pandiyan
(?)
@ 2018-05-22 9:44 ` Jani Nikula
-1 siblings, 0 replies; 14+ messages in thread
From: Jani Nikula @ 2018-05-22 9:44 UTC (permalink / raw)
To: dhinakaran.pandiyan, intel-gfx; +Cc: Paulo Zanoni, # v4 . 14+, Rodrigo Vivi
On Thu, 17 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> Yeah, agreed on starting with a clean slate.
>
> The offending commit claims alternate mode was intended to be used for
> PSR testing. I don't see any psr_basic failures in BAT or any other PSR
> sub-tests failing on shards, must have been a non-CI machine then.
>
> The revert itself looks correct,
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Thanks for the review, pushed to dinq.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 14+ messages in thread