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

[Daniel Vetter]

On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
<dhinakaran.pandiyan at 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 at ffwll.ch>
>> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at 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


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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch