| Message ID | 1405165643-13189-2-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jul 12, 2014 at 1:47 PM, Shobhit Kumar <shobhit.kumar@intel.com> wrote: > Call to vlv_crtc_clock_get is not needed for DSI and was causing dpio > read WARN dumps as well. Absence of ->get_config was casuing othet WARN > dumps as well. With this the last of the known WARN dumps for DSI should > be fixed. > > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 7 +++--- > drivers/gpu/drm/i915/intel_dsi.c | 45 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_dsi.h | 3 +++ > drivers/gpu/drm/i915/intel_dsi_pll.c | 4 +++- > 4 files changed, 55 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index fe6f1db..3d0ea7c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6309,9 +6309,10 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, > > if (IS_CHERRYVIEW(dev)) > chv_crtc_clock_get(crtc, pipe_config); > - else if (IS_VALLEYVIEW(dev)) > - vlv_crtc_clock_get(crtc, pipe_config); > - else > + else if (IS_VALLEYVIEW(dev)) { > + if (!intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DSI)) > + vlv_crtc_clock_get(crtc, pipe_config); If I understand the logic correctly we don't even enable the DPLL for dsi, i.e. bit31 is clear. So instead of this sw-side check (which is fragile since it depends upon corrected encoder->pipe links in our data structures) we should instead check bit 31 in vlv_crtc_clock_get and just bail out without reading out hw settings. The goal of the hw state cross-check is to check the hw state, so wherever possible we should rely 100% on available information in the hw and not on software tracking. > + } else > i9xx_crtc_clock_get(crtc, pipe_config); > > return true; > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index bfcefbf..61da0e5 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -351,9 +351,54 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, > static void intel_dsi_get_config(struct intel_encoder *encoder, > struct intel_crtc_config *pipe_config) > { > + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > + u32 dsi_clock, pclk; > + u32 pll_ctl, pll_div; > + u32 m = 0, p = 0; > + int refclk = 25000; > + int i; > + > DRM_DEBUG_KMS("\n"); > > /* XXX: read flags, set to adjusted_mode */ > + pipe_config->quirks = 1; Nack. First you need to use one of the symbolic quirk definitions (there's a bunch of them). Second this needs a comment why exactly we need the quirk (which really only should be used if there's no way to read a given piece of state back from the hw). > + > + memset(&pipe_config->dpll_hw_state, 0, > + sizeof(pipe_config->dpll_hw_state)); > + > + mutex_lock(&dev_priv->dpio_lock); > + pll_ctl = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_CONTROL); > + pll_div = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_DIVIDER); > + mutex_unlock(&dev_priv->dpio_lock); > + > + pll_ctl &= ~(DSI_PLL_CLK_GATE_DSI0_DSIPLL | DSI_PLL_VCO_EN); > + pll_ctl = pll_ctl >> (DSI_PLL_P1_POST_DIV_SHIFT - 2); > + > + while (pll_ctl) { > + pll_ctl = pll_ctl >> 1; > + p++; > + } > + p--; > + > + for (i = 0; i < num_lfsr_converts; i++) { > + if (lfsr_converts[i] == pll_div) > + break; > + } > + > + if (i == num_lfsr_converts) { > + DRM_ERROR("wrong m_seed programmed\n"); > + return; > + } > + > + m = i + 62; > + > + dsi_clock = (m * refclk) / p; > + pclk = DIV_ROUND_CLOSEST(dsi_clock * intel_dsi->lane_count, > + pipe_config->pipe_bpp); > + > + pipe_config->adjusted_mode.crtc_clock = pclk; > + pipe_config->port_clock = pclk; > } > > static enum drm_mode_status > diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h > index 31db33d..e0c16b0 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.h > +++ b/drivers/gpu/drm/i915/intel_dsi.h > @@ -130,6 +130,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) > return container_of(encoder, struct intel_dsi, base.base); > } > > +extern const u32 lfsr_converts[]; > +extern const int num_lfsr_converts; > + > extern void vlv_enable_dsi_pll(struct intel_encoder *encoder); > extern void vlv_disable_dsi_pll(struct intel_encoder *encoder); > > diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c > index ba79ec1..78449ea 100644 > --- a/drivers/gpu/drm/i915/intel_dsi_pll.c > +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c > @@ -43,13 +43,15 @@ struct dsi_mnp { > u32 dsi_pll_div; > }; > > -static const u32 lfsr_converts[] = { > +const u32 lfsr_converts[] = { > 426, 469, 234, 373, 442, 221, 110, 311, 411, /* 62 - 70 */ > 461, 486, 243, 377, 188, 350, 175, 343, 427, 213, /* 71 - 80 */ > 106, 53, 282, 397, 354, 227, 113, 56, 284, 142, /* 81 - 90 */ > 71, 35 /* 91 - 92 */ > }; Optional bikeshed: I'd extract the dsi pll read-out code into a helper function so that all the pll code is in this file and we don't need to export internal details. The get_hw_state function in intel_dsi.c would then use that to compute pclk and just store that at the right places in the pipe config. -Daniel > > +const int num_lfsr_converts = sizeof(lfsr_converts) / sizeof(lfsr_converts[0]); > + > #ifdef DSI_CLK_FROM_RR > > static u32 dsi_rr_formula(const struct drm_display_mode *mode, > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 7/12/2014 5:28 PM, Daniel Vetter wrote: > On Sat, Jul 12, 2014 at 1:47 PM, Shobhit Kumar <shobhit.kumar@intel.com> wrote: >> Call to vlv_crtc_clock_get is not needed for DSI and was causing dpio >> read WARN dumps as well. Absence of ->get_config was casuing othet WARN >> dumps as well. With this the last of the known WARN dumps for DSI should >> be fixed. >> >> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 7 +++--- >> drivers/gpu/drm/i915/intel_dsi.c | 45 ++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_dsi.h | 3 +++ >> drivers/gpu/drm/i915/intel_dsi_pll.c | 4 +++- >> 4 files changed, 55 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index fe6f1db..3d0ea7c 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6309,9 +6309,10 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, >> >> if (IS_CHERRYVIEW(dev)) >> chv_crtc_clock_get(crtc, pipe_config); >> - else if (IS_VALLEYVIEW(dev)) >> - vlv_crtc_clock_get(crtc, pipe_config); >> - else >> + else if (IS_VALLEYVIEW(dev)) { >> + if (!intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DSI)) >> + vlv_crtc_clock_get(crtc, pipe_config); > > If I understand the logic correctly we don't even enable the DPLL for > dsi, i.e. bit31 is clear. So instead of this sw-side check (which is > fragile since it depends upon corrected encoder->pipe links in our > data structures) we should instead check bit 31 in vlv_crtc_clock_get > and just bail out without reading out hw settings. Yeah, sounds better will do this. > > The goal of the hw state cross-check is to check the hw state, so > wherever possible we should rely 100% on available information in the > hw and not on software tracking. Got it. > >> + } else >> i9xx_crtc_clock_get(crtc, pipe_config); >> >> return true; >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >> index bfcefbf..61da0e5 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> @@ -351,9 +351,54 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, >> static void intel_dsi_get_config(struct intel_encoder *encoder, >> struct intel_crtc_config *pipe_config) >> { >> + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; >> + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >> + u32 dsi_clock, pclk; >> + u32 pll_ctl, pll_div; >> + u32 m = 0, p = 0; >> + int refclk = 25000; >> + int i; >> + >> DRM_DEBUG_KMS("\n"); >> >> /* XXX: read flags, set to adjusted_mode */ >> + pipe_config->quirks = 1; > > Nack. First you need to use one of the symbolic quirk definitions > (there's a bunch of them). Second this needs a comment why exactly we > need the quirk (which really only should be used if there's no way to > read a given piece of state back from the hw). Okay, in MIPI we have sync events going as short packets. In that case I think it should be okay to use PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS ? > >> + >> + memset(&pipe_config->dpll_hw_state, 0, >> + sizeof(pipe_config->dpll_hw_state)); >> + >> + mutex_lock(&dev_priv->dpio_lock); >> + pll_ctl = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_CONTROL); >> + pll_div = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_DIVIDER); >> + mutex_unlock(&dev_priv->dpio_lock); >> + >> + pll_ctl &= ~(DSI_PLL_CLK_GATE_DSI0_DSIPLL | DSI_PLL_VCO_EN); >> + pll_ctl = pll_ctl >> (DSI_PLL_P1_POST_DIV_SHIFT - 2); >> + >> + while (pll_ctl) { >> + pll_ctl = pll_ctl >> 1; >> + p++; >> + } >> + p--; >> + >> + for (i = 0; i < num_lfsr_converts; i++) { >> + if (lfsr_converts[i] == pll_div) >> + break; >> + } >> + >> + if (i == num_lfsr_converts) { >> + DRM_ERROR("wrong m_seed programmed\n"); >> + return; >> + } >> + >> + m = i + 62; >> + >> + dsi_clock = (m * refclk) / p; >> + pclk = DIV_ROUND_CLOSEST(dsi_clock * intel_dsi->lane_count, >> + pipe_config->pipe_bpp); >> + >> + pipe_config->adjusted_mode.crtc_clock = pclk; >> + pipe_config->port_clock = pclk; >> } >> >> static enum drm_mode_status >> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h >> index 31db33d..e0c16b0 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.h >> +++ b/drivers/gpu/drm/i915/intel_dsi.h >> @@ -130,6 +130,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) >> return container_of(encoder, struct intel_dsi, base.base); >> } >> >> +extern const u32 lfsr_converts[]; >> +extern const int num_lfsr_converts; >> + >> extern void vlv_enable_dsi_pll(struct intel_encoder *encoder); >> extern void vlv_disable_dsi_pll(struct intel_encoder *encoder); >> >> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c >> index ba79ec1..78449ea 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c >> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c >> @@ -43,13 +43,15 @@ struct dsi_mnp { >> u32 dsi_pll_div; >> }; >> >> -static const u32 lfsr_converts[] = { >> +const u32 lfsr_converts[] = { >> 426, 469, 234, 373, 442, 221, 110, 311, 411, /* 62 - 70 */ >> 461, 486, 243, 377, 188, 350, 175, 343, 427, 213, /* 71 - 80 */ >> 106, 53, 282, 397, 354, 227, 113, 56, 284, 142, /* 81 - 90 */ >> 71, 35 /* 91 - 92 */ >> }; > > Optional bikeshed: I'd extract the dsi pll read-out code into a helper > function so that all the pll code is in this file and we don't need to > export internal details. The get_hw_state function in intel_dsi.c > would then use that to compute pclk and just store that at the right > places in the pipe config. Yeah will correct this as well. Regards Shobhit
On Mon, Jul 14, 2014 at 4:36 PM, Kumar, Shobhit <shobhit.kumar@intel.com> wrote: >>> /* XXX: read flags, set to adjusted_mode */ >>> + pipe_config->quirks = 1; >> >> >> Nack. First you need to use one of the symbolic quirk definitions >> (there's a bunch of them). Second this needs a comment why exactly we >> need the quirk (which really only should be used if there's no way to >> read a given piece of state back from the hw). > > > Okay, in MIPI we have sync events going as short packets. In that case I > think it should be okay to use PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS ? Well it depends. From a quick look it seems like the current dsi code doesn't care at all about sync flags. In that case you should normalize the sync flags of the adjusted mode in the compute_config callback to 0 and not set them in the get_hw_state function. We do that already for e.g. tv encoder outputs. The quirk flag should only be used if we do set the sync modes but somehow can't read it back. The only case is sdvo where some encoders (in violation of the spec) don't support the flag readback. But that case needs a big comment explaining why. The goal here isn't to shut up the hw cross checker but to actually make it useful ;-) -Daniel
On 7/14/2014 9:20 PM, Daniel Vetter wrote: > On Mon, Jul 14, 2014 at 4:36 PM, Kumar, Shobhit <shobhit.kumar@intel.com> wrote: >>>> /* XXX: read flags, set to adjusted_mode */ >>>> + pipe_config->quirks = 1; >>> >>> >>> Nack. First you need to use one of the symbolic quirk definitions >>> (there's a bunch of them). Second this needs a comment why exactly we >>> need the quirk (which really only should be used if there's no way to >>> read a given piece of state back from the hw). >> >> >> Okay, in MIPI we have sync events going as short packets. In that case I >> think it should be okay to use PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS ? > > Well it depends. From a quick look it seems like the current dsi code > doesn't care at all about sync flags. In that case you should > normalize the sync flags of the adjusted mode in the compute_config > callback to 0 and not set them in the get_hw_state function. We do > that already for e.g. tv encoder outputs. I just assumed that as we don't care about sync flags just suppress their check :) Thanks for pointing correct way of doing this. I will send the corrected patch > > The quirk flag should only be used if we do set the sync modes but > somehow can't read it back. The only case is sdvo where some encoders > (in violation of the spec) don't support the flag readback. But that > case needs a big comment explaining why. > > The goal here isn't to shut up the hw cross checker but to actually > make it useful ;-) Of-course. Thanks for clarifying. Regards Shobhit
On Sat, 2014-07-12 at 17:17 +0530, Shobhit Kumar wrote: > Call to vlv_crtc_clock_get is not needed for DSI and was causing dpio > read WARN dumps as well. Absence of ->get_config was casuing othet WARN > dumps as well. With this the last of the known WARN dumps for DSI should > be fixed. > > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 7 +++--- > drivers/gpu/drm/i915/intel_dsi.c | 45 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_dsi.h | 3 +++ > drivers/gpu/drm/i915/intel_dsi_pll.c | 4 +++- > 4 files changed, 55 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index fe6f1db..3d0ea7c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6309,9 +6309,10 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, > > if (IS_CHERRYVIEW(dev)) > chv_crtc_clock_get(crtc, pipe_config); > - else if (IS_VALLEYVIEW(dev)) > - vlv_crtc_clock_get(crtc, pipe_config); > - else > + else if (IS_VALLEYVIEW(dev)) { > + if (!intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DSI)) > + vlv_crtc_clock_get(crtc, pipe_config); > + } else > i9xx_crtc_clock_get(crtc, pipe_config); > > return true; > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index bfcefbf..61da0e5 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -351,9 +351,54 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, > static void intel_dsi_get_config(struct intel_encoder *encoder, > struct intel_crtc_config *pipe_config) > { > + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > + u32 dsi_clock, pclk; > + u32 pll_ctl, pll_div; > + u32 m = 0, p = 0; > + int refclk = 25000; > + int i; > + > DRM_DEBUG_KMS("\n"); > > /* XXX: read flags, set to adjusted_mode */ This comment can be removed. > + pipe_config->quirks = 1; The proper macro should be used. > + > + memset(&pipe_config->dpll_hw_state, 0, > + sizeof(pipe_config->dpll_hw_state)); > + > + mutex_lock(&dev_priv->dpio_lock); > + pll_ctl = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_CONTROL); > + pll_div = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_DIVIDER); > + mutex_unlock(&dev_priv->dpio_lock); > + > + pll_ctl &= ~(DSI_PLL_CLK_GATE_DSI0_DSIPLL | DSI_PLL_VCO_EN); > + pll_ctl = pll_ctl >> (DSI_PLL_P1_POST_DIV_SHIFT - 2); I'd prefer a proper masking of the P1 field instead of depending on other register fields being 0. The same goes for pll_div[M1]. > + > + while (pll_ctl) { > + pll_ctl = pll_ctl >> 1; > + p++; > + } > + p--; > + > + for (i = 0; i < num_lfsr_converts; i++) { > + if (lfsr_converts[i] == pll_div) > + break; > + } > + > + if (i == num_lfsr_converts) { > + DRM_ERROR("wrong m_seed programmed\n"); > + return; > + } > + > + m = i + 62; > + > + dsi_clock = (m * refclk) / p; Should guard against div-by-zero. > + pclk = DIV_ROUND_CLOSEST(dsi_clock * intel_dsi->lane_count, > + pipe_config->pipe_bpp); dsi_clk_from_pclk() uses dsi->pixel_format in place of pipe_bpp, so an assert here that the two values agree would be nice. > + > + pipe_config->adjusted_mode.crtc_clock = pclk; > + pipe_config->port_clock = pclk; > } > > static enum drm_mode_status > diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h > index 31db33d..e0c16b0 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.h > +++ b/drivers/gpu/drm/i915/intel_dsi.h > @@ -130,6 +130,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) > return container_of(encoder, struct intel_dsi, base.base); > } > > +extern const u32 lfsr_converts[]; > +extern const int num_lfsr_converts; > + > extern void vlv_enable_dsi_pll(struct intel_encoder *encoder); > extern void vlv_disable_dsi_pll(struct intel_encoder *encoder); > > diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c > index ba79ec1..78449ea 100644 > --- a/drivers/gpu/drm/i915/intel_dsi_pll.c > +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c > @@ -43,13 +43,15 @@ struct dsi_mnp { > u32 dsi_pll_div; > }; > > -static const u32 lfsr_converts[] = { > +const u32 lfsr_converts[] = { > 426, 469, 234, 373, 442, 221, 110, 311, 411, /* 62 - 70 */ > 461, 486, 243, 377, 188, 350, 175, 343, 427, 213, /* 71 - 80 */ > 106, 53, 282, 397, 354, 227, 113, 56, 284, 142, /* 81 - 90 */ > 71, 35 /* 91 - 92 */ > }; > > +const int num_lfsr_converts = sizeof(lfsr_converts) / sizeof(lfsr_converts[0]); You can use ARRAY_SIZE here. > + > #ifdef DSI_CLK_FROM_RR > > static u32 dsi_rr_formula(const struct drm_display_mode *mode,
On Tue, Jul 29, 2014 at 02:38:36PM +0300, Imre Deak wrote: > On Sat, 2014-07-12 at 17:17 +0530, Shobhit Kumar wrote: > > Call to vlv_crtc_clock_get is not needed for DSI and was causing dpio > > read WARN dumps as well. Absence of ->get_config was casuing othet WARN > > dumps as well. With this the last of the known WARN dumps for DSI should > > be fixed. > > > > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 7 +++--- > > drivers/gpu/drm/i915/intel_dsi.c | 45 ++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_dsi.h | 3 +++ > > drivers/gpu/drm/i915/intel_dsi_pll.c | 4 +++- > > 4 files changed, 55 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index fe6f1db..3d0ea7c 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -6309,9 +6309,10 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, > > > > if (IS_CHERRYVIEW(dev)) > > chv_crtc_clock_get(crtc, pipe_config); > > - else if (IS_VALLEYVIEW(dev)) > > - vlv_crtc_clock_get(crtc, pipe_config); > > - else > > + else if (IS_VALLEYVIEW(dev)) { > > + if (!intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DSI)) > > + vlv_crtc_clock_get(crtc, pipe_config); > > + } else > > i9xx_crtc_clock_get(crtc, pipe_config); > > > > return true; > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > > index bfcefbf..61da0e5 100644 > > --- a/drivers/gpu/drm/i915/intel_dsi.c > > +++ b/drivers/gpu/drm/i915/intel_dsi.c > > @@ -351,9 +351,54 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, > > static void intel_dsi_get_config(struct intel_encoder *encoder, > > struct intel_crtc_config *pipe_config) > > { > > + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > > + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > > + u32 dsi_clock, pclk; > > + u32 pll_ctl, pll_div; > > + u32 m = 0, p = 0; > > + int refclk = 25000; > > + int i; > > + > > DRM_DEBUG_KMS("\n"); > > > > /* XXX: read flags, set to adjusted_mode */ > > This comment can be removed. There's a v2 with a lot of this addressed already. -Daniel > > > + pipe_config->quirks = 1; > > The proper macro should be used. > > > + > > + memset(&pipe_config->dpll_hw_state, 0, > > + sizeof(pipe_config->dpll_hw_state)); > > + > > + mutex_lock(&dev_priv->dpio_lock); > > + pll_ctl = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_CONTROL); > > + pll_div = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_DIVIDER); > > + mutex_unlock(&dev_priv->dpio_lock); > > + > > + pll_ctl &= ~(DSI_PLL_CLK_GATE_DSI0_DSIPLL | DSI_PLL_VCO_EN); > > + pll_ctl = pll_ctl >> (DSI_PLL_P1_POST_DIV_SHIFT - 2); > > I'd prefer a proper masking of the P1 field instead of depending on > other register fields being 0. The same goes for pll_div[M1]. > > > + > > + while (pll_ctl) { > > + pll_ctl = pll_ctl >> 1; > > + p++; > > + } > > + p--; > > + > > + for (i = 0; i < num_lfsr_converts; i++) { > > + if (lfsr_converts[i] == pll_div) > > + break; > > + } > > + > > + if (i == num_lfsr_converts) { > > + DRM_ERROR("wrong m_seed programmed\n"); > > + return; > > + } > > + > > + m = i + 62; > > + > > + dsi_clock = (m * refclk) / p; > > Should guard against div-by-zero. > > > + pclk = DIV_ROUND_CLOSEST(dsi_clock * intel_dsi->lane_count, > > + pipe_config->pipe_bpp); > > dsi_clk_from_pclk() uses dsi->pixel_format in place of pipe_bpp, so an > assert here that the two values agree would be nice. > > > + > > + pipe_config->adjusted_mode.crtc_clock = pclk; > > + pipe_config->port_clock = pclk; > > } > > > > static enum drm_mode_status > > diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h > > index 31db33d..e0c16b0 100644 > > --- a/drivers/gpu/drm/i915/intel_dsi.h > > +++ b/drivers/gpu/drm/i915/intel_dsi.h > > @@ -130,6 +130,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) > > return container_of(encoder, struct intel_dsi, base.base); > > } > > > > +extern const u32 lfsr_converts[]; > > +extern const int num_lfsr_converts; > > + > > extern void vlv_enable_dsi_pll(struct intel_encoder *encoder); > > extern void vlv_disable_dsi_pll(struct intel_encoder *encoder); > > > > diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c > > index ba79ec1..78449ea 100644 > > --- a/drivers/gpu/drm/i915/intel_dsi_pll.c > > +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c > > @@ -43,13 +43,15 @@ struct dsi_mnp { > > u32 dsi_pll_div; > > }; > > > > -static const u32 lfsr_converts[] = { > > +const u32 lfsr_converts[] = { > > 426, 469, 234, 373, 442, 221, 110, 311, 411, /* 62 - 70 */ > > 461, 486, 243, 377, 188, 350, 175, 343, 427, 213, /* 71 - 80 */ > > 106, 53, 282, 397, 354, 227, 113, 56, 284, 142, /* 81 - 90 */ > > 71, 35 /* 91 - 92 */ > > }; > > > > +const int num_lfsr_converts = sizeof(lfsr_converts) / sizeof(lfsr_converts[0]); > > You can use ARRAY_SIZE here. > > > + > > #ifdef DSI_CLK_FROM_RR > > > > static u32 dsi_rr_formula(const struct drm_display_mode *mode, > > _______________________________________________ > 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 fe6f1db..3d0ea7c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6309,9 +6309,10 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, if (IS_CHERRYVIEW(dev)) chv_crtc_clock_get(crtc, pipe_config); - else if (IS_VALLEYVIEW(dev)) - vlv_crtc_clock_get(crtc, pipe_config); - else + else if (IS_VALLEYVIEW(dev)) { + if (!intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DSI)) + vlv_crtc_clock_get(crtc, pipe_config); + } else i9xx_crtc_clock_get(crtc, pipe_config); return true; diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index bfcefbf..61da0e5 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -351,9 +351,54 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, static void intel_dsi_get_config(struct intel_encoder *encoder, struct intel_crtc_config *pipe_config) { + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); + u32 dsi_clock, pclk; + u32 pll_ctl, pll_div; + u32 m = 0, p = 0; + int refclk = 25000; + int i; + DRM_DEBUG_KMS("\n"); /* XXX: read flags, set to adjusted_mode */ + pipe_config->quirks = 1; + + memset(&pipe_config->dpll_hw_state, 0, + sizeof(pipe_config->dpll_hw_state)); + + mutex_lock(&dev_priv->dpio_lock); + pll_ctl = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_CONTROL); + pll_div = vlv_cck_read(dev_priv, CCK_REG_DSI_PLL_DIVIDER); + mutex_unlock(&dev_priv->dpio_lock); + + pll_ctl &= ~(DSI_PLL_CLK_GATE_DSI0_DSIPLL | DSI_PLL_VCO_EN); + pll_ctl = pll_ctl >> (DSI_PLL_P1_POST_DIV_SHIFT - 2); + + while (pll_ctl) { + pll_ctl = pll_ctl >> 1; + p++; + } + p--; + + for (i = 0; i < num_lfsr_converts; i++) { + if (lfsr_converts[i] == pll_div) + break; + } + + if (i == num_lfsr_converts) { + DRM_ERROR("wrong m_seed programmed\n"); + return; + } + + m = i + 62; + + dsi_clock = (m * refclk) / p; + pclk = DIV_ROUND_CLOSEST(dsi_clock * intel_dsi->lane_count, + pipe_config->pipe_bpp); + + pipe_config->adjusted_mode.crtc_clock = pclk; + pipe_config->port_clock = pclk; } static enum drm_mode_status diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index 31db33d..e0c16b0 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -130,6 +130,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) return container_of(encoder, struct intel_dsi, base.base); } +extern const u32 lfsr_converts[]; +extern const int num_lfsr_converts; + extern void vlv_enable_dsi_pll(struct intel_encoder *encoder); extern void vlv_disable_dsi_pll(struct intel_encoder *encoder); diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c index ba79ec1..78449ea 100644 --- a/drivers/gpu/drm/i915/intel_dsi_pll.c +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c @@ -43,13 +43,15 @@ struct dsi_mnp { u32 dsi_pll_div; }; -static const u32 lfsr_converts[] = { +const u32 lfsr_converts[] = { 426, 469, 234, 373, 442, 221, 110, 311, 411, /* 62 - 70 */ 461, 486, 243, 377, 188, 350, 175, 343, 427, 213, /* 71 - 80 */ 106, 53, 282, 397, 354, 227, 113, 56, 284, 142, /* 81 - 90 */ 71, 35 /* 91 - 92 */ }; +const int num_lfsr_converts = sizeof(lfsr_converts) / sizeof(lfsr_converts[0]); + #ifdef DSI_CLK_FROM_RR static u32 dsi_rr_formula(const struct drm_display_mode *mode,
Call to vlv_crtc_clock_get is not needed for DSI and was causing dpio read WARN dumps as well. Absence of ->get_config was casuing othet WARN dumps as well. With this the last of the known WARN dumps for DSI should be fixed. Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 7 +++--- drivers/gpu/drm/i915/intel_dsi.c | 45 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_dsi.h | 3 +++ drivers/gpu/drm/i915/intel_dsi_pll.c | 4 +++- 4 files changed, 55 insertions(+), 4 deletions(-)