| Message ID | 1367323306-13605-7-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 30, 2013 at 02:01:45PM +0200, Daniel Vetter wrote: > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -223,6 +222,10 @@ struct intel_crtc_config { > /* Controls for the clock computation, to override various stages. */ > bool clock_set; > > + /* SDVO TV has a bunch of special case. To make multifunction encoders > + * work correctly, we need to track this at runtime.*/ > + bool sdvo_tv_clock; > + > /* > * crtc bandwidth limit, don't increase pipe bpp or clock if not really > * required. This is set in the 2nd loop of calling encoder's What is becoming less clear over time is what is derived state internal to the modesetting sequence, and what is the state used to select a mode e.g. to decide if two configs are compatible. -Chris
On Tue, Apr 30, 2013 at 2:49 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Tue, Apr 30, 2013 at 02:01:45PM +0200, Daniel Vetter wrote: >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -223,6 +222,10 @@ struct intel_crtc_config { >> /* Controls for the clock computation, to override various stages. */ >> bool clock_set; >> >> + /* SDVO TV has a bunch of special case. To make multifunction encoders >> + * work correctly, we need to track this at runtime.*/ >> + bool sdvo_tv_clock; >> + >> /* >> * crtc bandwidth limit, don't increase pipe bpp or clock if not really >> * required. This is set in the 2nd loop of calling encoder's > > What is becoming less clear over time is what is derived state internal > to the modesetting sequence, and what is the state used to select a mode > e.g. to decide if two configs are compatible. Atm everything is derived state and we currently have don't take the pipe config into account for comparing state. My plan is that we eventually add flags to the pipe_config compare function to decide whether we need a full-blown modeset or can go with an "update pfit state" fastpath. So atm I'm just moving state around, since pipe_config will also be used for atomic modeset. And checking the hw config upfront e.g. for pll sharing has similar, but not completely overlapping needs to the state comparison fastboot needs to do. Another thing I don't yet have an opinion about is what we should do if the mode matches, but the configuration details are a bit suboptimal (like wasteful watermark settings). I guess we need to rework the code so that we can fix this all up without stopping the crtc. And there's tons of little bits of state like that (audio config, infoframes, clock trees, watermarks, fdi config, ...). Added fun is that often it is highly platform specific whether a given piece of state must match for fastboot or not ... My current plan is to just chip away with pipe_config conversions, with more a focus towards atomic modeset (i.e. upfront checking), leaving some of the harder fastboot issues for Jesse ;-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, 30 Apr 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > We need to track this correctly. While at it shovel the boolean > to track whether the sdvo is in tv mode or not into pipe_config. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36997 Bug scrubbing, should this be: https://bugs.freedesktop.org/show_bug.cgi?id=63609 ? Jani. > Tested-by: Pierre Assal <pierre.assal@verint.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36997 > Tested-by: cancan,feng <cancan.feng@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 8 +++----- > drivers/gpu/drm/i915/intel_drv.h | 5 ++++- > drivers/gpu/drm/i915/intel_sdvo.c | 8 ++------ > 3 files changed, 9 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 44bcfae..ef0d27b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4546,7 +4546,7 @@ static void i9xx_update_pll(struct intel_crtc *crtc, > if (INTEL_INFO(dev)->gen >= 4) > dpll |= (6 << PLL_LOAD_PULSE_PHASE_SHIFT); > > - if (is_sdvo && intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_TVOUT)) > + if (crtc->config.sdvo_tv_clock) > dpll |= PLL_REF_INPUT_TVCLKINBC; > else if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS) && > intel_panel_use_ssc(dev_priv) && num_connectors < 2) > @@ -5590,7 +5590,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, > struct intel_encoder *intel_encoder; > uint32_t dpll; > int factor, num_connectors = 0; > - bool is_lvds = false, is_sdvo = false, is_tv = false; > + bool is_lvds = false, is_sdvo = false; > > for_each_encoder_on_crtc(dev, crtc, intel_encoder) { > switch (intel_encoder->type) { > @@ -5600,8 +5600,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, > case INTEL_OUTPUT_SDVO: > case INTEL_OUTPUT_HDMI: > is_sdvo = true; > - if (intel_encoder->needs_tv_clock) > - is_tv = true; > break; > } > > @@ -5615,7 +5613,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, > dev_priv->lvds_ssc_freq == 100) || > (HAS_PCH_IBX(dev) && intel_is_dual_link_lvds(dev))) > factor = 25; > - } else if (is_sdvo && is_tv) > + } else if (intel_crtc->config.sdvo_tv_clock) > factor = 20; > > if (ironlake_needs_fb_cb_tune(&intel_crtc->config.dpll, factor)) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 766afcf..be196ff 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -120,7 +120,6 @@ struct intel_encoder { > struct intel_crtc *new_crtc; > > int type; > - bool needs_tv_clock; > /* > * Intel hw has only one MUX where encoders could be clone, hence a > * simple flag is enough to compute the possible_clones mask. > @@ -223,6 +222,10 @@ struct intel_crtc_config { > /* Controls for the clock computation, to override various stages. */ > bool clock_set; > > + /* SDVO TV has a bunch of special case. To make multifunction encoders > + * work correctly, we need to track this at runtime.*/ > + bool sdvo_tv_clock; > + > /* > * crtc bandwidth limit, don't increase pipe bpp or clock if not really > * required. This is set in the 2nd loop of calling encoder's > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index f6bf9fc..3a1d710 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -1092,6 +1092,7 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder, > (void) intel_sdvo_get_preferred_input_mode(intel_sdvo, > mode, > adjusted_mode); > + pipe_config->sdvo_tv_clock = true; > } else if (intel_sdvo->is_lvds) { > if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, > intel_sdvo->sdvo_lvds_fixed_mode)) > @@ -1655,12 +1656,9 @@ intel_sdvo_detect(struct drm_connector *connector, bool force) > if (ret == connector_status_connected) { > intel_sdvo->is_tv = false; > intel_sdvo->is_lvds = false; > - intel_sdvo->base.needs_tv_clock = false; > > - if (response & SDVO_TV_MASK) { > + if (response & SDVO_TV_MASK) > intel_sdvo->is_tv = true; > - intel_sdvo->base.needs_tv_clock = true; > - } > if (response & SDVO_LVDS_MASK) > intel_sdvo->is_lvds = intel_sdvo->sdvo_lvds_fixed_mode != NULL; > } > @@ -2357,7 +2355,6 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type) > intel_sdvo_connector->output_flag = type; > > intel_sdvo->is_tv = true; > - intel_sdvo->base.needs_tv_clock = true; > > intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo); > > @@ -2445,7 +2442,6 @@ static bool > intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, uint16_t flags) > { > intel_sdvo->is_tv = false; > - intel_sdvo->base.needs_tv_clock = false; > intel_sdvo->is_lvds = false; > > /* SDVO requires XXX1 function may not exist unless it has XXX0 function.*/ > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, May 6, 2013 at 3:36 PM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Tue, 30 Apr 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> We need to track this correctly. While at it shovel the boolean >> to track whether the sdvo is in tv mode or not into pipe_config. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36997 > > Bug scrubbing, should this be: > https://bugs.freedesktop.org/show_bug.cgi?id=63609 ? Yeah, I've copy&pasted the wrong bugzilla link, that's the 2nd one. Thanks for spotting. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, 30 Apr 2013, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > We need to track this correctly. While at it shovel the boolean > to track whether the sdvo is in tv mode or not into pipe_config. Reviewed-by: Jani Nikula <jani.nikula@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36997 > Tested-by: Pierre Assal <pierre.assal@verint.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36997 > Tested-by: cancan,feng <cancan.feng@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 8 +++----- > drivers/gpu/drm/i915/intel_drv.h | 5 ++++- > drivers/gpu/drm/i915/intel_sdvo.c | 8 ++------ > 3 files changed, 9 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 44bcfae..ef0d27b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4546,7 +4546,7 @@ static void i9xx_update_pll(struct intel_crtc *crtc, > if (INTEL_INFO(dev)->gen >= 4) > dpll |= (6 << PLL_LOAD_PULSE_PHASE_SHIFT); > > - if (is_sdvo && intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_TVOUT)) > + if (crtc->config.sdvo_tv_clock) > dpll |= PLL_REF_INPUT_TVCLKINBC; > else if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS) && > intel_panel_use_ssc(dev_priv) && num_connectors < 2) > @@ -5590,7 +5590,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, > struct intel_encoder *intel_encoder; > uint32_t dpll; > int factor, num_connectors = 0; > - bool is_lvds = false, is_sdvo = false, is_tv = false; > + bool is_lvds = false, is_sdvo = false; > > for_each_encoder_on_crtc(dev, crtc, intel_encoder) { > switch (intel_encoder->type) { > @@ -5600,8 +5600,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, > case INTEL_OUTPUT_SDVO: > case INTEL_OUTPUT_HDMI: > is_sdvo = true; > - if (intel_encoder->needs_tv_clock) > - is_tv = true; > break; > } > > @@ -5615,7 +5613,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, > dev_priv->lvds_ssc_freq == 100) || > (HAS_PCH_IBX(dev) && intel_is_dual_link_lvds(dev))) > factor = 25; > - } else if (is_sdvo && is_tv) > + } else if (intel_crtc->config.sdvo_tv_clock) > factor = 20; > > if (ironlake_needs_fb_cb_tune(&intel_crtc->config.dpll, factor)) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 766afcf..be196ff 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -120,7 +120,6 @@ struct intel_encoder { > struct intel_crtc *new_crtc; > > int type; > - bool needs_tv_clock; > /* > * Intel hw has only one MUX where encoders could be clone, hence a > * simple flag is enough to compute the possible_clones mask. > @@ -223,6 +222,10 @@ struct intel_crtc_config { > /* Controls for the clock computation, to override various stages. */ > bool clock_set; > > + /* SDVO TV has a bunch of special case. To make multifunction encoders > + * work correctly, we need to track this at runtime.*/ > + bool sdvo_tv_clock; > + > /* > * crtc bandwidth limit, don't increase pipe bpp or clock if not really > * required. This is set in the 2nd loop of calling encoder's > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index f6bf9fc..3a1d710 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -1092,6 +1092,7 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder, > (void) intel_sdvo_get_preferred_input_mode(intel_sdvo, > mode, > adjusted_mode); > + pipe_config->sdvo_tv_clock = true; > } else if (intel_sdvo->is_lvds) { > if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, > intel_sdvo->sdvo_lvds_fixed_mode)) > @@ -1655,12 +1656,9 @@ intel_sdvo_detect(struct drm_connector *connector, bool force) > if (ret == connector_status_connected) { > intel_sdvo->is_tv = false; > intel_sdvo->is_lvds = false; > - intel_sdvo->base.needs_tv_clock = false; > > - if (response & SDVO_TV_MASK) { > + if (response & SDVO_TV_MASK) > intel_sdvo->is_tv = true; > - intel_sdvo->base.needs_tv_clock = true; > - } > if (response & SDVO_LVDS_MASK) > intel_sdvo->is_lvds = intel_sdvo->sdvo_lvds_fixed_mode != NULL; > } > @@ -2357,7 +2355,6 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type) > intel_sdvo_connector->output_flag = type; > > intel_sdvo->is_tv = true; > - intel_sdvo->base.needs_tv_clock = true; > > intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo); > > @@ -2445,7 +2442,6 @@ static bool > intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, uint16_t flags) > { > intel_sdvo->is_tv = false; > - intel_sdvo->base.needs_tv_clock = false; > intel_sdvo->is_lvds = false; > > /* SDVO requires XXX1 function may not exist unless it has XXX0 function.*/ > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 44bcfae..ef0d27b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4546,7 +4546,7 @@ static void i9xx_update_pll(struct intel_crtc *crtc, if (INTEL_INFO(dev)->gen >= 4) dpll |= (6 << PLL_LOAD_PULSE_PHASE_SHIFT); - if (is_sdvo && intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_TVOUT)) + if (crtc->config.sdvo_tv_clock) dpll |= PLL_REF_INPUT_TVCLKINBC; else if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS) && intel_panel_use_ssc(dev_priv) && num_connectors < 2) @@ -5590,7 +5590,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, struct intel_encoder *intel_encoder; uint32_t dpll; int factor, num_connectors = 0; - bool is_lvds = false, is_sdvo = false, is_tv = false; + bool is_lvds = false, is_sdvo = false; for_each_encoder_on_crtc(dev, crtc, intel_encoder) { switch (intel_encoder->type) { @@ -5600,8 +5600,6 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, case INTEL_OUTPUT_SDVO: case INTEL_OUTPUT_HDMI: is_sdvo = true; - if (intel_encoder->needs_tv_clock) - is_tv = true; break; } @@ -5615,7 +5613,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc, dev_priv->lvds_ssc_freq == 100) || (HAS_PCH_IBX(dev) && intel_is_dual_link_lvds(dev))) factor = 25; - } else if (is_sdvo && is_tv) + } else if (intel_crtc->config.sdvo_tv_clock) factor = 20; if (ironlake_needs_fb_cb_tune(&intel_crtc->config.dpll, factor)) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 766afcf..be196ff 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -120,7 +120,6 @@ struct intel_encoder { struct intel_crtc *new_crtc; int type; - bool needs_tv_clock; /* * Intel hw has only one MUX where encoders could be clone, hence a * simple flag is enough to compute the possible_clones mask. @@ -223,6 +222,10 @@ struct intel_crtc_config { /* Controls for the clock computation, to override various stages. */ bool clock_set; + /* SDVO TV has a bunch of special case. To make multifunction encoders + * work correctly, we need to track this at runtime.*/ + bool sdvo_tv_clock; + /* * crtc bandwidth limit, don't increase pipe bpp or clock if not really * required. This is set in the 2nd loop of calling encoder's diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index f6bf9fc..3a1d710 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1092,6 +1092,7 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder, (void) intel_sdvo_get_preferred_input_mode(intel_sdvo, mode, adjusted_mode); + pipe_config->sdvo_tv_clock = true; } else if (intel_sdvo->is_lvds) { if (!intel_sdvo_set_output_timings_from_mode(intel_sdvo, intel_sdvo->sdvo_lvds_fixed_mode)) @@ -1655,12 +1656,9 @@ intel_sdvo_detect(struct drm_connector *connector, bool force) if (ret == connector_status_connected) { intel_sdvo->is_tv = false; intel_sdvo->is_lvds = false; - intel_sdvo->base.needs_tv_clock = false; - if (response & SDVO_TV_MASK) { + if (response & SDVO_TV_MASK) intel_sdvo->is_tv = true; - intel_sdvo->base.needs_tv_clock = true; - } if (response & SDVO_LVDS_MASK) intel_sdvo->is_lvds = intel_sdvo->sdvo_lvds_fixed_mode != NULL; } @@ -2357,7 +2355,6 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type) intel_sdvo_connector->output_flag = type; intel_sdvo->is_tv = true; - intel_sdvo->base.needs_tv_clock = true; intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo); @@ -2445,7 +2442,6 @@ static bool intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, uint16_t flags) { intel_sdvo->is_tv = false; - intel_sdvo->base.needs_tv_clock = false; intel_sdvo->is_lvds = false; /* SDVO requires XXX1 function may not exist unless it has XXX0 function.*/