[PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Laurent Pinchart]

Hi Daniel,

On Monday 06 Jun 2016 04:14:59 Laurent Pinchart wrote:
> On Wednesday 11 May 2016 09:37:56 Daniel Vetter wrote:
> > On Tue, May 10, 2016 at 04:24:11PM +0300, Tomi Valkeinen wrote:
> >> On 26/04/16 23:35, Laurent Pinchart wrote:
> >>> Instead of conditioning planes update based on the hardware device
> >>> state, use the CRTC state stored in the atomic state. This reduces the
> >>> dependency from the DRM layer to the DSS layer.
> >>> 
> >>> Signed-off-by: Laurent Pinchart 
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++-
> >>>  1 file changed, 14 insertions(+), 9 deletions(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6359d7933b93..4c56d6a68390
> >>> 100644
> >>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct
> >>> drm_crtc *crtc,
> >>> 
> >>>   WARN_ON(omap_crtc->vblank_irq.registered);
> >>> 
> >>> - if (dispc_mgr_is_enabled(omap_crtc->channel)) {
> >>> + /*
> >>> +  * Only flush the CRTC if it is currently active. CRTCs that require
> >>> a
> >>> +  * mode set are disabled prior plane updates and enabled afterwards.
> >>> +  * They are thus not active, regardless of what their state report.
> >>> +  */
> >>> + if (!crtc->state->active ||
> >>> drm_atomic_crtc_needs_modeset(crtc->state))
> >>> + return;
> >> 
> >> If the DRM core doesn't track whether a CRTC HW is enabled at the
> >> moment, maybe omapdrm should? I guess the above works, but that if()
> >> makes me a bit uneasy, as it's not really obvious, and the logic behind
> >> it could possibly change later...
> >> 
> >> A "if (crtc->is_hw_enabled)" would be much more readable.
> 
> The whole point of this patch is to remove local state and rely on DRM core
> state, so I'd like to avoid that if possible.
> 
> > Look at the active_only paramater of the planes_commit helper. That should
> > do exactly what you want.
> 
> The drm_atomic_helper_commit_planes() helper has this check
> 
> if (active_only && !crtc->state->active)
> continue;
> 
> funcs->atomic_flush(crtc, old_crtc_state);
> 
> I could thus remove the !crtc->state->active check, but the
> drm_atomic_crtc_needs_modeset() check would need to stay, wouldn't it ? When
> CRTCs go through a modeset they are disabled prior to plane updates, but
> their state active status can still be true. Or should that be fixed in the
> core ?

Any comment on this ? Knowing whether the CRTC hardware is enabled is helpful 
to implement the flush() function. Would it make sense to add a new "is 
hardware enabled" flag to the CRTC structure (or possibly the CRTC state 
structure, but given that there is by definition a single instance of the 
hardware state the CRTC structure would make more sense) ?

-- 
Regards,

Laurent Pinchart

[PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Laurent Pinchart]

Hi Tomi,

On Monday 06 Jun 2016 13:37:13 Tomi Valkeinen wrote:
> On 06/06/16 04:14, Laurent Pinchart wrote:
> >>> If the DRM core doesn't track whether a CRTC HW is enabled at the
> >>> moment, maybe omapdrm should? I guess the above works, but that if()
> >>> makes me a bit uneasy, as it's not really obvious, and the logic behind
> >>> it could possibly change later...
> >>> 
> >>> A "if (crtc->is_hw_enabled)" would be much more readable.
> > 
> > The whole point of this patch is to remove local state and rely on DRM
> > core state, so I'd like to avoid that if possible.
> 
> Yep, but if DRM core doesn't give that information...
> 
> Using drm_atomic_crtc_needs_modeset() to check if a crtc is enabled at a
> particular point in the commit sequence feels a bit risky to me.
> 
> You do explain it in the comment, so it's not that bad, but I'd still
> rather see an 'if (is-the-hw-enabled-or-not)' than looking at seemingly
> unrelated information, and deducing from that if the hw is enabled or not.

I agree, but I'd like that "is-the-hw-enabled-or-not" state to be provided by 
the DRM core, not the omapdrm driver. I'll see how we can get there, but I'd 
rather do that separately from this patch series as it will require very 
careful design and review.

-- 
Regards,

Laurent Pinchart

[PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Tomi Valkeinen]

On 06/06/16 04:14, Laurent Pinchart wrote:

>>> If the DRM core doesn't track whether a CRTC HW is enabled at the
>>> moment, maybe omapdrm should? I guess the above works, but that if()
>>> makes me a bit uneasy, as it's not really obvious, and the logic behind
>>> it could possibly change later...
>>>
>>> A "if (crtc->is_hw_enabled)" would be much more readable.
> 
> The whole point of this patch is to remove local state and rely on DRM core 
> state, so I'd like to avoid that if possible.

Yep, but if DRM core doesn't give that information...

Using drm_atomic_crtc_needs_modeset() to check if a crtc is enabled at a
particular point in the commit sequence feels a bit risky to me.

You do explain it in the comment, so it's not that bad, but I'd still
rather see an 'if (is-the-hw-enabled-or-not)' than looking at seemingly
unrelated information, and deducing from that if the hw is enabled or not.

 Tomi

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 

[PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Laurent Pinchart]

Hi Tomi and Daniel,

On Wednesday 11 May 2016 09:37:56 Daniel Vetter wrote:
> On Tue, May 10, 2016 at 04:24:11PM +0300, Tomi Valkeinen wrote:
> > On 26/04/16 23:35, Laurent Pinchart wrote:
> >> Instead of conditioning planes update based on the hardware device
> >> state, use the CRTC state stored in the atomic state. This reduces the
> >> dependency from the DRM layer to the DSS layer.
> >> 
> >> Signed-off-by: Laurent Pinchart 
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++-
> >>  1 file changed, 14 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6359d7933b93..4c56d6a68390
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc
> >> *crtc,
> >>WARN_ON(omap_crtc->vblank_irq.registered);
> >> 
> >> -  if (dispc_mgr_is_enabled(omap_crtc->channel)) {
> >> +  /*
> >> +   * Only flush the CRTC if it is currently active. CRTCs that require a
> >> +   * mode set are disabled prior plane updates and enabled afterwards.
> >> +   * They are thus not active, regardless of what their state report.
> >> +   */
> >> +  if (!crtc->state->active ||
> >> drm_atomic_crtc_needs_modeset(crtc->state))
> >> +  return;
> > 
> > If the DRM core doesn't track whether a CRTC HW is enabled at the
> > moment, maybe omapdrm should? I guess the above works, but that if()
> > makes me a bit uneasy, as it's not really obvious, and the logic behind
> > it could possibly change later...
> > 
> > A "if (crtc->is_hw_enabled)" would be much more readable.

The whole point of this patch is to remove local state and rely on DRM core 
state, so I'd like to avoid that if possible.

> Look at the active_only paramater of the planes_commit helper. That should
> do exactly what you want.

The drm_atomic_helper_commit_planes() helper has this check

if (active_only && !crtc->state->active)
continue;

funcs->atomic_flush(crtc, old_crtc_state);

I could thus remove the !crtc->state->active check, but the 
drm_atomic_crtc_needs_modeset() check would need to stay, wouldn't it ? When 
CRTCs go through a modeset they are disabled prior to plane updates, but their 
state active status can still be true. Or should that be fixed in the core ?

-- 
Regards,

Laurent Pinchart

[PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Daniel Vetter]

On Tue, May 10, 2016 at 04:24:11PM +0300, Tomi Valkeinen wrote:
> 
> On 26/04/16 23:35, Laurent Pinchart wrote:
> > Instead of conditioning planes update based on the hardware device
> > state, use the CRTC state stored in the atomic state. This reduces the
> > dependency from the DRM layer to the DSS layer.
> > 
> > Signed-off-by: Laurent Pinchart 
> > ---
> >  drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++-
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c 
> > b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > index 6359d7933b93..4c56d6a68390 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc 
> > *crtc,
> >  
> > WARN_ON(omap_crtc->vblank_irq.registered);
> >  
> > -   if (dispc_mgr_is_enabled(omap_crtc->channel)) {
> > +   /*
> > +* Only flush the CRTC if it is currently active. CRTCs that require a
> > +* mode set are disabled prior plane updates and enabled afterwards.
> > +* They are thus not active, regardless of what their state report.
> > +*/
> > +   if (!crtc->state->active || drm_atomic_crtc_needs_modeset(crtc->state))
> > +   return;
> 
> If the DRM core doesn't track whether a CRTC HW is enabled at the
> moment, maybe omapdrm should? I guess the above works, but that if()
> makes me a bit uneasy, as it's not really obvious, and the logic behind
> it could possibly change later...
> 
> A "if (crtc->is_hw_enabled)" would be much more readable.

Look at the active_only paramater of the planes_commit helper. That should
do exactly what you want.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Tomi Valkeinen]

On 26/04/16 23:35, Laurent Pinchart wrote:
> Instead of conditioning planes update based on the hardware device
> state, use the CRTC state stored in the atomic state. This reduces the
> dependency from the DRM layer to the DSS layer.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++-
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c 
> b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 6359d7933b93..4c56d6a68390 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc 
> *crtc,
>  
>   WARN_ON(omap_crtc->vblank_irq.registered);
>  
> - if (dispc_mgr_is_enabled(omap_crtc->channel)) {
> + /*
> +  * Only flush the CRTC if it is currently active. CRTCs that require a
> +  * mode set are disabled prior plane updates and enabled afterwards.
> +  * They are thus not active, regardless of what their state report.
> +  */
> + if (!crtc->state->active || drm_atomic_crtc_needs_modeset(crtc->state))
> + return;

If the DRM core doesn't track whether a CRTC HW is enabled at the
moment, maybe omapdrm should? I guess the above works, but that if()
makes me a bit uneasy, as it's not really obvious, and the logic behind
it could possibly change later...

A "if (crtc->is_hw_enabled)" would be much more readable.

 Tomi

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 

[PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Laurent Pinchart]

Instead of conditioning planes update based on the hardware device
state, use the CRTC state stored in the atomic state. This reduces the
dependency from the DRM layer to the DSS layer.

Signed-off-by: Laurent Pinchart 
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++-
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c 
b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 6359d7933b93..4c56d6a68390 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,

WARN_ON(omap_crtc->vblank_irq.registered);

-   if (dispc_mgr_is_enabled(omap_crtc->channel)) {
+   /*
+* Only flush the CRTC if it is currently active. CRTCs that require a
+* mode set are disabled prior plane updates and enabled afterwards.
+* They are thus not active, regardless of what their state report.
+*/
+   if (!crtc->state->active || drm_atomic_crtc_needs_modeset(crtc->state))
+   return;

-   DBG("%s: GO", omap_crtc->name);
+   DBG("%s: GO", omap_crtc->name);

-   rmb();
-   WARN_ON(omap_crtc->pending);
-   omap_crtc->pending = true;
-   wmb();
+   rmb();
+   WARN_ON(omap_crtc->pending);
+   omap_crtc->pending = true;
+   wmb();

-   dispc_mgr_go(omap_crtc->channel);
-   omap_irq_register(crtc->dev, _crtc->vblank_irq);
-   }
+   dispc_mgr_go(omap_crtc->channel);
+   omap_irq_register(crtc->dev, _crtc->vblank_irq);
 }

 static bool omap_crtc_is_plane_prop(struct drm_device *dev,
-- 
2.7.3