Re: [PATCH] drm/i915: Correctly populate user mode h/vdisplay with pipe src size during readout

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, May 03, 2018 at 09:50:09AM +0300, Jani Nikula wrote:
> On Wed, 02 May 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Quoting Ville Syrjälä (2018-05-02 17:14:21)
> >> On Wed, May 02, 2018 at 04:57:09PM +0100, Chris Wilson wrote:
> >> > Quoting Ville Syrjälä (2018-05-02 16:52:41)
> >> > > On Wed, May 02, 2018 at 04:33:30PM +0100, Chris Wilson wrote:
> >> > > > Quoting Ville Syrjala (2018-04-26 17:30:15)
> >> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > > > > 
> >> > > > > During state readout we first read out the pipe src size, store
> >> > > > > that information in the user mode h/vdisplay, but later on we overwrite
> >> > > > > that with the actual crtc timings. That makes our read out crtc state
> >> > > > > inconsistent with itself when the BIOS has enabled the panel fitter to
> >> > > > > scale the pipe contents. Let's preserve the pipe src size based
> >> > > > > information in the user mode to make things consistent again.
> >> > > > 
> >> > > > The question I don't feel answered is: If this is the BIOS mode, why
> >> > > > aren't we filling it from get_hw_state?
> >> > > 
> >> > > I suppose the answer is that we're only filling out the bare minimum
> >> > > of information during the basic readout. That is everything we need
> >> > > for intel_pipe_config_compare() to do its job. Later on we fill the
> >> > > gaps to make the state actually presentable to userspace. We don't
> >> > > have to do that if the state we read out isn't actually going to be
> >> > > exposed to userspace.
> >> > > 
> >> > > I suppose we could consider doing a more thorough job up front, but
> >> > > I think we'd need to spend some though on eg. the handling of the
> >> > > mode blob. We probably wouldn't want userspace to gain access to
> >> > > our short lived internal mode blob created from the read out state.
> >> > 
> >> > Will we run into a problem where we say the current mode is 800x600, but
> >> > is in fact 1024x768 scaledfrom 800x600? E.g. if we for whatever reason
> >> > want to switch to a real 800x600 mode?
> >> 
> >> Seems unlikely that the real 800x600 mode would have the same blanking
> >> lengths and clock as the 1024x768 mode. So we should end up with a full
> >> modeset.
> >
> > Right, that's going to be pretty weird and unlikely.
> >
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> >From [1],
> 
> Tested-by: Larry Finger <Larry.Finger@lwfinger.net>

Amended, and pushed to dinq. Thanks for the bug reports, testing and
review.

> 
> BR,
> Jani.
> 
> 
> [1] http://mid.mail-archive.com/4371fd28-49fb-f019-1fc3-f1318b9562fd@lwfinger.net
> 
> 
> >
> > I guess you would want to throw in a comment as to why this is a special
> > case... But this whole pass is pretty special inheritance code...
> > -Chris
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx