| Message ID | 20170918101250.6076-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 18, 2017 at 12:12:50PM +0200, Maarten Lankhorst wrote: > Commit b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") removed > the call to wait_for_vblanks and replaced it with flip_done. > > Unfortunately legacy_cursor_update was unset too late, and the > replacement call drm_atomic_helper_wait_for_flip_done() was > a noop. Make sure that its unset before setup_commit() is > called to fix this issue. > > Changes since v1: > - Force vblank wait for watermarks not yet converted to atomic too. (Ville) > - Use for_each_new_intel_crtc_in_state. (Ville) > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Fixes: b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102675 > Testcase: kms_cursor_crc > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Reported-by: Marta Löfstedt <marta.lofstedt@intel.com> > Cc: Marta Löfstedt <marta.lofstedt@intel.com> > Tested-by: Marta Löfstedt <marta.lofstedt@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++--------------- > 1 file changed, 26 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 8599e425abb1..8d051256da1e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev, > struct drm_i915_private *dev_priv = to_i915(dev); > int ret = 0; > > - ret = drm_atomic_helper_setup_commit(state, nonblock); > - if (ret) > - return ret; > - > drm_atomic_state_get(state); > i915_sw_fence_init(&intel_state->commit_ready, > intel_atomic_commit_ready); > > - ret = intel_atomic_prepare_commit(dev, state); > - if (ret) { > - DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret); > - i915_sw_fence_commit(&intel_state->commit_ready); > - return ret; > - } > - > /* > * The intel_legacy_cursor_update() fast path takes care > * of avoiding the vblank waits for simple cursor > @@ -12540,19 +12529,37 @@ static int intel_atomic_commit(struct drm_device *dev, > * updates happen during the correct frames. Gen9+ have > * double buffered watermarks and so shouldn't need this. > * > - * Do this after drm_atomic_helper_setup_commit() and > - * intel_atomic_prepare_commit() because we still want > - * to skip the flip and fb cleanup waits. Although that > - * does risk yanking the mapping from under the display > - * engine. > + * Unset state->legacy_cursor_update before the call to > + * drm_atomic_helper_setup_commit() because otherwise > + * drm_atomic_helper_wait_for_flip_done() is a noop and > + * we get FIFO underruns because we didn't wait > + * for vblank. > * > * FIXME doing watermarks and fb cleanup from a vblank worker > * (assuming we had any) would solve these problems. > */ > - if (INTEL_GEN(dev_priv) < 9) > - state->legacy_cursor_update = false; > + if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) { > + struct intel_crtc_state *new_crtc_state; > + struct intel_crtc *crtc; > + int i; > + > + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i) > + if (new_crtc_state->wm.need_postvbl_update || > + new_crtc_state->update_wm_post) > + state->legacy_cursor_update = false; Hmm. I guess that's better. But I still don't see why you want to change this bit of code in this patch. AFAICS it's got nothing to do with the fix itself, and instead it's just trying to optimize some cursor updates that were kicked over to the slow path. Or am I missing something? > + } > + > + ret = intel_atomic_prepare_commit(dev, state); > + if (ret) { > + DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret); > + i915_sw_fence_commit(&intel_state->commit_ready); > + return ret; > + } > + > + ret = drm_atomic_helper_setup_commit(state, nonblock); > + if (!ret) > + ret = drm_atomic_helper_swap_state(state, true); > > - ret = drm_atomic_helper_swap_state(state, true); > if (ret) { > i915_sw_fence_commit(&intel_state->commit_ready); > > -- > 2.14.1
Hi Maarten,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.14-rc1 next-20170915]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maarten-Lankhorst/drm-i915-Unset-legacy_cursor_update-early-in-intel_atomic_commit-v2/20170918-223150
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x003-201738 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_display.c: In function 'intel_atomic_commit':
>> drivers/gpu/drm/i915/intel_display.c:12608:3: error: implicit declaration of function 'for_each_new_intel_crtc_in_state' [-Werror=implicit-function-declaration]
for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/i915/intel_display.c:12609:4: error: expected ';' before 'if'
if (new_crtc_state->wm.need_postvbl_update ||
^~
cc1: some warnings being treated as errors
vim +/for_each_new_intel_crtc_in_state +12608 drivers/gpu/drm/i915/intel_display.c
12561
12562 /**
12563 * intel_atomic_commit - commit validated state object
12564 * @dev: DRM device
12565 * @state: the top-level driver state object
12566 * @nonblock: nonblocking commit
12567 *
12568 * This function commits a top-level state object that has been validated
12569 * with drm_atomic_helper_check().
12570 *
12571 * RETURNS
12572 * Zero for success or -errno.
12573 */
12574 static int intel_atomic_commit(struct drm_device *dev,
12575 struct drm_atomic_state *state,
12576 bool nonblock)
12577 {
12578 struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
12579 struct drm_i915_private *dev_priv = to_i915(dev);
12580 int ret = 0;
12581
12582 drm_atomic_state_get(state);
12583 i915_sw_fence_init(&intel_state->commit_ready,
12584 intel_atomic_commit_ready);
12585
12586 /*
12587 * The intel_legacy_cursor_update() fast path takes care
12588 * of avoiding the vblank waits for simple cursor
12589 * movement and flips. For cursor on/off and size changes,
12590 * we want to perform the vblank waits so that watermark
12591 * updates happen during the correct frames. Gen9+ have
12592 * double buffered watermarks and so shouldn't need this.
12593 *
12594 * Unset state->legacy_cursor_update before the call to
12595 * drm_atomic_helper_setup_commit() because otherwise
12596 * drm_atomic_helper_wait_for_flip_done() is a noop and
12597 * we get FIFO underruns because we didn't wait
12598 * for vblank.
12599 *
12600 * FIXME doing watermarks and fb cleanup from a vblank worker
12601 * (assuming we had any) would solve these problems.
12602 */
12603 if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) {
12604 struct intel_crtc_state *new_crtc_state;
12605 struct intel_crtc *crtc;
12606 int i;
12607
12608 for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
12609 if (new_crtc_state->wm.need_postvbl_update ||
12610 new_crtc_state->update_wm_post)
12611 state->legacy_cursor_update = false;
12612 }
12613
12614 ret = intel_atomic_prepare_commit(dev, state);
12615 if (ret) {
12616 DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
12617 i915_sw_fence_commit(&intel_state->commit_ready);
12618 return ret;
12619 }
12620
12621 ret = drm_atomic_helper_setup_commit(state, nonblock);
12622 if (!ret)
12623 ret = drm_atomic_helper_swap_state(state, true);
12624
12625 if (ret) {
12626 i915_sw_fence_commit(&intel_state->commit_ready);
12627
12628 drm_atomic_helper_cleanup_planes(dev, state);
12629 return ret;
12630 }
12631 dev_priv->wm.distrust_bios_wm = false;
12632 intel_shared_dpll_swap_state(state);
12633 intel_atomic_track_fbs(state);
12634
12635 if (intel_state->modeset) {
12636 memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
12637 sizeof(intel_state->min_pixclk));
12638 dev_priv->active_crtcs = intel_state->active_crtcs;
12639 dev_priv->cdclk.logical = intel_state->cdclk.logical;
12640 dev_priv->cdclk.actual = intel_state->cdclk.actual;
12641 }
12642
12643 drm_atomic_state_get(state);
12644 INIT_WORK(&state->commit_work, intel_atomic_commit_work);
12645
12646 i915_sw_fence_commit(&intel_state->commit_ready);
12647 if (nonblock)
12648 queue_work(system_unbound_wq, &state->commit_work);
12649 else
12650 intel_atomic_commit_tail(state);
12651
12652
12653 return 0;
12654 }
12655
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Maarten,
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.14-rc1 next-20170918]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maarten-Lankhorst/drm-i915-Unset-legacy_cursor_update-early-in-intel_atomic_commit-v2/20170918-223150
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x012-201738 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/intel_display.c: In function 'intel_atomic_commit':
drivers/gpu/drm/i915/intel_display.c:12608:3: error: implicit declaration of function 'for_each_new_intel_crtc_in_state' [-Werror=implicit-function-declaration]
for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/list.h:4,
from include/linux/dmi.h:4,
from drivers/gpu/drm/i915/intel_display.c:27:
include/linux/compiler.h:156:2: error: expected ';' before 'if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> drivers/gpu/drm/i915/intel_display.c:12609:4: note: in expansion of macro 'if'
if (new_crtc_state->wm.need_postvbl_update ||
^~
drivers/gpu/drm/i915/intel_display.c: At top level:
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'strcpy' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:390:2: note: in expansion of macro 'if'
if (p_size == (size_t)-1 && q_size == (size_t)-1)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'kmemdup' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:380:2: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'kmemdup' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:378:2: note: in expansion of macro 'if'
if (__builtin_constant_p(size) && p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr_inv' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:369:2: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr_inv' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:367:2: note: in expansion of macro 'if'
if (__builtin_constant_p(size) && p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:358:2: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memchr' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:356:2: note: in expansion of macro 'if'
if (__builtin_constant_p(size) && p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:348:2: note: in expansion of macro 'if'
if (p_size < size || q_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:345:3: note: in expansion of macro 'if'
if (q_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:343:3: note: in expansion of macro 'if'
if (p_size < size)
^~
include/linux/compiler.h:162:4: warning: '______f' is static but declared in inline function 'memcmp' which is not static
______f = { \
^
include/linux/compiler.h:154:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
include/linux/string.h:342:2: note: in expansion of macro 'if'
vim +/if +12609 drivers/gpu/drm/i915/intel_display.c
12561
12562 /**
12563 * intel_atomic_commit - commit validated state object
12564 * @dev: DRM device
12565 * @state: the top-level driver state object
12566 * @nonblock: nonblocking commit
12567 *
12568 * This function commits a top-level state object that has been validated
12569 * with drm_atomic_helper_check().
12570 *
12571 * RETURNS
12572 * Zero for success or -errno.
12573 */
12574 static int intel_atomic_commit(struct drm_device *dev,
12575 struct drm_atomic_state *state,
12576 bool nonblock)
12577 {
12578 struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
12579 struct drm_i915_private *dev_priv = to_i915(dev);
12580 int ret = 0;
12581
12582 drm_atomic_state_get(state);
12583 i915_sw_fence_init(&intel_state->commit_ready,
12584 intel_atomic_commit_ready);
12585
12586 /*
12587 * The intel_legacy_cursor_update() fast path takes care
12588 * of avoiding the vblank waits for simple cursor
12589 * movement and flips. For cursor on/off and size changes,
12590 * we want to perform the vblank waits so that watermark
12591 * updates happen during the correct frames. Gen9+ have
12592 * double buffered watermarks and so shouldn't need this.
12593 *
12594 * Unset state->legacy_cursor_update before the call to
12595 * drm_atomic_helper_setup_commit() because otherwise
12596 * drm_atomic_helper_wait_for_flip_done() is a noop and
12597 * we get FIFO underruns because we didn't wait
12598 * for vblank.
12599 *
12600 * FIXME doing watermarks and fb cleanup from a vblank worker
12601 * (assuming we had any) would solve these problems.
12602 */
12603 if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) {
12604 struct intel_crtc_state *new_crtc_state;
12605 struct intel_crtc *crtc;
12606 int i;
12607
12608 for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
12609 if (new_crtc_state->wm.need_postvbl_update ||
12610 new_crtc_state->update_wm_post)
12611 state->legacy_cursor_update = false;
12612 }
12613
12614 ret = intel_atomic_prepare_commit(dev, state);
12615 if (ret) {
12616 DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
12617 i915_sw_fence_commit(&intel_state->commit_ready);
12618 return ret;
12619 }
12620
12621 ret = drm_atomic_helper_setup_commit(state, nonblock);
12622 if (!ret)
12623 ret = drm_atomic_helper_swap_state(state, true);
12624
12625 if (ret) {
12626 i915_sw_fence_commit(&intel_state->commit_ready);
12627
12628 drm_atomic_helper_cleanup_planes(dev, state);
12629 return ret;
12630 }
12631 dev_priv->wm.distrust_bios_wm = false;
12632 intel_shared_dpll_swap_state(state);
12633 intel_atomic_track_fbs(state);
12634
12635 if (intel_state->modeset) {
12636 memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
12637 sizeof(intel_state->min_pixclk));
12638 dev_priv->active_crtcs = intel_state->active_crtcs;
12639 dev_priv->cdclk.logical = intel_state->cdclk.logical;
12640 dev_priv->cdclk.actual = intel_state->cdclk.actual;
12641 }
12642
12643 drm_atomic_state_get(state);
12644 INIT_WORK(&state->commit_work, intel_atomic_commit_work);
12645
12646 i915_sw_fence_commit(&intel_state->commit_ready);
12647 if (nonblock)
12648 queue_work(system_unbound_wq, &state->commit_work);
12649 else
12650 intel_atomic_commit_tail(state);
12651
12652
12653 return 0;
12654 }
12655
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Op 18-09-17 om 17:03 schreef Ville Syrjälä: > On Mon, Sep 18, 2017 at 12:12:50PM +0200, Maarten Lankhorst wrote: >> Commit b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") removed >> the call to wait_for_vblanks and replaced it with flip_done. >> >> Unfortunately legacy_cursor_update was unset too late, and the >> replacement call drm_atomic_helper_wait_for_flip_done() was >> a noop. Make sure that its unset before setup_commit() is >> called to fix this issue. >> >> Changes since v1: >> - Force vblank wait for watermarks not yet converted to atomic too. (Ville) >> - Use for_each_new_intel_crtc_in_state. (Ville) >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Fixes: b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102675 >> Testcase: kms_cursor_crc >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com> >> Cc: Marta Löfstedt <marta.lofstedt@intel.com> >> Tested-by: Marta Löfstedt <marta.lofstedt@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++--------------- >> 1 file changed, 26 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 8599e425abb1..8d051256da1e 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev, >> struct drm_i915_private *dev_priv = to_i915(dev); >> int ret = 0; >> >> - ret = drm_atomic_helper_setup_commit(state, nonblock); >> - if (ret) >> - return ret; >> - >> drm_atomic_state_get(state); >> i915_sw_fence_init(&intel_state->commit_ready, >> intel_atomic_commit_ready); >> >> - ret = intel_atomic_prepare_commit(dev, state); >> - if (ret) { >> - DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret); >> - i915_sw_fence_commit(&intel_state->commit_ready); >> - return ret; >> - } >> - >> /* >> * The intel_legacy_cursor_update() fast path takes care >> * of avoiding the vblank waits for simple cursor >> @@ -12540,19 +12529,37 @@ static int intel_atomic_commit(struct drm_device *dev, >> * updates happen during the correct frames. Gen9+ have >> * double buffered watermarks and so shouldn't need this. >> * >> - * Do this after drm_atomic_helper_setup_commit() and >> - * intel_atomic_prepare_commit() because we still want >> - * to skip the flip and fb cleanup waits. Although that >> - * does risk yanking the mapping from under the display >> - * engine. >> + * Unset state->legacy_cursor_update before the call to >> + * drm_atomic_helper_setup_commit() because otherwise >> + * drm_atomic_helper_wait_for_flip_done() is a noop and >> + * we get FIFO underruns because we didn't wait >> + * for vblank. >> * >> * FIXME doing watermarks and fb cleanup from a vblank worker >> * (assuming we had any) would solve these problems. >> */ >> - if (INTEL_GEN(dev_priv) < 9) >> - state->legacy_cursor_update = false; >> + if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) { >> + struct intel_crtc_state *new_crtc_state; >> + struct intel_crtc *crtc; >> + int i; >> + >> + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i) >> + if (new_crtc_state->wm.need_postvbl_update || >> + new_crtc_state->update_wm_post) >> + state->legacy_cursor_update = false; > Hmm. I guess that's better. But I still don't see why you want to change > this bit of code in this patch. AFAICS it's got nothing to do with the fix > itself, and instead it's just trying to optimize some cursor updates > that were kicked over to the slow path. Or am I missing something? We accidentally removed the vblank wait for the slowpath, but I don't think we should reintroduce the vblank except where we need it.. ~Maarten
On Tue, Sep 19, 2017 at 11:06:52AM +0200, Maarten Lankhorst wrote: > Op 18-09-17 om 17:03 schreef Ville Syrjälä: > > On Mon, Sep 18, 2017 at 12:12:50PM +0200, Maarten Lankhorst wrote: > >> Commit b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") removed > >> the call to wait_for_vblanks and replaced it with flip_done. > >> > >> Unfortunately legacy_cursor_update was unset too late, and the > >> replacement call drm_atomic_helper_wait_for_flip_done() was > >> a noop. Make sure that its unset before setup_commit() is > >> called to fix this issue. > >> > >> Changes since v1: > >> - Force vblank wait for watermarks not yet converted to atomic too. (Ville) > >> - Use for_each_new_intel_crtc_in_state. (Ville) > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Fixes: b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102675 > >> Testcase: kms_cursor_crc > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Cc: Jani Nikula <jani.nikula@linux.intel.com> > >> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com> > >> Cc: Marta Löfstedt <marta.lofstedt@intel.com> > >> Tested-by: Marta Löfstedt <marta.lofstedt@intel.com> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++--------------- > >> 1 file changed, 26 insertions(+), 19 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 8599e425abb1..8d051256da1e 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev, > >> struct drm_i915_private *dev_priv = to_i915(dev); > >> int ret = 0; > >> > >> - ret = drm_atomic_helper_setup_commit(state, nonblock); > >> - if (ret) > >> - return ret; > >> - > >> drm_atomic_state_get(state); > >> i915_sw_fence_init(&intel_state->commit_ready, > >> intel_atomic_commit_ready); > >> > >> - ret = intel_atomic_prepare_commit(dev, state); > >> - if (ret) { > >> - DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret); > >> - i915_sw_fence_commit(&intel_state->commit_ready); > >> - return ret; > >> - } > >> - > >> /* > >> * The intel_legacy_cursor_update() fast path takes care > >> * of avoiding the vblank waits for simple cursor > >> @@ -12540,19 +12529,37 @@ static int intel_atomic_commit(struct drm_device *dev, > >> * updates happen during the correct frames. Gen9+ have > >> * double buffered watermarks and so shouldn't need this. > >> * > >> - * Do this after drm_atomic_helper_setup_commit() and > >> - * intel_atomic_prepare_commit() because we still want > >> - * to skip the flip and fb cleanup waits. Although that > >> - * does risk yanking the mapping from under the display > >> - * engine. > >> + * Unset state->legacy_cursor_update before the call to > >> + * drm_atomic_helper_setup_commit() because otherwise > >> + * drm_atomic_helper_wait_for_flip_done() is a noop and > >> + * we get FIFO underruns because we didn't wait > >> + * for vblank. > >> * > >> * FIXME doing watermarks and fb cleanup from a vblank worker > >> * (assuming we had any) would solve these problems. > >> */ > >> - if (INTEL_GEN(dev_priv) < 9) > >> - state->legacy_cursor_update = false; > >> + if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) { > >> + struct intel_crtc_state *new_crtc_state; > >> + struct intel_crtc *crtc; > >> + int i; > >> + > >> + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i) > >> + if (new_crtc_state->wm.need_postvbl_update || > >> + new_crtc_state->update_wm_post) > >> + state->legacy_cursor_update = false; > > Hmm. I guess that's better. But I still don't see why you want to change > > this bit of code in this patch. AFAICS it's got nothing to do with the fix > > itself, and instead it's just trying to optimize some cursor updates > > that were kicked over to the slow path. Or am I missing something? > > We accidentally removed the vblank wait for the slowpath, but I don't think we should reintroduce the vblank except where we need it.. IMO any regression fix should ideally get us back exactly where we were.
Op 19-09-17 om 12:24 schreef Ville Syrjälä: > On Tue, Sep 19, 2017 at 11:06:52AM +0200, Maarten Lankhorst wrote: >> Op 18-09-17 om 17:03 schreef Ville Syrjälä: >>> On Mon, Sep 18, 2017 at 12:12:50PM +0200, Maarten Lankhorst wrote: >>>> Commit b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") removed >>>> the call to wait_for_vblanks and replaced it with flip_done. >>>> >>>> Unfortunately legacy_cursor_update was unset too late, and the >>>> replacement call drm_atomic_helper_wait_for_flip_done() was >>>> a noop. Make sure that its unset before setup_commit() is >>>> called to fix this issue. >>>> >>>> Changes since v1: >>>> - Force vblank wait for watermarks not yet converted to atomic too. (Ville) >>>> - Use for_each_new_intel_crtc_in_state. (Ville) >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Fixes: b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") >>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102675 >>>> Testcase: kms_cursor_crc >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Cc: Jani Nikula <jani.nikula@linux.intel.com> >>>> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com> >>>> Cc: Marta Löfstedt <marta.lofstedt@intel.com> >>>> Tested-by: Marta Löfstedt <marta.lofstedt@intel.com> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++--------------- >>>> 1 file changed, 26 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index 8599e425abb1..8d051256da1e 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev, >>>> struct drm_i915_private *dev_priv = to_i915(dev); >>>> int ret = 0; >>>> >>>> - ret = drm_atomic_helper_setup_commit(state, nonblock); >>>> - if (ret) >>>> - return ret; >>>> - >>>> drm_atomic_state_get(state); >>>> i915_sw_fence_init(&intel_state->commit_ready, >>>> intel_atomic_commit_ready); >>>> >>>> - ret = intel_atomic_prepare_commit(dev, state); >>>> - if (ret) { >>>> - DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret); >>>> - i915_sw_fence_commit(&intel_state->commit_ready); >>>> - return ret; >>>> - } >>>> - >>>> /* >>>> * The intel_legacy_cursor_update() fast path takes care >>>> * of avoiding the vblank waits for simple cursor >>>> @@ -12540,19 +12529,37 @@ static int intel_atomic_commit(struct drm_device *dev, >>>> * updates happen during the correct frames. Gen9+ have >>>> * double buffered watermarks and so shouldn't need this. >>>> * >>>> - * Do this after drm_atomic_helper_setup_commit() and >>>> - * intel_atomic_prepare_commit() because we still want >>>> - * to skip the flip and fb cleanup waits. Although that >>>> - * does risk yanking the mapping from under the display >>>> - * engine. >>>> + * Unset state->legacy_cursor_update before the call to >>>> + * drm_atomic_helper_setup_commit() because otherwise >>>> + * drm_atomic_helper_wait_for_flip_done() is a noop and >>>> + * we get FIFO underruns because we didn't wait >>>> + * for vblank. >>>> * >>>> * FIXME doing watermarks and fb cleanup from a vblank worker >>>> * (assuming we had any) would solve these problems. >>>> */ >>>> - if (INTEL_GEN(dev_priv) < 9) >>>> - state->legacy_cursor_update = false; >>>> + if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) { >>>> + struct intel_crtc_state *new_crtc_state; >>>> + struct intel_crtc *crtc; >>>> + int i; >>>> + >>>> + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i) >>>> + if (new_crtc_state->wm.need_postvbl_update || >>>> + new_crtc_state->update_wm_post) >>>> + state->legacy_cursor_update = false; >>> Hmm. I guess that's better. But I still don't see why you want to change >>> this bit of code in this patch. AFAICS it's got nothing to do with the fix >>> itself, and instead it's just trying to optimize some cursor updates >>> that were kicked over to the slow path. Or am I missing something? >> We accidentally removed the vblank wait for the slowpath, but I don't think we should reintroduce the vblank except where we need it.. > IMO any regression fix should ideally get us back exactly where we were. > Ok I'll send it out as separate patch then..
On Mon, Sep 18, 2017 at 12:12:50PM +0200, Maarten Lankhorst wrote: > Commit b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") removed > the call to wait_for_vblanks and replaced it with flip_done. > > Unfortunately legacy_cursor_update was unset too late, and the > replacement call drm_atomic_helper_wait_for_flip_done() was > a noop. Make sure that its unset before setup_commit() is > called to fix this issue. > > Changes since v1: > - Force vblank wait for watermarks not yet converted to atomic too. (Ville) > - Use for_each_new_intel_crtc_in_state. (Ville) > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Fixes: b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102675 > Testcase: kms_cursor_crc > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Reported-by: Marta Löfstedt <marta.lofstedt@intel.com> > Cc: Marta Löfstedt <marta.lofstedt@intel.com> > Tested-by: Marta Löfstedt <marta.lofstedt@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> I think the proper fix is switching over to the async plane helpers and unconditionally always clearing the legacy cursor hack. Maybe even outright removing it, since it doesn't really work all that well with most drivers. -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++--------------- > 1 file changed, 26 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 8599e425abb1..8d051256da1e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev, > struct drm_i915_private *dev_priv = to_i915(dev); > int ret = 0; > > - ret = drm_atomic_helper_setup_commit(state, nonblock); > - if (ret) > - return ret; > - > drm_atomic_state_get(state); > i915_sw_fence_init(&intel_state->commit_ready, > intel_atomic_commit_ready); > > - ret = intel_atomic_prepare_commit(dev, state); > - if (ret) { > - DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret); > - i915_sw_fence_commit(&intel_state->commit_ready); > - return ret; > - } > - > /* > * The intel_legacy_cursor_update() fast path takes care > * of avoiding the vblank waits for simple cursor > @@ -12540,19 +12529,37 @@ static int intel_atomic_commit(struct drm_device *dev, > * updates happen during the correct frames. Gen9+ have > * double buffered watermarks and so shouldn't need this. > * > - * Do this after drm_atomic_helper_setup_commit() and > - * intel_atomic_prepare_commit() because we still want > - * to skip the flip and fb cleanup waits. Although that > - * does risk yanking the mapping from under the display > - * engine. > + * Unset state->legacy_cursor_update before the call to > + * drm_atomic_helper_setup_commit() because otherwise > + * drm_atomic_helper_wait_for_flip_done() is a noop and > + * we get FIFO underruns because we didn't wait > + * for vblank. > * > * FIXME doing watermarks and fb cleanup from a vblank worker > * (assuming we had any) would solve these problems. > */ > - if (INTEL_GEN(dev_priv) < 9) > - state->legacy_cursor_update = false; > + if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) { > + struct intel_crtc_state *new_crtc_state; > + struct intel_crtc *crtc; > + int i; > + > + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i) > + if (new_crtc_state->wm.need_postvbl_update || > + new_crtc_state->update_wm_post) > + state->legacy_cursor_update = false; > + } > + > + ret = intel_atomic_prepare_commit(dev, state); > + if (ret) { > + DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret); > + i915_sw_fence_commit(&intel_state->commit_ready); > + return ret; > + } > + > + ret = drm_atomic_helper_setup_commit(state, nonblock); > + if (!ret) > + ret = drm_atomic_helper_swap_state(state, true); > > - ret = drm_atomic_helper_swap_state(state, true); > if (ret) { > i915_sw_fence_commit(&intel_state->commit_ready); > > -- > 2.14.1 >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8599e425abb1..8d051256da1e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev, struct drm_i915_private *dev_priv = to_i915(dev); int ret = 0; - ret = drm_atomic_helper_setup_commit(state, nonblock); - if (ret) - return ret; - drm_atomic_state_get(state); i915_sw_fence_init(&intel_state->commit_ready, intel_atomic_commit_ready); - ret = intel_atomic_prepare_commit(dev, state); - if (ret) { - DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret); - i915_sw_fence_commit(&intel_state->commit_ready); - return ret; - } - /* * The intel_legacy_cursor_update() fast path takes care * of avoiding the vblank waits for simple cursor @@ -12540,19 +12529,37 @@ static int intel_atomic_commit(struct drm_device *dev, * updates happen during the correct frames. Gen9+ have * double buffered watermarks and so shouldn't need this. * - * Do this after drm_atomic_helper_setup_commit() and - * intel_atomic_prepare_commit() because we still want - * to skip the flip and fb cleanup waits. Although that - * does risk yanking the mapping from under the display - * engine. + * Unset state->legacy_cursor_update before the call to + * drm_atomic_helper_setup_commit() because otherwise + * drm_atomic_helper_wait_for_flip_done() is a noop and + * we get FIFO underruns because we didn't wait + * for vblank. * * FIXME doing watermarks and fb cleanup from a vblank worker * (assuming we had any) would solve these problems. */ - if (INTEL_GEN(dev_priv) < 9) - state->legacy_cursor_update = false; + if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) { + struct intel_crtc_state *new_crtc_state; + struct intel_crtc *crtc; + int i; + + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i) + if (new_crtc_state->wm.need_postvbl_update || + new_crtc_state->update_wm_post) + state->legacy_cursor_update = false; + } + + ret = intel_atomic_prepare_commit(dev, state); + if (ret) { + DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret); + i915_sw_fence_commit(&intel_state->commit_ready); + return ret; + } + + ret = drm_atomic_helper_setup_commit(state, nonblock); + if (!ret) + ret = drm_atomic_helper_swap_state(state, true); - ret = drm_atomic_helper_swap_state(state, true); if (ret) { i915_sw_fence_commit(&intel_state->commit_ready);
Commit b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") removed the call to wait_for_vblanks and replaced it with flip_done. Unfortunately legacy_cursor_update was unset too late, and the replacement call drm_atomic_helper_wait_for_flip_done() was a noop. Make sure that its unset before setup_commit() is called to fix this issue. Changes since v1: - Force vblank wait for watermarks not yet converted to atomic too. (Ville) - Use for_each_new_intel_crtc_in_state. (Ville) Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Fixes: b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102675 Testcase: kms_cursor_crc Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Jani Nikula <jani.nikula@linux.intel.com> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com> Cc: Marta Löfstedt <marta.lofstedt@intel.com> Tested-by: Marta Löfstedt <marta.lofstedt@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 19 deletions(-)