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

[Ying Liu]

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.

> use-case filtering in disable will lead to hung hardware. Which really

Hung hardware due to filtering? How?

> means that you need to filter in commit_planes.

Really don't understand the rational for filtering in commit_planes...
Did I miss anything?

Regards,
Liu Ying

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