On Thu, 6 Mar 2014 09:35:23 +0000
Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Mar 05, 2014 at 02:48:26PM -0800, Jesse Barnes wrote:
> > @@ -7554,6 +7610,8 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> > goto fail;
> > }
> >
> > + intel_sync_crtc(crtc);
> > +
> > /* we only need to pin inside GTT if cursor is non-phy */
> > mutex_lock(&dev->struct_mutex);
> > if (!INTEL_INFO(dev)->cursor_needs_physical) {
> > @@ -7639,6 +7697,8 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
> > intel_crtc->cursor_x = clamp_t(int, x, SHRT_MIN, SHRT_MAX);
> > intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX);
> >
> > + intel_sync_crtc(crtc);
> > +
> > if (intel_crtc->active)
> > intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> >
>
> Hmm. Would be much nicer if touching the cursor didn't incur a delay.
> And it would seem to quite easy to capture the state change and queue it
> for when the CRTC is re-enabled.
Do you think that's worthwhile? I guess we'll block userspace a bit
here, but presumably the cursor won't be visible until the mode set
completes anyway...
But queuing this stuff is another option.
>
> > @@ -8682,6 +8742,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> > if (work == NULL)
> > return -ENOMEM;
> >
> > + intel_sync_crtc(crtc);
> > +
> > work->event = event;
> > work->crtc = crtc;
> > work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
> > @@ -9677,8 +9739,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> > intel_crtc_disable(&intel_crtc->base);
> >
> > for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> > - if (intel_crtc->base.enabled)
> > - dev_priv->display.crtc_disable(&intel_crtc->base);
> > + if (intel_crtc->base.enabled) {
> > + intel_queue_crtc_disable(&intel_crtc->base);
> > + intel_sync_crtc(&intel_crtc->base);
> > + }
> > }
> >
> > /* crtc->mode is already used by the ->mode_set callbacks, hence we need
> > @@ -9719,7 +9783,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> >
> > /* Now enable the clocks, plane, pipe, and connectors that we set up. */
> > for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
> > - dev_priv->display.crtc_enable(&intel_crtc->base);
> > + intel_queue_crtc_enable(&intel_crtc->base);
> >
> > /* FIXME: add subpixel order */
> > done:
> > @@ -9740,8 +9804,9 @@ static int intel_set_mode(struct drm_crtc *crtc,
> >
> > ret = __intel_set_mode(crtc, mode, x, y, fb);
> >
> > - if (ret == 0)
> > - intel_modeset_check_state(crtc->dev);
> > + /* FIXME: check after async crtc enable/disable */
> > +// if (ret == 0)
> > +// intel_modeset_check_state(crtc->dev);
> >
> > return ret;
> > }
> > @@ -10306,6 +10371,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
> >
> > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> > +
> > + INIT_WORK(&intel_crtc->enable_work, intel_crtc_enable_work);
> > + INIT_WORK(&intel_crtc->disable_work, intel_crtc_disable_work);
>
> I feel using independent work items (remember the global wq is really a
> pool of many wq) is horribly prone to deadlocks.
>
> We have the usual caveat that this has an implicit API change in that
> setcrtc can now return before the change is complete - and so userspace
> may write to a still currently visible scanout. Its not a huge issue
> (and is a change I am in favour of), it is just a change in behaviour we
> have to be wary of (which also means stating it in the changelog for future
> reference).
Yeah that's a good point, and if we're not careful it could result in
some visible ugliness.
--
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
