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

[Laurent Pinchart]

Hi Jyri,

On Thu, Feb 13, 2020 at 11:03:15AM +0200, Jyri Sarha 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.
> 
> 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.

Atomic drivers should not store state in internal structures, but they
can subclass the standard state structures and add data there. You
could, in your atomic_check implementation, compute the values of the
CRTC registers that govern plane positioning, store them in your
subsclas of drm_crtc_state, and then just apply them in the CRTC atomic
flush.

> 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.

-- 
Regards,

Laurent Pinchart