On Tue, Jun 11, 2019 at 10:56:45PM +0200, Sam Ravnborg wrote:
> Hi Sean.
>
> Small things here and there. Did not stare at this long enough to
> understand the code, but added some feedback anyway.
Thanks for the comments, Sam, I'll send a revision shortly
>
> Sam
> >
/snip
> > +static void drm_self_refresh_helper_entry_work(struct work_struct *work)
> > +{
> > + struct drm_self_refresh_data *sr_data = container_of(
> > + to_delayed_work(work),
> > + struct drm_self_refresh_data, entry_work);
> > + struct drm_crtc *crtc = sr_data->crtc;
> > + struct drm_device *dev = crtc->dev;
> > + struct drm_modeset_acquire_ctx ctx;
> > + struct drm_atomic_state *state;
> > + struct drm_connector *conn;
> > + struct drm_connector_state *conn_state;
> > + struct drm_crtc_state *crtc_state;
> > + int i, ret;
> This function is called from a workqueue.
> Just wondering if this require any locking?
Yes, it does. The locks are acquired in the various drm_atomic_get_*_state()
function calls and dropped below in the out label (drm_modeset_drop_locks).
> (Maybe I missed it, browsed the code without a detailed review)
>
> > +
> > + drm_modeset_acquire_init(&ctx, 0);
> > +
> > + state = drm_atomic_state_alloc(dev);
> > + if (!state) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > +retry:
> > + state->acquire_ctx = &ctx;
> > +
> > + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > + if (IS_ERR(crtc_state)) {
> > + ret = PTR_ERR(crtc_state);
> > + goto out;
> > + }
> > +
> > + if (!crtc_state->enable)
> > + goto out;
> > +
> > + ret = drm_atomic_add_affected_connectors(state, crtc);
> > + if (ret)
> > + goto out;
> > +
> > + for_each_new_connector_in_state(state, conn, conn_state, i) {
> > + if (!conn_state->self_refresh_aware)
> > + goto out;
> > + }
> > +
> > + crtc_state->active = false;
> > + crtc_state->self_refresh_active = true;
> > +
> > + ret = drm_atomic_commit(state);
> > + if (ret)
> > + goto out;
> > +
> > +out:
> > + if (ret == -EDEADLK) {
> > + drm_atomic_state_clear(state);
> > + ret = drm_modeset_backoff(&ctx);
> > + if (!ret)
> > + goto retry;
> > + }
> > +
> > + drm_atomic_state_put(state);
> > + drm_modeset_drop_locks(&ctx);
> > + drm_modeset_acquire_fini(&ctx);
> > +}
> > +
/snip
--
Sean Paul, Software Engineer, Google / Chromium OS
