Re: [PATCH v3 7/8] drm: Connector helper function to release resources

["Lankhorst, Maarten" <maarten.lankhorst@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+0000]:
> On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
> > 
> > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> > > 
> > > Having a ->atomic_release callback is useful to release shared
> > > resources
> > > that get allocated in compute_config(). This function is expected
> > > to
> > > be
> > > called in the atomic_check() phase before new resources are
> > > acquired.
> > > 
> > > v2: Moved the caller hunk to this patch (Daniel)
> > > 
> > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com
> > > >
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c      | 19
> > > +++++++++++++++++++
> > >  include/drm/drm_modeset_helper_vtables.h | 13 +++++++++++++
> > >  2 files changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 8795088..92bd741 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > drm_device *dev,
> > >  		}
> > >  	}
> > >  
> > > +	for_each_connector_in_state(state, connector,
> > > connector_state, i) {
> > > +		const struct drm_connector_helper_funcs
> > > *conn_funcs;
> > > +		struct drm_crtc_state *crtc_state;
> > > +
> > > +		conn_funcs = connector->helper_private;
> > > +		if (!conn_funcs->atomic_release)
> > > +			continue;
> > > +
> > > +		if (!connector->state->crtc)
> > > +			continue;
> > > +
> > > +		crtc_state =
> > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > >crtc);
> > > +
> > > +		if (crtc_state->connectors_changed ||
> > > +		    crtc_state->mode_changed ||
> > > +		    (crtc_state->active_changed && !crtc_state-
> > > > 
> > > > active))
> > > +			conn_funcs->atomic_release(connector,
> > > connector_state);
> > > +	}
> > 
> > Could we deal with the VCPI state separately in
> > intel_modeset_checks,
> > like we do with dpll?
> 
> We'd want to release the VCPI slots before they are acquired in
> ->compute_config(). intel_modeset_checks() will be too late to
> release
> them. Are you suggesting both acquiring and releasing slots should be
> done in intel_modeset_checks()?

That makes things a bit more nasty. Maybe add a
conn_funcs->atomic_check that always gets called, something like I did
below?

I'd love to use it for some atomic connector properties too.
> > 
> > 
> > Maybe implementing the relevant VCPI state could be done as an
> > atomic
> > helper function too, so other atomic drivers can just plug it in.
> > 
> The idea was to reduce boilerplate in the drivers and use the
> private_obj state for different object types.
> 
> 
> > 
> > Not sure how doable this is, but if it's not too hard, then it's
> > probably cleaner :)
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-patch; name="patch", Size: 1579 bytes --]

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 39cbacd8cd07..1e5f0a95c685 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -381,11 +381,24 @@ mode_fixup(struct drm_atomic_state *state)
 
 	for_each_new_connector_in_state(state, connector, conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
+		const struct drm_connector_helper_funcs *conn_funcs;
 		struct drm_encoder *encoder;
 
+		conn_funcs = connector->helper_private;
+
 		WARN_ON(!!conn_state->best_encoder != !!conn_state->crtc);
 
-		if (!conn_state->crtc || !conn_state->best_encoder)
+		if (!conn_state->crtc || !conn_state->best_encoder) {
+			if (conn_funcs && conn_funcs->atomic_check) {
+				ret = conn_funcs->atomic_check(connector, conn_state);
+
+				if (ret) {
+					DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] check failed\n",
+							 connector->base.id, connector->name);
+					return ret;
+				}
+			}
+
 			continue;
 
 		crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
@@ -404,7 +417,15 @@ mode_fixup(struct drm_atomic_state *state)
 			return -EINVAL;
 		}
 
-		if (funcs && funcs->atomic_check) {
+		if (conn_funcs && conn_funcs->atomic_check) {
+			ret = conn_funcs->atomic_check(connector, conn_state);
+
+			if (ret) {
+				DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] check failed\n",
+						  connector->base.id, connector->name);
+				return ret;
+			}
+		} else if (funcs && funcs->atomic_check) {
 			ret = funcs->atomic_check(encoder, crtc_state,
 						  conn_state);
 			if (ret) {

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx