Re: [PATCH v5 05/11] drm: Add helpers to kick off self refresh mode in drivers

From: Sean Paul <sean@poorly.run>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Sean Paul <sean@poorly.run>,
	dri-devel@lists.freedesktop.org, Zain Wang <wzz@rock-chips.com>,
	Jonathan Corbet <corbet@lwn.net>, David Airlie <airlied@linux.ie>,
	linux-doc@vger.kernel.org, Jose Souza <jose.souza@intel.com>,
	Tomasz Figa <tfiga@chromium.org>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Sean Paul <seanpaul@chromium.org>
Subject: Re: [PATCH v5 05/11] drm: Add helpers to kick off self refresh mode in drivers
Date: Wed, 12 Jun 2019 10:19:26 -0400	[thread overview]
Message-ID: <20190612141926.GE179831@art_vandelay> (raw)
In-Reply-To: <20190611205645.GB18315@ravnborg.org>

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