| Message ID | 1379059389-2890-2-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote: > Flat out skip anything to do with PLL if we have a DSI encoder (and thus > DSI PLL). Also skip PLL computation if the encoder has already set > clocks. This allows for some tidying up of the code, including a > superfluous call to intel_limit() for LVDS downclock path. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Tried to merge this but the baseline seems to be off, at least wrt dinq. Do I miss some patches that I should apply first? -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++----------------- > 1 file changed, 22 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0446dc7..6a51cc2 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > num_connectors++; > } > > - refclk = i9xx_get_refclk(crtc, num_connectors); > + if (is_dsi) > + goto skip_dpll; > + > + if (!intel_crtc->config.clock_set) { > + refclk = i9xx_get_refclk(crtc, num_connectors); > > - if (!is_dsi && !intel_crtc->config.clock_set) { > /* > * Returns a set of divisors for the desired target clock with > * the given refclk, or FALSE. The returned values represent > @@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > ok = dev_priv->display.find_dpll(limit, crtc, > intel_crtc->config.port_clock, > refclk, NULL, &clock); > - if (!ok && !intel_crtc->config.clock_set) { > + if (!ok) { > DRM_ERROR("Couldn't find PLL settings for mode!\n"); > return -EINVAL; > } > - } > > - if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) { > - /* > - * Ensure we match the reduced clock's P to the target clock. > - * If the clocks don't match, we can't switch the display clock > - * by using the FP0/FP1. In such case we will disable the LVDS > - * downclock feature. > - */ > - limit = intel_limit(crtc, refclk); > - has_reduced_clock = > - dev_priv->display.find_dpll(limit, crtc, > - dev_priv->lvds_downclock, > - refclk, &clock, > - &reduced_clock); > - } > - /* Compat-code for transition, will disappear. */ > - if (!intel_crtc->config.clock_set) { > + if (is_lvds && dev_priv->lvds_downclock_avail) { > + /* > + * Ensure we match the reduced clock's P to the target > + * clock. If the clocks don't match, we can't switch > + * the display clock by using the FP0/FP1. In such case > + * we will disable the LVDS downclock feature. > + */ > + has_reduced_clock = > + dev_priv->display.find_dpll(limit, crtc, > + dev_priv->lvds_downclock, > + refclk, &clock, > + &reduced_clock); > + } > + /* Compat-code for transition, will disappear. */ > intel_crtc->config.dpll.n = clock.n; > intel_crtc->config.dpll.m1 = clock.m1; > intel_crtc->config.dpll.m2 = clock.m2; > @@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > has_reduced_clock ? &reduced_clock : NULL, > num_connectors); > } else if (IS_VALLEYVIEW(dev)) { > - if (!is_dsi) > - vlv_update_pll(intel_crtc); > + vlv_update_pll(intel_crtc); > } else { > i9xx_update_pll(intel_crtc, > has_reduced_clock ? &reduced_clock : NULL, > num_connectors); > } > > +skip_dpll: > /* Set up the display plane register */ > dspcntr = DISPPLANE_GAMMA_ENABLE; > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 13 Sep 2013, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote: >> Flat out skip anything to do with PLL if we have a DSI encoder (and thus >> DSI PLL). Also skip PLL computation if the encoder has already set >> clocks. This allows for some tidying up of the code, including a >> superfluous call to intel_limit() for LVDS downclock path. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Tried to merge this but the baseline seems to be off, at least wrt dinq. > Do I miss some patches that I should apply first? This one depends on "drm/i915: do not update cursor in crtc mode set" in this thread (my patch but with Ville's commit message amendmend). Did you push the assert patch before that? It's a good order. Jani. > -Daniel > >> --- >> drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++----------------- >> 1 file changed, 22 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 0446dc7..6a51cc2 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, >> num_connectors++; >> } >> >> - refclk = i9xx_get_refclk(crtc, num_connectors); >> + if (is_dsi) >> + goto skip_dpll; >> + >> + if (!intel_crtc->config.clock_set) { >> + refclk = i9xx_get_refclk(crtc, num_connectors); >> >> - if (!is_dsi && !intel_crtc->config.clock_set) { >> /* >> * Returns a set of divisors for the desired target clock with >> * the given refclk, or FALSE. The returned values represent >> @@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, >> ok = dev_priv->display.find_dpll(limit, crtc, >> intel_crtc->config.port_clock, >> refclk, NULL, &clock); >> - if (!ok && !intel_crtc->config.clock_set) { >> + if (!ok) { >> DRM_ERROR("Couldn't find PLL settings for mode!\n"); >> return -EINVAL; >> } >> - } >> >> - if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) { >> - /* >> - * Ensure we match the reduced clock's P to the target clock. >> - * If the clocks don't match, we can't switch the display clock >> - * by using the FP0/FP1. In such case we will disable the LVDS >> - * downclock feature. >> - */ >> - limit = intel_limit(crtc, refclk); >> - has_reduced_clock = >> - dev_priv->display.find_dpll(limit, crtc, >> - dev_priv->lvds_downclock, >> - refclk, &clock, >> - &reduced_clock); >> - } >> - /* Compat-code for transition, will disappear. */ >> - if (!intel_crtc->config.clock_set) { >> + if (is_lvds && dev_priv->lvds_downclock_avail) { >> + /* >> + * Ensure we match the reduced clock's P to the target >> + * clock. If the clocks don't match, we can't switch >> + * the display clock by using the FP0/FP1. In such case >> + * we will disable the LVDS downclock feature. >> + */ >> + has_reduced_clock = >> + dev_priv->display.find_dpll(limit, crtc, >> + dev_priv->lvds_downclock, >> + refclk, &clock, >> + &reduced_clock); >> + } >> + /* Compat-code for transition, will disappear. */ >> intel_crtc->config.dpll.n = clock.n; >> intel_crtc->config.dpll.m1 = clock.m1; >> intel_crtc->config.dpll.m2 = clock.m2; >> @@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, >> has_reduced_clock ? &reduced_clock : NULL, >> num_connectors); >> } else if (IS_VALLEYVIEW(dev)) { >> - if (!is_dsi) >> - vlv_update_pll(intel_crtc); >> + vlv_update_pll(intel_crtc); >> } else { >> i9xx_update_pll(intel_crtc, >> has_reduced_clock ? &reduced_clock : NULL, >> num_connectors); >> } >> >> +skip_dpll: >> /* Set up the display plane register */ >> dspcntr = DISPPLANE_GAMMA_ENABLE; >> >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Sep 13, 2013 at 04:38:28PM +0300, Jani Nikula wrote: > On Fri, 13 Sep 2013, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote: > >> Flat out skip anything to do with PLL if we have a DSI encoder (and thus > >> DSI PLL). Also skip PLL computation if the encoder has already set > >> clocks. This allows for some tidying up of the code, including a > >> superfluous call to intel_limit() for LVDS downclock path. > >> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Tried to merge this but the baseline seems to be off, at least wrt dinq. > > Do I miss some patches that I should apply first? > > This one depends on "drm/i915: do not update cursor in crtc mode set" in > this thread (my patch but with Ville's commit message amendmend). Did > you push the assert patch before that? It's a good order. I've pushed the assert patch to dinq, the real fixes need to go to -fixes. Tbh I'm a bit confused about what to put where, so please scream ;-) For Ville's patches I'm waiting a bit for Paulo to take a look and ack them. -Daniel > > Jani. > > > > > -Daniel > > > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++----------------- > >> 1 file changed, 22 insertions(+), 22 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 0446dc7..6a51cc2 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > >> num_connectors++; > >> } > >> > >> - refclk = i9xx_get_refclk(crtc, num_connectors); > >> + if (is_dsi) > >> + goto skip_dpll; > >> + > >> + if (!intel_crtc->config.clock_set) { > >> + refclk = i9xx_get_refclk(crtc, num_connectors); > >> > >> - if (!is_dsi && !intel_crtc->config.clock_set) { > >> /* > >> * Returns a set of divisors for the desired target clock with > >> * the given refclk, or FALSE. The returned values represent > >> @@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > >> ok = dev_priv->display.find_dpll(limit, crtc, > >> intel_crtc->config.port_clock, > >> refclk, NULL, &clock); > >> - if (!ok && !intel_crtc->config.clock_set) { > >> + if (!ok) { > >> DRM_ERROR("Couldn't find PLL settings for mode!\n"); > >> return -EINVAL; > >> } > >> - } > >> > >> - if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) { > >> - /* > >> - * Ensure we match the reduced clock's P to the target clock. > >> - * If the clocks don't match, we can't switch the display clock > >> - * by using the FP0/FP1. In such case we will disable the LVDS > >> - * downclock feature. > >> - */ > >> - limit = intel_limit(crtc, refclk); > >> - has_reduced_clock = > >> - dev_priv->display.find_dpll(limit, crtc, > >> - dev_priv->lvds_downclock, > >> - refclk, &clock, > >> - &reduced_clock); > >> - } > >> - /* Compat-code for transition, will disappear. */ > >> - if (!intel_crtc->config.clock_set) { > >> + if (is_lvds && dev_priv->lvds_downclock_avail) { > >> + /* > >> + * Ensure we match the reduced clock's P to the target > >> + * clock. If the clocks don't match, we can't switch > >> + * the display clock by using the FP0/FP1. In such case > >> + * we will disable the LVDS downclock feature. > >> + */ > >> + has_reduced_clock = > >> + dev_priv->display.find_dpll(limit, crtc, > >> + dev_priv->lvds_downclock, > >> + refclk, &clock, > >> + &reduced_clock); > >> + } > >> + /* Compat-code for transition, will disappear. */ > >> intel_crtc->config.dpll.n = clock.n; > >> intel_crtc->config.dpll.m1 = clock.m1; > >> intel_crtc->config.dpll.m2 = clock.m2; > >> @@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > >> has_reduced_clock ? &reduced_clock : NULL, > >> num_connectors); > >> } else if (IS_VALLEYVIEW(dev)) { > >> - if (!is_dsi) > >> - vlv_update_pll(intel_crtc); > >> + vlv_update_pll(intel_crtc); > >> } else { > >> i9xx_update_pll(intel_crtc, > >> has_reduced_clock ? &reduced_clock : NULL, > >> num_connectors); > >> } > >> > >> +skip_dpll: > >> /* Set up the display plane register */ > >> dspcntr = DISPPLANE_GAMMA_ENABLE; > >> > >> -- > >> 1.7.9.5 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
On Fri, Sep 13, 2013 at 04:07:19PM +0200, Daniel Vetter wrote: > On Fri, Sep 13, 2013 at 04:38:28PM +0300, Jani Nikula wrote: > > On Fri, 13 Sep 2013, Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Fri, Sep 13, 2013 at 11:03:09AM +0300, Jani Nikula wrote: > > >> Flat out skip anything to do with PLL if we have a DSI encoder (and thus > > >> DSI PLL). Also skip PLL computation if the encoder has already set > > >> clocks. This allows for some tidying up of the code, including a > > >> superfluous call to intel_limit() for LVDS downclock path. > > >> > > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Tried to merge this but the baseline seems to be off, at least wrt dinq. > > > Do I miss some patches that I should apply first? > > > > This one depends on "drm/i915: do not update cursor in crtc mode set" in > > this thread (my patch but with Ville's commit message amendmend). Did > > you push the assert patch before that? It's a good order. > > I've pushed the assert patch to dinq, the real fixes need to go to -fixes. > Tbh I'm a bit confused about what to put where, so please scream ;-) > > For Ville's patches I'm waiting a bit for Paulo to take a look and ack > them. For stable we need just (assuming it also fixes Paulo's hangs): drm/i915: do not update cursor in crtc mode set On top of that we can add the cursor assert patch (not sure if we want that to go to stable): drm/i915: add asserts for cursor disabled I get the impression you applied the assert patch alone. That's probably not a very good idea since it will (or at least should) scream every time you do a modeset, unless the other patch is applied as well.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0446dc7..6a51cc2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4892,9 +4892,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, num_connectors++; } - refclk = i9xx_get_refclk(crtc, num_connectors); + if (is_dsi) + goto skip_dpll; + + if (!intel_crtc->config.clock_set) { + refclk = i9xx_get_refclk(crtc, num_connectors); - if (!is_dsi && !intel_crtc->config.clock_set) { /* * Returns a set of divisors for the desired target clock with * the given refclk, or FALSE. The returned values represent @@ -4905,28 +4908,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, ok = dev_priv->display.find_dpll(limit, crtc, intel_crtc->config.port_clock, refclk, NULL, &clock); - if (!ok && !intel_crtc->config.clock_set) { + if (!ok) { DRM_ERROR("Couldn't find PLL settings for mode!\n"); return -EINVAL; } - } - if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) { - /* - * Ensure we match the reduced clock's P to the target clock. - * If the clocks don't match, we can't switch the display clock - * by using the FP0/FP1. In such case we will disable the LVDS - * downclock feature. - */ - limit = intel_limit(crtc, refclk); - has_reduced_clock = - dev_priv->display.find_dpll(limit, crtc, - dev_priv->lvds_downclock, - refclk, &clock, - &reduced_clock); - } - /* Compat-code for transition, will disappear. */ - if (!intel_crtc->config.clock_set) { + if (is_lvds && dev_priv->lvds_downclock_avail) { + /* + * Ensure we match the reduced clock's P to the target + * clock. If the clocks don't match, we can't switch + * the display clock by using the FP0/FP1. In such case + * we will disable the LVDS downclock feature. + */ + has_reduced_clock = + dev_priv->display.find_dpll(limit, crtc, + dev_priv->lvds_downclock, + refclk, &clock, + &reduced_clock); + } + /* Compat-code for transition, will disappear. */ intel_crtc->config.dpll.n = clock.n; intel_crtc->config.dpll.m1 = clock.m1; intel_crtc->config.dpll.m2 = clock.m2; @@ -4939,14 +4939,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, has_reduced_clock ? &reduced_clock : NULL, num_connectors); } else if (IS_VALLEYVIEW(dev)) { - if (!is_dsi) - vlv_update_pll(intel_crtc); + vlv_update_pll(intel_crtc); } else { i9xx_update_pll(intel_crtc, has_reduced_clock ? &reduced_clock : NULL, num_connectors); } +skip_dpll: /* Set up the display plane register */ dspcntr = DISPPLANE_GAMMA_ENABLE;