[PATCH v2] drm/tidss: dispc: Rewrite naive plane positioning code

[Daniel Vetter]

On Thu, Feb 13, 2020 at 12:03:51PM +0200, Jyri Sarha wrote:
> On 13/02/2020 11:19, Daniel Vetter wrote:
> > On Thu, Feb 13, 2020 at 10:03 AM Jyri Sarha <jsarha at ti.com> wrote:
> >>
> >> On 12/02/2020 22:28, Daniel Vetter wrote:
> >>> On Wed, Feb 12, 2020 at 7:01 PM Jyri Sarha <jsarha at ti.com> wrote:
> >>>>
> >>>> On 12/02/2020 16:33, Ville Syrjälä wrote:
> >>>>> On Wed, Feb 12, 2020 at 04:08:11PM +0200, Jyri Sarha wrote:
> >>>>>> On 12/02/2020 15:59, Jyri Sarha wrote:
> >>>>>>> The old implementation of placing planes on the CRTC while configuring
> >>>>>>> the planes was naive and relied on the order in which the planes were
> >>>>>>> configured, enabled, and disabled. The situation where a plane's zpos
> >>>>>>> was changed on the fly was completely broken. The usual symptoms of
> >>>>>>> this problem was scrambled display and a flood of sync lost errors,
> >>>>>>> when a plane was active in two layers at the same time, or a missing
> >>>>>>> plane, in case when a layer was accidentally disabled.
> >>>>>>>
> >>>>>>> The rewrite takes a more straight forward approach when HW is
> >>>>>>> concerned. The plane positioning registers are in the CRTC (actually
> >>>>>>> OVR) register space and it is more natural to configure them in one go
> >>>>>>> while configuring the CRTC. To do this we need to make sure we have
> >>>>>>> all the planes on updated CRTCs in the new atomic state to be
> >>>>>>> committed. This is done by calling drm_atomic_add_affected_planes() in
> >>>>>>> crtc_atomic_check().
> >>>>>>>
> >>>>>>> Signed-off-by: Jyri Sarha <jsarha at ti.com>
> >>>>>>> ---
> >>>>>>>  drivers/gpu/drm/tidss/tidss_crtc.c  | 55 ++++++++++++++++++++++++++++-
> >>>>>>>  drivers/gpu/drm/tidss/tidss_dispc.c | 55 +++++++++++------------------
> >>>>>>>  drivers/gpu/drm/tidss/tidss_dispc.h |  5 +++
> >>>>>>>  3 files changed, 79 insertions(+), 36 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> >>>>>>> index 032c31ee2820..f7c5fd1094a8 100644
> >>>>>>> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> >>>>>>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> >>>>>> ...
> >>>>>>> @@ -108,7 +110,54 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
> >>>>>>>             return -EINVAL;
> >>>>>>>     }
> >>>>>>>
> >>>>>>> -   return dispc_vp_bus_check(dispc, hw_videoport, state);
> >>>>>>> +   ret = dispc_vp_bus_check(dispc, hw_videoport, state);
> >>>>>>> +   if (ret)
> >>>>>>> +           return ret;
> >>>>>>> +
> >>>>>>> +   /* Add unchanged planes on this crtc to state for zpos update. */
> >>>>>>> +   return drm_atomic_add_affected_planes(state->state, crtc);
> >>>>>>
> >>>>>> Is this a correct way to use drm_atomic_add_affected_planes()?
> >>>>>>
> >>>>>> I saw that some other drivers implement their own mode_config
> >>>>>> atomic_check() and have this call there in
> >>>>>> for_each_new_crtc_in_state()-loop, but I thought it should be fine to
> >>>>>> call it in crtc_atomic_check().
> >>>>>
> >>>>> You seem to be using drm_atomic_helper_check_planes(), which means
> >>>>> crtc.atomic_check() gets called after plane.atomic_check(). So this
> >>>>> might be good or bad depending on whether you'd like the planes you
> >>>>> add here to go through their .atomic_check() or not.
> >>>>>
> >>>>
> >>>> Should have thought of that my self. Extra plane.atomic_check() calls do
> >>>> not do any actual harm, but they are potentially expensive. The planes
> >>>> are really only needed there in the commit phase (crtc_enable() or
> >>>> flush()). Well, I'll do my own mode_config.atomic_check() and call
> >>>> drm_atomic_add_affected_planes() in a for_each_new_crtc_in_state()-loop
> >>>> after all the checks.
> >>>
> >>> Also, if you do use the helpers then this should already have happened
> >>> for you. Which makes me wonder why all this work, so maybe there's
> >>> some dependency between all the various check functions you have going
> >>> on? Might be time to roll your own top-level check function that calls
> >>
> >>> stuff in the order your hw needs things to be checked, so that you
> >>> don't add new states late and have to run one check phase twice (which
> >>> is totally fine with the helpers as long as all your check callbacks
> >>> are idempotent, but often just overkill and confusing).
> >>
> >> All the driver specific checks are perfectly independent without any
> >> cross dependencies. And there is no extra data in the plane- or
> >> CRTC-state, nor do the driver specific checks touch the state in any way.
> >>
> >> I only want all the planes on a crtc to be there, if any of the planes
> >> on the CRTC changes, so that I can write the plane positions from the
> >> scratch directly from the atomic state with each commit.
> > 
> > You /should/ get this already with the atomic helpers, no further
> > action needed. Does it not work?
> > 
> >> Long explanation (if you are interested):
> >>
> >> With the DSS HW the planes are positioned to CRTCs by filling each
> >> plane's id and x,y position to correct overlay register according to the
> >> plane's zpos (each overlay reg has its own static zpos).
> >>
> >> Using the naive implementation, there is a problem if I have plane0 at
> >> zpos0 and another commit comes with plane1 at zpos0 and plane0 disabled.
> >> If plane1 in the commit is handled first, it overwrites plane0's
> >> previous position, and plane0 handled afterwards will disable freshly
> >> configured plane1. There is number of other problematic cases.
> >>
> >> Ok, I can easily fix this by storing the plane positions (x,y, and z) in
> >> the driver's internal state, and rolling the positions out in the
> >> crtc_enable() or -flush(). But I have understood the atomic drivers
> >> should avoid having any internal state. So the obvious choice would be
> >> to roll out the plane positions directly from the atomic state. However,
> >> there is a problem with that.
> > 
> > Hm I'm not entirely following the problem, but if you have some
> > requirements around the order in which your planes need to be
> > committed, then roll your own plane commit code. At least for the
> > parts of the plane state which need to be committed like that. You can
> > then stuff that into one of your crtc commit functions.
> > 
> > And I guess "commit planes in order of zpos" is probably fairly common
> > driver requirement, we might want to have a shared iterator for that.
> > 
> > Aside: Driver private state is totally fine. Just needs to be attached
> > to one of the drm_*_state objects (you can have private state objects
> > too). What's not ok in atomic is stuffing that kind of data into your
> > drm_crtc structure (or somewhere else like that), that's where the
> > problems happen.
> > 
> 
> The root cause is the plane position being part of the plane-state, but
> in fact the positions being programmed into CRTC registers. I can figure
> out all kind of strategies to deal with situation in plane commit code.
> However, the simplest solution is to write all plane positions from the
> scratch with each commit to all CRTCs with changed planes.
> 
> >> The problem appears when I have more than one plane active on a crtc and
> >> I just need to update one of the planes. In the situation nothing
> >> changes about the CRTC and the unchanged planes, so it is quite
> >> understandable that the helpers do not add the unchanged planes to the
> >> atomic state. But if I want to roll out the new plane positions from the
> >> scratch with every commit that touches any plane on that CRTC, I need to
> >> have the unchanged planes there.
> >>
> >> So the drm_atomic_add_affected_planes()-calls are added just to avoid
> >> any internal plane position state in the driver.
> > 
> > Again, this should all happen already.
> 
> drm_atomic_helper_check() is supposed to add all the planes on the same
> CRTC in the atomic state, if one plane on that CRTC changes?
> 
> If that should be the case, then there is a bug somewhere.

Sry I misread, it only adds it for the case when there's a modeset. If you
need it in more cases (like when zpos changes), you indeed have to do that
yourself. Apologies for the confusion, Tom helped me clear this up with a
quick chat on irc.

Tom said you're working on a patch that calls add_affected_planes at the
right spot in atomic_check, and it sounds like that's all you really need.
-Daniel

> My test changes a property in a single plane, while another plane is
> active on the same CRTC. When I loop the planes with
> for_each_new_plane_in_state() in the crtc_flush() I can only find the
> updated plane.
> 
> Best regards,
> Jyri
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch