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@xxxxxx> 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@xxxxxx> > >>>>> --- > >>>>> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
