| Message ID | 1351024208-3489-9-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 23, 2012 at 06:29:58PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > We need to check if any of the pipes is using TRANSCODER_EDP. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> One thing that irks me is that you add new state readout code here, but don't call the same code in the modeset_check_state code. Now the design behind all these ->get_hw_state functions is very much that we use the exact same code to check the modeset state as we do for fastboot. For otherwise we simply won't get enough testcoverage for the fastboot code - but if we also use it to check all our own state, we should be able to take over at least all the crazy configs our driver leaves behind. I'm ok with merging this as-is, but I'd like you to fix this up in a follow-up series. As a rule of thumb the exact same hw state readout code should be used in setup_hw_state (for fastboot) as for the modeset state checks. Might be that we need to add a get_crtc_hw_state function and a intel_crtc_hw_state struct which contains all the interesting bits (which transcoder, pch resources, eventually modes and shared plls, ...) that we could either store in the intel_crtc (for setup_hw_state) or compare with the stored state in intel_crtc. Also, modeset_check_state should grow cross checks imo to ensure that we don't wire up these cpu transcoders in a stupid way (or leave an unused transcoder enabled). Cheers, Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index d63da7b..2f546e8 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8690,6 +8690,31 @@ void intel_modeset_setup_hw_state(struct drm_device *dev) > struct intel_encoder *encoder; > struct intel_connector *connector; > > + if (IS_HASWELL(dev)) { > + tmp = I915_READ(DDI_FUNC_CTL(TRANSCODER_EDP)); > + > + if (tmp & TRANS_DDI_FUNC_ENABLE) { > + switch (tmp & TRANS_DDI_EDP_INPUT_MASK) { > + case TRANS_DDI_EDP_INPUT_A_ON: > + case TRANS_DDI_EDP_INPUT_A_ONOFF: > + pipe = PIPE_A; > + break; > + case TRANS_DDI_EDP_INPUT_B_ONOFF: > + pipe = PIPE_B; > + break; > + case TRANS_DDI_EDP_INPUT_C_ONOFF: > + pipe = PIPE_C; > + break; > + } > + > + crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > + crtc->cpu_transcoder = TRANSCODER_EDP; > + > + DRM_DEBUG_KMS("Pipe %c using transcoder EDP\n", > + pipe_name(pipe)); > + } > + } > + > for_each_pipe(pipe) { > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > > -- > 1.7.11.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 24, 2012 at 1:38 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Oct 23, 2012 at 06:29:58PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> We need to check if any of the pipes is using TRANSCODER_EDP. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > One thing that irks me is that you add new state readout code here, but > don't call the same code in the modeset_check_state code Isn't what the previous patch introduce? (look at the intel_ddi_get_hw_state() hunk in the patch 07/18 of the series). AFAICS we're are already checking if the pipe returned by the encoder is what we think it is.
On Wed, Oct 24, 2012 at 04:45:04PM +0100, Lespiau, Damien wrote: > On Wed, Oct 24, 2012 at 1:38 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Oct 23, 2012 at 06:29:58PM -0200, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> We need to check if any of the pipes is using TRANSCODER_EDP. > >> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > One thing that irks me is that you add new state readout code here, but > > don't call the same code in the modeset_check_state code > > Isn't what the previous patch introduce? (look at the > intel_ddi_get_hw_state() hunk in the patch 07/18 of the series). > AFAICS we're are already checking if the pipe returned by the encoder > is what we think it is. In a way, yes. But in the best case we should have the same code in both places (now it's copied), and also should be able to check which parts of the hw we actually need (since the get_hw_state now hides the cpu_transcoder in between the ddi and the pipe that's not so easy to do). So I still think we should try to unify/improve this a bit, especially since for the other fastboo stuff we need more cleverness for pipe state anyway (otherwise handling plls&friends will be a pain). -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d63da7b..2f546e8 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8690,6 +8690,31 @@ void intel_modeset_setup_hw_state(struct drm_device *dev) struct intel_encoder *encoder; struct intel_connector *connector; + if (IS_HASWELL(dev)) { + tmp = I915_READ(DDI_FUNC_CTL(TRANSCODER_EDP)); + + if (tmp & TRANS_DDI_FUNC_ENABLE) { + switch (tmp & TRANS_DDI_EDP_INPUT_MASK) { + case TRANS_DDI_EDP_INPUT_A_ON: + case TRANS_DDI_EDP_INPUT_A_ONOFF: + pipe = PIPE_A; + break; + case TRANS_DDI_EDP_INPUT_B_ONOFF: + pipe = PIPE_B; + break; + case TRANS_DDI_EDP_INPUT_C_ONOFF: + pipe = PIPE_C; + break; + } + + crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); + crtc->cpu_transcoder = TRANSCODER_EDP; + + DRM_DEBUG_KMS("Pipe %c using transcoder EDP\n", + pipe_name(pipe)); + } + } + for_each_pipe(pipe) { crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);