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

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

[-- Attachment #1.1: Type: text/plain, Size: 5762 bytes --]

Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> 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@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.
> 
> 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.

Having an atomic check would be nice for implementing connector
properties. Some of them may need to be validated regardless of crtc.

I would really like to be able to do the validation in atomic_check
instead of during the set_property callback. The state is not
completely valid at that point yet, so this would be a logical 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 :)

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3282 bytes --]

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

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