Op 12-02-18 om 21:55 schreef Chris Wilson: > Quoting Ville Syrjälä (2018-02-12 18:06:58) >> On Mon, Feb 12, 2018 at 05:24:54PM +0000, Chris Wilson wrote: >>> Quoting Ville Syrjälä (2018-02-12 16:55:28) >>>> On Mon, Feb 12, 2018 at 04:41:05PM +0100, Maarten Lankhorst wrote: >>>>> Op 12-02-18 om 16:31 schreef Chris Wilson: >>>>>> Quoting Maarten Lankhorst (2018-02-12 15:27:34) >>>>>>> Op 12-02-18 om 16:22 schreef Chris Wilson: >>>>>>>> Quoting Maarten Lankhorst (2018-02-12 15:16:39) >>>>>>>>> Op 12-02-18 om 16:10 schreef Chris Wilson: >>>>>>>>>> Quoting Maarten Lankhorst (2018-02-09 09:54:00) >>>>>>>>>>> This is a nice preparation for grabbing the uncore lock during >>>>>>>>>>> evasion. >>>>>>>>>>> Grabbing the spinlock with the lock held messes up the locking, >>>>>>>>>>> so it's easier to handover the reference to the eve >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com> >>>>>>>>>>> --- >>>>>>>>>>> drivers/gpu/drm/i915/intel_sprite.c | 11 ++++------- >>>>>>>>>>> 1 file changed, 4 insertions(+), 7 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c >>>>>>>>>>> b/drivers/gpu/drm/i915/intel_sprite.c >>>>>>>>>>> index 3be22c0fcfb5..971a1ea0db45 100644 >>>>>>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c >>>>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c >>>>>>>>>>> @@ -109,10 +109,10 @@ void intel_pipe_update_start(const struct >>>>>>>>>>> intel_crtc_state *new_crtc_state) >>>>>>>>>>> >>>>>>>>>>> local_irq_disable(); >>>>>>>>>>> >>>>>>>>>>> - if (min <= 0 || max <= 0) >>>>>>>>>>> + if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) >>>>>>>>>>> return; >>>>>>>>>>> >>>>>>>>>>> - if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) >>>>>>>>>>> + if (min <= 0 || max <= 0) >>>>>>>>>>> return; >>>>>>>>>>> >>>>>>>>>> The corresponding vblank_put is the one later in update_start(), >>>>>>>>>> right? >>>>>>>>>> I don't think you intended to keep this chunk. >>>>>>>>>> -Chris >>>>>>>>> I'm not sure what you mean? The vblank_put is now in pipe_update_end, >>>>>>>>> except if the >>>>>>>>> event takes over the reference. I think the code is correct. :) >>>>>>>> Then it's unbalanced in the case of error still. >>>>>>>> -Chris >>>>>>> It already would have been for events, hence the WARN_ON there. >>>>>>> I don't think we can do anything about it, this shouldn't ever >>>>>>> happen in practice, could be a BUG_ON for all I care. :) >>>>>> I would much prefer that over intentionally bad code. >>>>>> >>>>>> But do we really need to enable the vblank irq here? If the event >>>>>> requires it, doesn't it already enable the vblank. Here, we only need it >>>>>> when sleeping, can we not determine we have enough time before the >>>>>> vblank without enabling the interrupt? >>>>> I'm not sure why we get a reference to the vblank counter here. Perhaps >>>>> Ville does? >>>> We need the vblank irq to be enabled before we check the scanline since >>>> otherwise we may end up doing: >>>> >>>> 1. check scanline >>>> 3. vblank irq fires >>>> 2. enable vblank irq >>>> 3. wait for the next vblank >>>> >>>> So we'd end up wasting an entire frame. >>> Step: 2.5, check_scanline? >>> >>> Something like, >>> >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c >>> b/drivers/gpu/drm/i915/intel_sprite.c >>> index 574bd02c5a2e..70c2ee1c7b8c 100644 >>> --- a/drivers/gpu/drm/i915/intel_sprite.c >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c >>> @@ -97,6 +97,7 @@ void intel_pipe_update_start(const struct >>> intel_crtc_state *new_crtc_state) >>> bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || >>> IS_CHERRYVIEW(dev_priv)) && >>> intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI); >>> DEFINE_WAIT(wait); >>> + bool have_vblank_irq = false; >>> >>> vblank_start = adjusted_mode->crtc_vblank_start; >>> if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) >>> @@ -112,9 +113,6 @@ void intel_pipe_update_start(const struct >>> intel_crtc_state *new_crtc_state) >>> if (min <= 0 || max <= 0) >>> return; >>> >>> - if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) >>> - return; >>> - >>> crtc->debug.min_vbl = min; >>> crtc->debug.max_vbl = max; >>> trace_i915_pipe_update_start(crtc); >>> @@ -127,6 +125,10 @@ void intel_pipe_update_start(const struct >>> intel_crtc_state *new_crtc_state) >>> */ >>> prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE); >>> >>> + if (!have_vblank_irq) >>> + have_vblank_irq = !drm_crtc_vblank_get(&crtc->base); >>> + >> This doesn't seem to change anything. > Nothing wrt to the existing code :) > > The idea is that we have to enable the vblank-irq before doing the > scanline check in order to not miss the interrupt, which as I understand > it was the danger you highlighted with trying to avoid taking the > vblank-irq. > -Chris
I've taken a look at the code, and most of the time we set crtc_state->event. Either through calls like pageflip, or if not set we always allocate one in drm_atomic_helper_setup_commit() except for legacy cursor updates. Because of this I think the original patch is fine, and I kind of like having everything prepared in pipe_update_start, while pipe_update_end only has to release it. ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
