| Message ID | 1379608835-11760-18-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 19, 2013 at 05:40:32PM +0100, Damien Lespiau wrote: > When booting with i915.fastboot=1, we always take tha code path and end > up undoing what we're trying to do with adjusted_mode. > > Hopefully, as the fastboot hardware readout code is using adjusted_mode > as well, it should be equivalent. > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index f868266..2b9f80b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2288,9 +2288,12 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, > > /* Update pipe size and adjust fitter if needed */ > if (i915_fastboot) { > + const struct drm_display_mode *adjusted_mode = > + &intel_crtc->config.adjusted_mode; > + > I915_WRITE(PIPESRC(intel_crtc->pipe), > - ((crtc->mode.hdisplay - 1) << 16) | > - (crtc->mode.vdisplay - 1)); > + ((adjusted_mode->crtc_hdisplay - 1) << 16) | > + (adjusted_mode->crtc_vdisplay - 1)); > if (!intel_crtc->config.pch_pfit.enabled && > (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) || > intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) { OK, I'm offically confused by this thing. Maybe it got a bit broken by the pfit.enabled change? I must assume that the original intention of this was to turn off the panel fitter in case the BIOS had left it enabled w/ 0x0 size, but I'm not sure how that would even work. Anyways, now it will turn it off if it's already off, which doesn't make much sense. And I guess the PIPESRC write is there because we assume the BIOS left it wrong for the non-pfit case. We have explicit readout for it now, so we could actually check if that's the case. Next I started to think we'd need to fix up DSPSIZE too, but that's only for gen2/3 plane B, and the pfit parts here clearly assume a PCH platform. But we never check for PCH anywhere actually, so i915_fastboot seems to be one of those shoot yourelf in the foot kind of things. Also I have no idea what we'll be scanning out if the primary plane is enabled and ends up changing size when we hit this code.
On Fri, Sep 20, 2013 at 05:54:55PM +0300, Ville Syrjälä wrote: > On Thu, Sep 19, 2013 at 05:40:32PM +0100, Damien Lespiau wrote: > > When booting with i915.fastboot=1, we always take tha code path and end > > up undoing what we're trying to do with adjusted_mode. > > > > Hopefully, as the fastboot hardware readout code is using adjusted_mode > > as well, it should be equivalent. > > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index f868266..2b9f80b 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2288,9 +2288,12 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, > > > > /* Update pipe size and adjust fitter if needed */ > > if (i915_fastboot) { > > + const struct drm_display_mode *adjusted_mode = > > + &intel_crtc->config.adjusted_mode; > > + > > I915_WRITE(PIPESRC(intel_crtc->pipe), > > - ((crtc->mode.hdisplay - 1) << 16) | > > - (crtc->mode.vdisplay - 1)); > > + ((adjusted_mode->crtc_hdisplay - 1) << 16) | > > + (adjusted_mode->crtc_vdisplay - 1)); > > if (!intel_crtc->config.pch_pfit.enabled && > > (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) || > > intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) { > > OK, I'm offically confused by this thing. Maybe it got a bit broken > by the pfit.enabled change? > > I must assume that the original intention of this was to turn off the > panel fitter in case the BIOS had left it enabled w/ 0x0 size, but > I'm not sure how that would even work. Anyways, now it will turn it > off if it's already off, which doesn't make much sense. > > And I guess the PIPESRC write is there because we assume the BIOS left > it wrong for the non-pfit case. We have explicit readout for it now, > so we could actually check if that's the case. Well, I didn't even read beyond the PIPESRC write, but yes, now that you mention it, it looks dodgy. Jesse, do you remember what was the original intention? neither the commit introducing the change nor the comment are very verbose.
On Fri, 20 Sep 2013 16:33:47 +0100 Damien Lespiau <damien.lespiau@intel.com> wrote: > On Fri, Sep 20, 2013 at 05:54:55PM +0300, Ville Syrjälä wrote: > > On Thu, Sep 19, 2013 at 05:40:32PM +0100, Damien Lespiau wrote: > > > When booting with i915.fastboot=1, we always take tha code path and end > > > up undoing what we're trying to do with adjusted_mode. > > > > > > Hopefully, as the fastboot hardware readout code is using adjusted_mode > > > as well, it should be equivalent. > > > > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index f868266..2b9f80b 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -2288,9 +2288,12 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, > > > > > > /* Update pipe size and adjust fitter if needed */ > > > if (i915_fastboot) { > > > + const struct drm_display_mode *adjusted_mode = > > > + &intel_crtc->config.adjusted_mode; > > > + > > > I915_WRITE(PIPESRC(intel_crtc->pipe), > > > - ((crtc->mode.hdisplay - 1) << 16) | > > > - (crtc->mode.vdisplay - 1)); > > > + ((adjusted_mode->crtc_hdisplay - 1) << 16) | > > > + (adjusted_mode->crtc_vdisplay - 1)); > > > if (!intel_crtc->config.pch_pfit.enabled && > > > (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) || > > > intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) { > > > > OK, I'm offically confused by this thing. Maybe it got a bit broken > > by the pfit.enabled change? > > > > I must assume that the original intention of this was to turn off the > > panel fitter in case the BIOS had left it enabled w/ 0x0 size, but > > I'm not sure how that would even work. Anyways, now it will turn it > > off if it's already off, which doesn't make much sense. > > > > And I guess the PIPESRC write is there because we assume the BIOS left > > it wrong for the non-pfit case. We have explicit readout for it now, > > so we could actually check if that's the case. > > Well, I didn't even read beyond the PIPESRC write, but yes, now that you > mention it, it looks dodgy. > > Jesse, do you remember what was the original intention? neither the > commit introducing the change nor the comment are very verbose. Just for the record, I'll capture what we discussed on IRC. The reason for this is that in compute_mode_changes we check the native mode (not the pfit mode) to see if we can flip rather than do a full mode set. In the fastboot case, we'll flip, but if we don't update the pipesrc and pfit state, we'll end up with a big fb scanned out into the wrong sized surface. To fix this properly, we need to hoist the checks up into compute_mode_changes (or above), check the actual pfit state and whether the platform allows pfit disable with pipe active, and only then update the pipesrc and pfit state, even on the flip path. On top of that, other state like info frames and audio state needs to be tracked and preserved for fastboot as applicable. Then we can remove the parameter hacks.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f868266..2b9f80b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2288,9 +2288,12 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, /* Update pipe size and adjust fitter if needed */ if (i915_fastboot) { + const struct drm_display_mode *adjusted_mode = + &intel_crtc->config.adjusted_mode; + I915_WRITE(PIPESRC(intel_crtc->pipe), - ((crtc->mode.hdisplay - 1) << 16) | - (crtc->mode.vdisplay - 1)); + ((adjusted_mode->crtc_hdisplay - 1) << 16) | + (adjusted_mode->crtc_vdisplay - 1)); if (!intel_crtc->config.pch_pfit.enabled && (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) || intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
When booting with i915.fastboot=1, we always take tha code path and end up undoing what we're trying to do with adjusted_mode. Hopefully, as the fastboot hardware readout code is using adjusted_mode as well, it should be equivalent. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)