[PATCH v3 3/3] drm/imx: Add active plane reconfiguration support

[Daniel Vetter]

On Mon, Aug 22, 2016 at 04:23:36PM +0800, Ying Liu wrote:
> On Mon, Aug 22, 2016 at 3:53 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Mon, Aug 22, 2016 at 9:43 AM, Ying Liu <gnuiyl at gmail.com> wrote:
> >>>> +
> >>>> +     /*
> >>>> +      * The relevant plane's ->atomic_check callback may set
> >>>> +      * crtc_state->mode_changed to be true when the active
> >>>> +      * plane needs to be reconfigured.  In this case and only
> >>>> +      * this case, active_changed is false - we disable all the
> >>>> +      * appropriate active planes here.
> >>>> +      */
> >>>> +     if (!crtc->state->active_changed) {
> >>>> +             struct drm_plane *plane;
> >>>> +
> >>>> +             drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) {
> >>>> +                     const struct drm_plane_helper_funcs *plane_funcs =
> >>>> +                             plane->helper_private;
> >>>> +
> >>>> +                     /*
> >>>> +                      * Filter out the plane which is explicitly required
> >>>> +                      * to be disabled by the user via atomic commit
> >>>> +                      * so that it won't be accidentally disabled twice.
> >>>> +                      */
> >>>> +                     if (!plane->state->crtc)
> >>>> +                             continue;
> >>>
> >>> I think the better place would be to do the filtering in commit_planes(),
> >>> not here. And would be great to include your patch to fix up
> >>> disable_planes_on_crtc(), too.
> >>
> >> Do you mean we can do the filtering by checking (!plane->state->crtc)
> >> right before plane_funcs->atomic_disable in commit_planes()?
> >> Won't it filter out all the apples?
> >> commit_planes() doesn't know whether the driver calls
> >> disable_planes_on_crtc() or not.
> >
> > You need to filter on needs_modeset(crtc_state), and it needs to be
> > optional like active_only, using a flag.
> 
> Then, IIUC, the flag will be CRTC specific and be dynamically
> changeable. I'm not clear about the way to implement this.
> 
> >
> >> imo, doing the filtering in crtc_funcs->atomic_disable is sane.
> >
> > It is not sane in general, since usually you need to call
> > disable_planes_on_crtc to avoid upsetting your hardware. And for that
> 
> Calling disable_planes_on_crtc() in crtc_funcs->atomic_disable is
> a bit over-kill and will cause the issue of disabling plane twice, unless
> the filtering is done in disable_planes_on_crtc() (which was rejected
> by you on irc) or in commit_planes()?(which I'm not clear about
> the way to implement, as I mentioned before).  So I chose to do the
> filtering in the imx-drm specific crtc_funcs->atomic_disable function.

For filtering in commit_planes I think the best plane is to
- add a new flag NO_DISABLE_AFTER_MODESET
- which does exactly what it says on the tin: If the commmit_planes helper
  would call plane_funcs->atomic_disable, but
  drm_atomic_crtc_needs_modeset() for that crtc is true then you skip the
  ->atomic_disable call.

This assumes that all disables have already been committed when disabling
the crtc. For a lot of hardware it only makes sense to set both this flag
and ACTIVE_ONLY, but there's probably some fun hardware out there where
this is not the case (yours seems to be one example).

Now it's not pretty to have 2 boolean arguments for the same function, so
we also need to convert that into a bitflag field. End result:

#define COMMIT_ACTIVE_ONLY		BIT(0)
#define	COMMIT_NO_DISABLE_AFTER_MODESET	BIT(1)

Ofc kernel-doc should explain what the flags are for and when to use them.

void drm_atomic_helper_commit_planes(struct drm_device *dev,
				     struct drm_atomic_state *state,
				     unsigned flags);

A bit more work, but I think overall worth it.

> > use-case filtering in disable will lead to hung hardware. Which really
> 
> Hung hardware due to filtering? How?

If you shut down the crtc and keep the planes running the hw dies. Yup,
this happens.

> > means that you need to filter in commit_planes.
> 
> Really don't understand the rational for filtering in commit_planes...
> Did I miss anything?

See above, it might work for your driver, but it definitely restricts the
usefulness of the helpers in general. And helpers which are only useful
for 1 driver aren't useful.

I hope this explains my idea a bit better, otherwise probably best if you
ping me on irc.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch