| Message ID | 20241003154421.33805-4-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/xe: Reduce flickering when inheriting BIOS fb. | expand |
On Thu, 03 Oct 2024, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > I'm planning to reorder readout in the Xe sequence in such a way that > interrupts will not be available, so just use an async flip. > > Since the new FB points to the same pages, it will not tear. It also > has the benefit of perhaps being slightly faster. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c > index fdb141cfa4274..73a3624e34098 100644 > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > @@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, > to_intel_plane_state(plane->base.state); > enum plane_id plane_id = plane->id; > enum pipe pipe = crtc->pipe; > - u32 base; > + u32 base, plane_ctl; > > if (!plane_state->uapi.visible) > return false; > @@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, > if (plane_config->base == base) > return false; > > + /* Perform an async flip to the new surface. */ > + plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id)); > + plane_ctl |= PLANE_CTL_ASYNC_FLIP; > + > + intel_de_write(i915, PLANE_CTL(pipe, plane_id), plane_ctl); > intel_de_write(i915, PLANE_SURF(pipe, plane_id), base); > > - return true; > + if (intel_de_wait_custom(i915, PLANE_SURFLIVE(pipe, plane_id), ~0U, base, 0, 40, NULL) < 0) Why not just intel_de_wait()? BR, Jani. > + drm_warn(&i915->drm, "async flip timed out\n"); > + > + /* No need to vblank wait either */ > + return false; > }
On Thu, Oct 03, 2024 at 05:44:12PM +0200, Maarten Lankhorst wrote: > I'm planning to reorder readout in the Xe sequence in such a way that > interrupts will not be available, so just use an async flip. > > Since the new FB points to the same pages, it will not tear. It also > has the benefit of perhaps being slightly faster. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c > index fdb141cfa4274..73a3624e34098 100644 > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > @@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, > to_intel_plane_state(plane->base.state); > enum plane_id plane_id = plane->id; > enum pipe pipe = crtc->pipe; > - u32 base; > + u32 base, plane_ctl; > > if (!plane_state->uapi.visible) > return false; > @@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, > if (plane_config->base == base) > return false; > > + /* Perform an async flip to the new surface. */ > + plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id)); > + plane_ctl |= PLANE_CTL_ASYNC_FLIP; > + No async flips! > + intel_de_write(i915, PLANE_CTL(pipe, plane_id), plane_ctl); > intel_de_write(i915, PLANE_SURF(pipe, plane_id), base); > > - return true; > + if (intel_de_wait_custom(i915, PLANE_SURFLIVE(pipe, plane_id), ~0U, base, 0, 40, NULL) < 0) > + drm_warn(&i915->drm, "async flip timed out\n"); > + > + /* No need to vblank wait either */ > + return false; > } > -- > 2.45.2
Hello, Den 2024-10-03 kl. 18:14, skrev Ville Syrjälä: > On Thu, Oct 03, 2024 at 05:44:12PM +0200, Maarten Lankhorst wrote: >> I'm planning to reorder readout in the Xe sequence in such a way that >> interrupts will not be available, so just use an async flip. >> >> Since the new FB points to the same pages, it will not tear. It also >> has the benefit of perhaps being slightly faster. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c >> index fdb141cfa4274..73a3624e34098 100644 >> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c >> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c >> @@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, >> to_intel_plane_state(plane->base.state); >> enum plane_id plane_id = plane->id; >> enum pipe pipe = crtc->pipe; >> - u32 base; >> + u32 base, plane_ctl; >> >> if (!plane_state->uapi.visible) >> return false; >> @@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, >> if (plane_config->base == base) >> return false; >> >> + /* Perform an async flip to the new surface. */ >> + plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id)); >> + plane_ctl |= PLANE_CTL_ASYNC_FLIP; >> + > > No async flips! Can you please explain your reasoning? I think async flip would fit perfectly here. We cannot perform a wait_for_vblank as we will not have interrupts enabled yet. Additionally an async flip would cause a faster driver loading. Since the FB is exactly the same except set to a different address, no tearing will occur. Cheers, ~Maarten
Den 2024-10-03 kl. 17:58, skrev Jani Nikula: > On Thu, 03 Oct 2024, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: >> I'm planning to reorder readout in the Xe sequence in such a way that >> interrupts will not be available, so just use an async flip. >> >> Since the new FB points to the same pages, it will not tear. It also >> has the benefit of perhaps being slightly faster. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c >> index fdb141cfa4274..73a3624e34098 100644 >> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c >> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c >> @@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, >> to_intel_plane_state(plane->base.state); >> enum plane_id plane_id = plane->id; >> enum pipe pipe = crtc->pipe; >> - u32 base; >> + u32 base, plane_ctl; >> >> if (!plane_state->uapi.visible) >> return false; >> @@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, >> if (plane_config->base == base) >> return false; >> >> + /* Perform an async flip to the new surface. */ >> + plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id)); >> + plane_ctl |= PLANE_CTL_ASYNC_FLIP; >> + >> + intel_de_write(i915, PLANE_CTL(pipe, plane_id), plane_ctl); >> intel_de_write(i915, PLANE_SURF(pipe, plane_id), base); >> >> - return true; >> + if (intel_de_wait_custom(i915, PLANE_SURFLIVE(pipe, plane_id), ~0U, base, 0, 40, NULL) < 0) > > Why not just intel_de_wait()? > > BR, > Jani. Yes, I overlooked that when I was browsing through intel_de.h :-) Can respin if we agree on the direction. Cheers, ~Maarten
On Thu, Oct 03, 2024 at 10:50:24PM +0200, Maarten Lankhorst wrote: > Hello, > > Den 2024-10-03 kl. 18:14, skrev Ville Syrjälä: > > On Thu, Oct 03, 2024 at 05:44:12PM +0200, Maarten Lankhorst wrote: > >> I'm planning to reorder readout in the Xe sequence in such a way that > >> interrupts will not be available, so just use an async flip. > >> > >> Since the new FB points to the same pages, it will not tear. It also > >> has the benefit of perhaps being slightly faster. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++-- > >> 1 file changed, 11 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c > >> index fdb141cfa4274..73a3624e34098 100644 > >> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > >> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > >> @@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, > >> to_intel_plane_state(plane->base.state); > >> enum plane_id plane_id = plane->id; > >> enum pipe pipe = crtc->pipe; > >> - u32 base; > >> + u32 base, plane_ctl; > >> > >> if (!plane_state->uapi.visible) > >> return false; > >> @@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, > >> if (plane_config->base == base) > >> return false; > >> > >> + /* Perform an async flip to the new surface. */ > >> + plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id)); > >> + plane_ctl |= PLANE_CTL_ASYNC_FLIP; > >> + > > > > No async flips! > > Can you please explain your reasoning? Async flips are special. They have all kinds of random hardware limitations. > I think async flip would fit > perfectly here. We cannot perform a wait_for_vblank as we will not have > interrupts enabled yet. The type of flip is irrelevant when you just poll until it's done. > Additionally an async flip would cause a faster > driver loading. Since the FB is exactly the same except set to a > different address, no tearing will occur. Until we violate some hardware requirement and you get a fault.
Hi Maarten, kernel test robot noticed the following build errors: [auto build test ERROR on drm-xe/drm-xe-next] [also build test ERROR on drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.12-rc1] [cannot apply to next-20241004] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Maarten-Lankhorst/drm-xe-display-Handle-stolen-bar-readout-in-the-same-way-as-lmem/20241004-000534 base: https://gitlab.freedesktop.org/drm/xe/kernel.git drm-xe-next patch link: https://lore.kernel.org/r/20241003154421.33805-4-maarten.lankhorst%40linux.intel.com patch subject: [PATCH v3 03/12] drm/i915/display: Use async flip when available for initial plane config config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20241004/202410042053.DCNgBMOr-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241004/202410042053.DCNgBMOr-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202410042053.DCNgBMOr-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/gpu/drm/i915/display/skl_universal_plane.c:2814:14: error: call to undeclared function 'intel_read'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 2814 | plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id)); | ^ 1 error generated. vim +/intel_read +2814 drivers/gpu/drm/i915/display/skl_universal_plane.c 2789 2790 bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, 2791 const struct intel_initial_plane_config *plane_config) 2792 { 2793 struct drm_i915_private *i915 = to_i915(crtc->base.dev); 2794 struct intel_plane *plane = to_intel_plane(crtc->base.primary); 2795 const struct intel_plane_state *plane_state = 2796 to_intel_plane_state(plane->base.state); 2797 enum plane_id plane_id = plane->id; 2798 enum pipe pipe = crtc->pipe; 2799 u32 base, plane_ctl; 2800 2801 if (!plane_state->uapi.visible) 2802 return false; 2803 2804 base = intel_plane_ggtt_offset(plane_state); 2805 2806 /* 2807 * We may have moved the surface to a different 2808 * part of ggtt, make the plane aware of that. 2809 */ 2810 if (plane_config->base == base) 2811 return false; 2812 2813 /* Perform an async flip to the new surface. */ > 2814 plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id));
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c index fdb141cfa4274..73a3624e34098 100644 --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c @@ -2800,7 +2800,7 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, to_intel_plane_state(plane->base.state); enum plane_id plane_id = plane->id; enum pipe pipe = crtc->pipe; - u32 base; + u32 base, plane_ctl; if (!plane_state->uapi.visible) return false; @@ -2814,7 +2814,16 @@ bool skl_fixup_initial_plane_config(struct intel_crtc *crtc, if (plane_config->base == base) return false; + /* Perform an async flip to the new surface. */ + plane_ctl = intel_read(i915, PLANE_CTL(pipe, plane_id)); + plane_ctl |= PLANE_CTL_ASYNC_FLIP; + + intel_de_write(i915, PLANE_CTL(pipe, plane_id), plane_ctl); intel_de_write(i915, PLANE_SURF(pipe, plane_id), base); - return true; + if (intel_de_wait_custom(i915, PLANE_SURFLIVE(pipe, plane_id), ~0U, base, 0, 40, NULL) < 0) + drm_warn(&i915->drm, "async flip timed out\n"); + + /* No need to vblank wait either */ + return false; }
I'm planning to reorder readout in the Xe sequence in such a way that interrupts will not be available, so just use an async flip. Since the new FB points to the same pages, it will not tear. It also has the benefit of perhaps being slightly faster. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/display/skl_universal_plane.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)