| Message ID | 20180426163015.14232-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? -Chris
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.
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? -Chris
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. I was actually wondering whether we should make the scaled 800x600 mode look more like a proper 800x600 mode, ie. that the blanking lengths and clock would also get scaled proportionally to the h/vdisplay. That would more closely match the "fullscreen" scaling mode, whereas the way we do it here would match the "center" scaling mode. But I guess we generally just have to ingore the blanking lengths when scaling is involved, so migth as well leave the original timings in place apart from hdisplay/vdisplay. It's somewhat unfortunate that we don't have a better uapi than "fake the timings" for pipe scaling :(
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> 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
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> 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
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
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 09e96d547c01..5afb5a4cc67e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15288,6 +15288,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); if (crtc_state->base.active) { intel_mode_from_pipe_config(&crtc->base.mode, crtc_state); + crtc->base.mode.hdisplay = crtc_state->pipe_src_w; + crtc->base.mode.vdisplay = crtc_state->pipe_src_h; intel_mode_from_pipe_config(&crtc_state->base.adjusted_mode, crtc_state); WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));