| Message ID | 20181204101926.17174-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/icl: fix transcoder state readout | expand |
On Tue, Dec 04, 2018 at 12:19:26PM +0200, Jani Nikula wrote: > Commit 2ca711caeca2 ("drm/i915/icl: Consider DSI for getting transcoder > state") clobbers the previously read TRANS_DDI_FUNC_CTL_EDP register > contents with TRANS_DDI_FUNC_CTL_DSI0 contents. Fix the state readout, > and handle DSI 1 while at it. > > Use a bitmask for iterating and logging transcoders, because the allowed > combinations are a bit funky. > > Fixes: 2ca711caeca2 ("drm/i915/icl: Consider DSI for getting transcoder state") Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108928 > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Madhav Chauhan <madhav.chauhan@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++++++++++++---------------- > 1 file changed, 27 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a2584f977ab1..64b0ecd538fd 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9476,13 +9476,18 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc, > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > enum intel_display_power_domain power_domain; > + unsigned long panel_transcoder_mask = BIT(TRANSCODER_EDP); > + unsigned long enabled_panel_transcoders = 0; > + enum transcoder panel_transcoder; > u32 tmp; > - bool is_dsi = false; > - bool is_edp = false; > + > + if (IS_ICELAKE(dev_priv)) > + panel_transcoder_mask |= > + BIT(TRANSCODER_DSI_0) | BIT(TRANSCODER_DSI_1); > > /* > * The pipe->transcoder mapping is fixed with the exception of the eDP > - * transcoder handled below. > + * and DSI transcoders handled below. > */ > pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe; > > @@ -9490,23 +9495,22 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc, > * XXX: Do intel_display_power_get_if_enabled before reading this (for > * consistency and less surprising code; it's in always on power). > */ > - tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP)); > - if (tmp & TRANS_DDI_FUNC_ENABLE) > - is_edp = true; > + for_each_set_bit(panel_transcoder, &panel_transcoder_mask, 32) { nit: BITS_PER_LONG or I915_MAX_TRANSCODERS? > + enum pipe trans_pipe; > > - if (IS_ICELAKE(dev_priv)) { > - tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_DSI_0)); > - if (tmp & TRANS_DDI_FUNC_ENABLE) > - is_dsi = true; > - } > + tmp = I915_READ(TRANS_DDI_FUNC_CTL(panel_transcoder)); > + if (!(tmp & TRANS_DDI_FUNC_ENABLE)) > + continue; > > - WARN_ON(is_edp && is_dsi); > + /* Log all enabled ones, only use the first one */ > + enabled_panel_transcoders |= BIT(panel_transcoder); > + if (enabled_panel_transcoders != BIT(panel_transcoder)) > + continue; nit: As you explained for DSI dual mode both transcoders are needed and we don't support atm two separate DSI ports. A code comment about this would be nice. Maybe for the two DSI port case we should also warn below. With or without the nits: Reviewed-by: Imre Deak <imre.deak@intel.com> > > - if (is_edp || is_dsi) { > - enum pipe trans_pipe; > switch (tmp & TRANS_DDI_EDP_INPUT_MASK) { > default: > - WARN(1, "unknown pipe linked to edp transcoder\n"); > + WARN(1, "unknown pipe linked to transcoder %s\n", > + transcoder_name(panel_transcoder)); > /* fall through */ > case TRANS_DDI_EDP_INPUT_A_ONOFF: > case TRANS_DDI_EDP_INPUT_A_ON: > @@ -9520,14 +9524,16 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc, > break; > } > > - if (trans_pipe == crtc->pipe) { > - if (is_edp) > - pipe_config->cpu_transcoder = TRANSCODER_EDP; > - else if (is_dsi) > - pipe_config->cpu_transcoder = TRANSCODER_DSI_0; > - } > + if (trans_pipe == crtc->pipe) > + pipe_config->cpu_transcoder = panel_transcoder; > } > > + /* > + * Valid combos: none, eDP, DSI0, DSI1, DSI0+DSI1 > + */ > + WARN_ON((enabled_panel_transcoders & BIT(TRANSCODER_EDP)) && > + enabled_panel_transcoders != BIT(TRANSCODER_EDP)); > + > power_domain = POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder); > if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) > return false; > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 04 Dec 2018, Imre Deak <imre.deak@intel.com> wrote: > On Tue, Dec 04, 2018 at 12:19:26PM +0200, Jani Nikula wrote: >> Commit 2ca711caeca2 ("drm/i915/icl: Consider DSI for getting transcoder >> state") clobbers the previously read TRANS_DDI_FUNC_CTL_EDP register >> contents with TRANS_DDI_FUNC_CTL_DSI0 contents. Fix the state readout, >> and handle DSI 1 while at it. >> >> Use a bitmask for iterating and logging transcoders, because the allowed >> combinations are a bit funky. >> >> Fixes: 2ca711caeca2 ("drm/i915/icl: Consider DSI for getting transcoder state") > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108928 > >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Madhav Chauhan <madhav.chauhan@intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++++++++++++---------------- >> 1 file changed, 27 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index a2584f977ab1..64b0ecd538fd 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -9476,13 +9476,18 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc, >> struct drm_device *dev = crtc->base.dev; >> struct drm_i915_private *dev_priv = to_i915(dev); >> enum intel_display_power_domain power_domain; >> + unsigned long panel_transcoder_mask = BIT(TRANSCODER_EDP); >> + unsigned long enabled_panel_transcoders = 0; >> + enum transcoder panel_transcoder; >> u32 tmp; >> - bool is_dsi = false; >> - bool is_edp = false; >> + >> + if (IS_ICELAKE(dev_priv)) >> + panel_transcoder_mask |= >> + BIT(TRANSCODER_DSI_0) | BIT(TRANSCODER_DSI_1); >> >> /* >> * The pipe->transcoder mapping is fixed with the exception of the eDP >> - * transcoder handled below. >> + * and DSI transcoders handled below. >> */ >> pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe; >> >> @@ -9490,23 +9495,22 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc, >> * XXX: Do intel_display_power_get_if_enabled before reading this (for >> * consistency and less surprising code; it's in always on power). >> */ >> - tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP)); >> - if (tmp & TRANS_DDI_FUNC_ENABLE) >> - is_edp = true; >> + for_each_set_bit(panel_transcoder, &panel_transcoder_mask, 32) { > > nit: BITS_PER_LONG or I915_MAX_TRANSCODERS? > >> + enum pipe trans_pipe; >> >> - if (IS_ICELAKE(dev_priv)) { >> - tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_DSI_0)); >> - if (tmp & TRANS_DDI_FUNC_ENABLE) >> - is_dsi = true; >> - } >> + tmp = I915_READ(TRANS_DDI_FUNC_CTL(panel_transcoder)); >> + if (!(tmp & TRANS_DDI_FUNC_ENABLE)) >> + continue; >> >> - WARN_ON(is_edp && is_dsi); >> + /* Log all enabled ones, only use the first one */ >> + enabled_panel_transcoders |= BIT(panel_transcoder); >> + if (enabled_panel_transcoders != BIT(panel_transcoder)) >> + continue; > > > nit: As you explained for DSI dual mode both transcoders are needed and > we don't support atm two separate DSI ports. A code comment about this > would be nice. Maybe for the two DSI port case we should also warn below. > > With or without the nits: > Reviewed-by: Imre Deak <imre.deak@intel.com> Thanks, pushed with the comment added. BR, Jani. > >> >> - if (is_edp || is_dsi) { >> - enum pipe trans_pipe; >> switch (tmp & TRANS_DDI_EDP_INPUT_MASK) { >> default: >> - WARN(1, "unknown pipe linked to edp transcoder\n"); >> + WARN(1, "unknown pipe linked to transcoder %s\n", >> + transcoder_name(panel_transcoder)); >> /* fall through */ >> case TRANS_DDI_EDP_INPUT_A_ONOFF: >> case TRANS_DDI_EDP_INPUT_A_ON: >> @@ -9520,14 +9524,16 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc, >> break; >> } >> >> - if (trans_pipe == crtc->pipe) { >> - if (is_edp) >> - pipe_config->cpu_transcoder = TRANSCODER_EDP; >> - else if (is_dsi) >> - pipe_config->cpu_transcoder = TRANSCODER_DSI_0; >> - } >> + if (trans_pipe == crtc->pipe) >> + pipe_config->cpu_transcoder = panel_transcoder; >> } >> >> + /* >> + * Valid combos: none, eDP, DSI0, DSI1, DSI0+DSI1 >> + */ >> + WARN_ON((enabled_panel_transcoders & BIT(TRANSCODER_EDP)) && >> + enabled_panel_transcoders != BIT(TRANSCODER_EDP)); >> + >> power_domain = POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder); >> if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) >> return false; >> -- >> 2.11.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://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 a2584f977ab1..64b0ecd538fd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9476,13 +9476,18 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc, struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = to_i915(dev); enum intel_display_power_domain power_domain; + unsigned long panel_transcoder_mask = BIT(TRANSCODER_EDP); + unsigned long enabled_panel_transcoders = 0; + enum transcoder panel_transcoder; u32 tmp; - bool is_dsi = false; - bool is_edp = false; + + if (IS_ICELAKE(dev_priv)) + panel_transcoder_mask |= + BIT(TRANSCODER_DSI_0) | BIT(TRANSCODER_DSI_1); /* * The pipe->transcoder mapping is fixed with the exception of the eDP - * transcoder handled below. + * and DSI transcoders handled below. */ pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe; @@ -9490,23 +9495,22 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc, * XXX: Do intel_display_power_get_if_enabled before reading this (for * consistency and less surprising code; it's in always on power). */ - tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP)); - if (tmp & TRANS_DDI_FUNC_ENABLE) - is_edp = true; + for_each_set_bit(panel_transcoder, &panel_transcoder_mask, 32) { + enum pipe trans_pipe; - if (IS_ICELAKE(dev_priv)) { - tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_DSI_0)); - if (tmp & TRANS_DDI_FUNC_ENABLE) - is_dsi = true; - } + tmp = I915_READ(TRANS_DDI_FUNC_CTL(panel_transcoder)); + if (!(tmp & TRANS_DDI_FUNC_ENABLE)) + continue; - WARN_ON(is_edp && is_dsi); + /* Log all enabled ones, only use the first one */ + enabled_panel_transcoders |= BIT(panel_transcoder); + if (enabled_panel_transcoders != BIT(panel_transcoder)) + continue; - if (is_edp || is_dsi) { - enum pipe trans_pipe; switch (tmp & TRANS_DDI_EDP_INPUT_MASK) { default: - WARN(1, "unknown pipe linked to edp transcoder\n"); + WARN(1, "unknown pipe linked to transcoder %s\n", + transcoder_name(panel_transcoder)); /* fall through */ case TRANS_DDI_EDP_INPUT_A_ONOFF: case TRANS_DDI_EDP_INPUT_A_ON: @@ -9520,14 +9524,16 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc, break; } - if (trans_pipe == crtc->pipe) { - if (is_edp) - pipe_config->cpu_transcoder = TRANSCODER_EDP; - else if (is_dsi) - pipe_config->cpu_transcoder = TRANSCODER_DSI_0; - } + if (trans_pipe == crtc->pipe) + pipe_config->cpu_transcoder = panel_transcoder; } + /* + * Valid combos: none, eDP, DSI0, DSI1, DSI0+DSI1 + */ + WARN_ON((enabled_panel_transcoders & BIT(TRANSCODER_EDP)) && + enabled_panel_transcoders != BIT(TRANSCODER_EDP)); + power_domain = POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder); if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false;
Commit 2ca711caeca2 ("drm/i915/icl: Consider DSI for getting transcoder state") clobbers the previously read TRANS_DDI_FUNC_CTL_EDP register contents with TRANS_DDI_FUNC_CTL_DSI0 contents. Fix the state readout, and handle DSI 1 while at it. Use a bitmask for iterating and logging transcoders, because the allowed combinations are a bit funky. Fixes: 2ca711caeca2 ("drm/i915/icl: Consider DSI for getting transcoder state") Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Madhav Chauhan <madhav.chauhan@intel.com> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 21 deletions(-)