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

[Lankhorst, Maarten]

Daniel Vetter schreef op zo 26-02-2017 om 21:00 [+0100]:
> On Fri, Feb 24, 2017 at 12:52:53AM +, Pandiyan, Dhinakaran wrote:
> > 
> > On Thu, 2017-02-16 at 09:09 +, Lankhorst, Maarten wrote:
> > > 
> > > Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> > > > 
> > > > On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> > > >  wrote:
> > > > > 
> > > > > On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> > > > > > 
> > > > > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55
> > > > > > [+]:
> > > > > > > 
> > > > > > > > 
> > > > > > > > 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.
> > > 
> > 
> > Can we add this later when we need state validation that is
> > appropriate
> > for an ->atomic_check()? 
> 
> +1 on not solving problems we don't have yet :-) If we'd write code
> for
> every eventuality that we can come up with, we'd get nothing done.
> And
> ime, such unused code will also be full of bugs.
> 
> Discussing issues like this is still good and useful, just to make
> sure we
> have a coherent plan for the eventual future, once it happens.

Near future, I'm working on converting i915 specific connector
properties to atomic, and it would be nice if I had a connector atomic
check function like this. :)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Fri, Feb 24, 2017 at 12:52:53AM +, Pandiyan, Dhinakaran wrote:
> On Thu, 2017-02-16 at 09:09 +, Lankhorst, Maarten wrote:
> > Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> > > On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> > >  wrote:
> > > > On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> > > > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> > > > > > > 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.
> > 
> 
> Can we add this later when we need state validation that is appropriate
> for an ->atomic_check()? 

+1 on not solving problems we don't have yet :-) If we'd write code for
every eventuality that we can come up with, we'd get nothing done. And
ime, such unused code will also be full of bugs.

Discussing issues like this is still good and useful, just to make sure we
have a coherent plan for the eventual future, once it happens.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

On Thu, 2017-02-16 at 09:09 +, Lankhorst, Maarten wrote:
> Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> > On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> >  wrote:
> > > On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> > > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> > > > > On Thu, 2017-02-09 at 09:01 +, 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 
> > > > > > > Signed-off-by: Dhinakaran Pandiyan  > > > > > > 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.
> 

Can we add this later when we need state validation that is appropriate
for an ->atomic_check()? 

-DK

> 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 

Re: [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 [+]:
> On Mon, 2017-02-13 at 21:26 +, Pandiyan, Dhinakaran wrote:
> > 
> > On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> > > 
> > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> > > > 
> > > > On Thu, 2017-02-09 at 09:01 +, 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 
> > > > > > Signed-off-by: Dhinakaran Pandiyan  > > > > > 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Lankhorst, Maarten]

Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
>  wrote:
> > On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> > > > On Thu, 2017-02-09 at 09:01 +, 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 
> > > > > > Signed-off-by: Dhinakaran Pandiyan  > > > > > 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
> > > > > 

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

[Pandiyan, Dhinakaran]

On Tue, 2017-02-14 at 20:51 +0100, Daniel Vetter wrote:
> On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
>  wrote:
> > On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> >> Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> >> > On Thu, 2017-02-09 at 09:01 +, 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 
> >> > > > Signed-off-by: Dhinakaran Pandiyan  >> > > > >
> >> > > > ---
> >> > > >  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
> >> > > 

Re: [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
 wrote:
> On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
>> Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
>> > On Thu, 2017-02-09 at 09:01 +, 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 
>> > > > Signed-off-by: Dhinakaran Pandiyan > > > > >
>> > > > ---
>> > > >  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@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
>



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

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

[Pandiyan, Dhinakaran]

On Mon, 2017-02-13 at 21:26 +, Pandiyan, Dhinakaran wrote:
> On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> > > On Thu, 2017-02-09 at 09:01 +, 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 
> > > > > Signed-off-by: Dhinakaran Pandiyan  > > > > >
> > > > > ---
> > > > >  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.


-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

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

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

[Pandiyan, Dhinakaran]

On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> > On Thu, 2017-02-09 at 09:01 +, 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 
> > > > Signed-off-by: Dhinakaran Pandiyan  > > > >
> > > > ---
> > > >  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
> > > 
> > > 
> > > 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

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

[Lankhorst, Maarten]

Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> On Thu, 2017-02-09 at 09:01 +, 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 
> > > Signed-off-by: Dhinakaran Pandiyan  > > >
> > > ---
> > >  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
> 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) {
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

On Thu, 2017-02-09 at 09:01 +, 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 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  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()?


> 
> 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

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

[Lankhorst, Maarten]

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 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  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?

Maybe implementing the relevant VCPI state could be done as an atomic
helper function too, so other atomic drivers can just plug it in.

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] [PATCH v3 7/8] drm: Connector helper function to release resources

[Dhinakaran Pandiyan]

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 
Signed-off-by: Dhinakaran Pandiyan 
---
 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);
+   }
+
return mode_fixup(state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
diff --git a/include/drm/drm_modeset_helper_vtables.h 
b/include/drm/drm_modeset_helper_vtables.h
index 091c422..394ec0c 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -836,6 +836,19 @@ struct drm_connector_helper_funcs {
 */
struct drm_encoder *(*atomic_best_encoder)(struct drm_connector 
*connector,
   struct drm_connector_state 
*connector_state);
+
+   /**
+* @atomic_release:
+*
+* This function is used to release shared resources that were
+* previously acquired.
+*
+* NOTE:
+*
+* This function is called in the check phase of an atomic update.
+*/
+   void (*atomic_release)(struct drm_connector *connector,
+  struct drm_connector_state *connector_state);
 };
 
 /**
-- 
2.7.4

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