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

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

[Ville Syrjala]

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.

This fixes a problem introduced by commit a2936e3d9a9c ("drm/i915:
Use drm_mode_get_hv_timing() to populate plane clip rectangle")
where the inconsistent state is now leading the plane clipping code
to report a failure on account the plane dst coordinates not matching
the user mode size. Previously we did the plane clipping based on
the pipe src size instead and thus never noticed the inconsistency.

The failure manifests as a WARN:
[    0.762117] [drm:intel_dump_pipe_config [i915]] requested mode:
[    0.762142] [drm:drm_mode_debug_printmodeline [drm]] Modeline 0:"1366x768" 60 72143 1366 1414 1446 1526 768 771 777 784 0x40 0xa
...
[    0.762327] [drm:intel_dump_pipe_config [i915]] port clock: 72143, pipe src size: 1024x768, pixel rate 72143
...
[    0.764666] [drm:drm_atomic_helper_check_plane_state [drm_kms_helper]] Plane must cover entire CRTC
[    0.764690] [drm:drm_rect_debug_print [drm]] dst: 1024x768+0+0
[    0.764711] [drm:drm_rect_debug_print [drm]] clip: 1366x768+0+0
[    0.764713] ------------[ cut here ]------------
[    0.764714] Could not determine valid watermarks for inherited state
[    0.764792] WARNING: CPU: 4 PID: 159 at drivers/gpu/drm/i915/intel_display.c:14584 intel_modeset_init+0x3ce/0x19d0 [i915]
...

Cc: FadeMind <fademind@gmail.com>
Cc: Dave Jones <davej@codemonkey.org.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reported-by: FadeMind <fademind@gmail.com>
Reported-by: Dave Jones <davej@codemonkey.org.uk>
Tested-by: Dave Jones <davej@codemonkey.org.uk>
References: https://lists.freedesktop.org/archives/intel-gfx/2018-April/163186.html
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105992
Fixes: a2936e3d9a9c ("drm/i915: Use drm_mode_get_hv_timing() to populate plane clip rectangle")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

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));
 
-- 
2.16.1

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

✗ Fi.CI.CHECKPATCH: warning for drm/i915: Correctly populate user mode h/vdisplay with pipe src size during readout

[Patchwork]

== Series Details ==

Series: drm/i915: Correctly populate user mode h/vdisplay with pipe src size during readout
URL   : https://patchwork.freedesktop.org/series/42351/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
f70e93b138f0 drm/i915: Correctly populate user mode h/vdisplay with pipe src size during readout
-:44: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#44: 
References: https://lists.freedesktop.org/archives/intel-gfx/2018-April/163186.html

total: 0 errors, 1 warnings, 0 checks, 8 lines checked

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

✓ Fi.CI.BAT: success for drm/i915: Correctly populate user mode h/vdisplay with pipe src size during readout

[Patchwork]

== Series Details ==

Series: drm/i915: Correctly populate user mode h/vdisplay with pipe src size during readout
URL   : https://patchwork.freedesktop.org/series/42351/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4106 -> Patchwork_8817 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8817 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8817, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42351/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8817:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_8817 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-ivb-3520m:       PASS -> DMESG-WARN (fdo#106084)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-cnl-psr:         PASS -> DMESG-WARN (fdo#104951)

    
    ==== Possible fixes ====

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         FAIL (fdo#102575) -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         FAIL (fdo#104008) -> PASS

    
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#104951 https://bugs.freedesktop.org/show_bug.cgi?id=104951
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (39 -> 36) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4106 -> Patchwork_8817

  CI_DRM_4106: dd4a65248ce636039848b97f0b8e4704aa2f32b2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4450: 0350f0e7f6a0e07281445fc3082aa70419f4aac7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8817: f70e93b138f0d04711491a223655c72326136eb2 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4450: b57600ba58ae0cdbad86826fd653aa0191212f27 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

f70e93b138f0 drm/i915: Correctly populate user mode h/vdisplay with pipe src size during readout

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8817/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Correctly populate user mode h/vdisplay with pipe src size during readout

[Patchwork]

== Series Details ==

Series: drm/i915: Correctly populate user mode h/vdisplay with pipe src size during readout
URL   : https://patchwork.freedesktop.org/series/42351/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4106_full -> Patchwork_8817_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8817_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8817_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42351/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8817_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-vebox:
      shard-kbl:          SKIP -> PASS +1

    
== Known issues ==

  Here are the changes found in Patchwork_8817_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@2x-plain-flip-fb-recreate:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_flip@absolute-wf_vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#106087)

    igt@kms_sysfs_edid_timing:
      shard-apl:          PASS -> WARN (fdo#100047)

    
    ==== Possible fixes ====

    igt@gem_eio@in-flight-contexts-1us:
      shard-glk:          FAIL (fdo#105957) -> PASS

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
      shard-glk:          DMESG-WARN (fdo#106247) -> PASS

    igt@kms_rotation_crc@primary-rotation-180:
      shard-hsw:          FAIL (fdo#103925) -> PASS

    igt@kms_vblank@pipe-a-accuracy-idle:
      shard-hsw:          FAIL (fdo#102583) -> PASS

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102583 https://bugs.freedesktop.org/show_bug.cgi?id=102583
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#105957 https://bugs.freedesktop.org/show_bug.cgi?id=105957
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106087 https://bugs.freedesktop.org/show_bug.cgi?id=106087
  fdo#106247 https://bugs.freedesktop.org/show_bug.cgi?id=106247


== Participating hosts (9 -> 5) ==

  Missing    (4): shard-glkb pig-skl-6600 pig-glk-j4005 pig-hsw-4770r 


== Build changes ==

    * Linux: CI_DRM_4106 -> Patchwork_8817

  CI_DRM_4106: dd4a65248ce636039848b97f0b8e4704aa2f32b2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4450: 0350f0e7f6a0e07281445fc3082aa70419f4aac7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8817: f70e93b138f0d04711491a223655c72326136eb2 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4450: b57600ba58ae0cdbad86826fd653aa0191212f27 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8817/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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.

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

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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 :(

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

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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