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

["Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>]

On Tue, 2017-02-14 at 20:51 +0100, Daniel Vetter wrote:
> On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> 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@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.
> >
> >
> > 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.
> 
> Adding ->atomic_check that's unconditionally called sounds troubling,
> because all the other ->atomic_check functions are _only_ called when
> enabling stuff. ->atomic_release sounds much better to me, and from a
> helper pov DK's patch above is the right place.
> 
> If that place doesn't work for i915.ko, then we need our own callback
> (like we already have with e.g. ->compute_config, we could do a
> ->release_config). But if it's just cosmetics, then I don't see the
> reason why we need to change this. On that issue: How exactly does our
> compute_config work if we haven't updated the routing (using the above
> helper) yet? That sounds like a pretty big misdesign on our side ...
> -Daniel
> 
Are you referring to the drm_atomic_helper_check_modeset() helper? We do
call it to update connector routing before compute_config .

-DK

> >
> > -DK
> >> > >
> >> > >
> >> > > 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
> >> >
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> 
> 

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