[Intel-gfx] [PATCH v3 7/8] drm: Connector helper function to release resources

[Lankhorst, Maarten]

Pandiyan, Dhinakaran schreef op ma 13-02-2017 om 22:48 [+0000]:
> On Mon, 2017-02-13 at 21:26 +0000, Pandiyan, Dhinakaran wrote:
> > 
> > On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
> > > 
> > > 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 at ffwll.ch>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at int
> > > > > > el.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.
> > 
> > 
> > Adding and unconditionally calling conn_funcs->atomic_check()
> > should be
> > doable. It also follows the pattern we have for encoders and CRTCs.
> > But
> > I'll have to move the connector->state->crtc state checks inside
> > the
> > function.
> > 
> > -DK
> 
> 
> This is what I mean -https://pastebin.ubuntu.com/23991405/
> But, I do have one concern with calling this conn_func-
> >atomic_check().
> We are not validating the new connector_state like atomic_check()
> seems
> to do generally but only cleaning up vcpi resources for
> compute_config()
> to later acquire. Let me know if I am wrong in my understanding what
> atomic_check() is expected to do.

Yeah looks good. I think it makes sense to have such a validation
function. There may not be much in it now but that could change when
i915 connector properties are made atomic. :)

~Maarten