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

[Daniel Vetter]

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.

> 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
use-case filtering in disable will lead to hung hardware. Which really
means that you need to filter in commit_planes.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch