| Message ID | 1351024208-3489-6-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 23, 2012 at 9:29 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > static void haswell_crtc_off(struct drm_crtc *crtc) > { > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + > + intel_crtc->cpu_transcoder = intel_crtc->pipe; > intel_ddi_put_crtc_pll(crtc); > } > I can't find the reason why you would set the cpu_transcoder in the off() function, would you mind explaining why? (or maybe the clue is in a later patch, which might mean this hunk belongs to a later patch as well).
Hi 2012/10/24 Lespiau, Damien <damien.lespiau@intel.com>: > On Tue, Oct 23, 2012 at 9:29 PM, Paulo Zanoni <przanoni@gmail.com> wrote: >> static void haswell_crtc_off(struct drm_crtc *crtc) >> { >> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> + >> + intel_crtc->cpu_transcoder = intel_crtc->pipe; >> intel_ddi_put_crtc_pll(crtc); >> } >> > > I can't find the reason why you would set the cpu_transcoder in the > off() function, would you mind explaining why? (or maybe the clue is > in a later patch, which might mean this hunk belongs to a later patch > as well). The very first version I wrote for this patch did not have this line too, until I discovered I needed it. Fasten your seatbelts... Because TRANSCODER_EDP can be used by any CRTC, so when you stop using it you have to stop saying you're using it, otherwise you may have at some point 2 crtcs claiming they're using TRANSCODER_EDP (a disable crtc and an enabled one), then the HW state readout code will get completely confused. In other words: Imagine the following case: - xrandr --output eDP1 --auto --crtc 0 - xrandr --output eDP1 --off - xrandr --output eDP1 --auto --crtc 2 After the last command you will get a nice "pipe A assertion failure (expected off, current on)" because crtc 0 still claims it's using transcoder_edp, so the hw state readout function will read it (through pipeconf) and expect it to be off, when it is actually on because it's being used by crtc 2. So when we make "intel_crtc->cpu_transcoder = intel_crtc->pipe" we make sure we're pointing to our own original crtc which is certainly not used by any other CRTC. > > -- > Damien
On Wed, Oct 24, 2012 at 6:33 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > 2012/10/24 Lespiau, Damien <damien.lespiau@intel.com>: >> On Tue, Oct 23, 2012 at 9:29 PM, Paulo Zanoni <przanoni@gmail.com> wrote: >>> static void haswell_crtc_off(struct drm_crtc *crtc) >>> { >>> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>> + >>> + intel_crtc->cpu_transcoder = intel_crtc->pipe; >>> intel_ddi_put_crtc_pll(crtc); >>> } >>> >> >> I can't find the reason why you would set the cpu_transcoder in the >> off() function, would you mind explaining why? (or maybe the clue is >> in a later patch, which might mean this hunk belongs to a later patch >> as well). > > The very first version I wrote for this patch did not have this line > too, until I discovered I needed it. Fasten your seatbelts... > > Because TRANSCODER_EDP can be used by any CRTC, so when you stop using > it you have to stop saying you're using it, otherwise you may have at > some point 2 crtcs claiming they're using TRANSCODER_EDP (a disable > crtc and an enabled one), then the HW state readout code will get > completely confused. > > In other words: > > Imagine the following case: > - xrandr --output eDP1 --auto --crtc 0 > - xrandr --output eDP1 --off > - xrandr --output eDP1 --auto --crtc 2 > > After the last command you will get a nice "pipe A assertion failure > (expected off, current on)" because crtc 0 still claims it's using > transcoder_edp, so the hw state readout function will read it (through > pipeconf) and expect it to be off, when it is actually on because it's > being used by crtc 2. > > So when we make "intel_crtc->cpu_transcoder = intel_crtc->pipe" we > make sure we're pointing to our own original crtc which is certainly > not used by any other CRTC. This needs to be in a comment somewhere. I think the long version here in the commit message, and a short one in the code. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 987af6f..2fcf284 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -58,6 +58,14 @@ enum pipe { }; #define pipe_name(p) ((p) + 'A') +enum transcoder { + TRANSCODER_A = 0, + TRANSCODER_B, + TRANSCODER_C, + TRANSCODER_EDP = 0xF, +}; +#define transcoder_name(t) ((t) + 'A') + enum plane { PLANE_A = 0, PLANE_B, diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c7c4b96..598f83a 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -26,6 +26,7 @@ #define _I915_REG_H_ #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a))) +#define _TRANSCODER(tran, a, b) ((a) + (tran)*((b)-(a))) #define _PORT(port, a, b) ((a) + (port)*((b)-(a))) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 67c9472..214ff5a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -937,6 +937,15 @@ intel_vlv_find_best_pll(const intel_limit_t *limit, struct drm_crtc *crtc, return true; } +enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv, + enum pipe pipe) +{ + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + + return intel_crtc->cpu_transcoder; +} + static void ironlake_wait_for_vblank(struct drm_device *dev, int pipe) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -3485,6 +3494,9 @@ static void ironlake_crtc_off(struct drm_crtc *crtc) static void haswell_crtc_off(struct drm_crtc *crtc) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + + intel_crtc->cpu_transcoder = intel_crtc->pipe; intel_ddi_put_crtc_pll(crtc); } @@ -5361,6 +5373,11 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, num_connectors++; } + if (is_cpu_edp) + intel_crtc->cpu_transcoder = TRANSCODER_EDP; + else + intel_crtc->cpu_transcoder = pipe; + /* We are not sure yet this won't happen. */ WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev)); @@ -7897,6 +7914,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) /* Swap pipes & planes for FBC on pre-965 */ intel_crtc->pipe = pipe; intel_crtc->plane = pipe; + intel_crtc->cpu_transcoder = pipe; if (IS_MOBILE(dev) && IS_GEN3(dev)) { DRM_DEBUG_KMS("swapping pipes & planes for FBC\n"); intel_crtc->plane = !pipe; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index c2e439b..14484ef 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -195,6 +195,7 @@ struct intel_crtc { struct drm_crtc base; enum pipe pipe; enum plane plane; + enum transcoder cpu_transcoder; u8 lut_r[256], lut_g[256], lut_b[256]; /* * Whether the crtc and the connected output pipeline is active. Implies @@ -506,6 +507,9 @@ extern struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev, struct drm_crtc *crtc); int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data, struct drm_file *file_priv); +extern enum transcoder +intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv, + enum pipe pipe); extern void intel_wait_for_vblank(struct drm_device *dev, int pipe); extern void intel_wait_for_pipe_off(struct drm_device *dev, int pipe);