Re: [PATCH 17/20] drm/i915: Use adjusted_mode in the fastboot hack to disable pfit

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel