| Message ID | 20200212135936.31326-1-jsarha@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/tidss: dispc: Rewrite naive plane positioning code | expand |
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@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(). BR, Jyri -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@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. -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@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. Thanks, Jyri
On Wed, Feb 12, 2020 at 7:01 PM Jyri Sarha <jsarha@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@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). -Daniel
On 12/02/2020 22:28, Daniel Vetter wrote: > On Wed, Feb 12, 2020 at 7:01 PM Jyri Sarha <jsarha@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@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. 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. Best regards, Jyri
On Thu, Feb 13, 2020 at 10:03 AM Jyri Sarha <jsarha@ti.com> wrote: > > On 12/02/2020 22:28, Daniel Vetter wrote: > > On Wed, Feb 12, 2020 at 7:01 PM Jyri Sarha <jsarha@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@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 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. -Daniel
On 13/02/2020 11:19, Daniel Vetter wrote: > On Thu, Feb 13, 2020 at 10:03 AM Jyri Sarha <jsarha@ti.com> wrote: >> >> On 12/02/2020 22:28, Daniel Vetter wrote: >>> On Wed, Feb 12, 2020 at 7:01 PM Jyri Sarha <jsarha@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@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. 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
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@ti.com> wrote: > >> > >> On 12/02/2020 22:28, Daniel Vetter wrote: > >>> On Wed, Feb 12, 2020 at 7:01 PM Jyri Sarha <jsarha@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@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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@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@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.
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 @@ -17,6 +17,7 @@ #include "tidss_dispc.h" #include "tidss_drv.h" #include "tidss_irq.h" +#include "tidss_plane.h" /* Page flip and frame done IRQs */ @@ -93,6 +94,7 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc, u32 hw_videoport = tcrtc->hw_videoport; const struct drm_display_mode *mode; enum drm_mode_status ok; + int ret; dev_dbg(ddev->dev, "%s\n", __func__); @@ -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); +} + +static void tidss_crtc_position_planes(struct tidss_device *tidss, + struct drm_crtc *crtc, + struct drm_crtc_state *old_state, + bool newmodeset) +{ + struct drm_atomic_state *ostate = old_state->state; + struct tidss_crtc *tcrtc = to_tidss_crtc(crtc); + struct drm_crtc_state *cstate = crtc->state; + int zpos; + + if (!newmodeset && !cstate->zpos_changed && !cstate->planes_changed) + return; + + for (zpos = 0; zpos < tidss->feat->num_planes; zpos++) { + struct drm_plane_state *pstate; + struct drm_plane *plane; + bool zpos_taken = false; + int i; + + for_each_new_plane_in_state(ostate, plane, pstate, i) { + if (pstate->crtc != crtc || !pstate->visible) + continue; + + if (pstate->normalized_zpos == zpos) { + zpos_taken = true; + break; + } + } + + if (zpos_taken) { + struct tidss_plane *tplane = to_tidss_plane(plane); + + dispc_ovr_set_plane(tidss->dispc, tplane->hw_plane_id, + tcrtc->hw_videoport, + pstate->crtc_x, pstate->crtc_y, + zpos); + } + dispc_ovr_enable_layer(tidss->dispc, tcrtc->hw_videoport, zpos, + zpos_taken); + } } static void tidss_crtc_atomic_flush(struct drm_crtc *crtc, @@ -146,6 +195,9 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc, /* Write vp properties to HW if needed. */ dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state, false); + /* Update plane positions if needed. */ + tidss_crtc_position_planes(tidss, crtc, old_crtc_state, false); + WARN_ON(drm_crtc_vblank_get(crtc) != 0); spin_lock_irqsave(&ddev->event_lock, flags); @@ -183,6 +235,7 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc, return; dispc_vp_setup(tidss->dispc, tcrtc->hw_videoport, crtc->state, true); + tidss_crtc_position_planes(tidss, crtc, old_state, true); /* Turn vertical blanking interrupt reporting on. */ drm_crtc_vblank_on(crtc); diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c index eeb160dc047b..e79dad246b1e 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.c +++ b/drivers/gpu/drm/tidss/tidss_dispc.c @@ -281,11 +281,6 @@ struct dss_vp_data { u32 *gamma_table; }; -struct dss_plane_data { - u32 zorder; - u32 hw_videoport; -}; - struct dispc_device { struct tidss_device *tidss; struct device *dev; @@ -307,8 +302,6 @@ struct dispc_device { struct dss_vp_data vp_data[TIDSS_MAX_PORTS]; - struct dss_plane_data plane_data[TIDSS_MAX_PLANES]; - u32 *fourccs; u32 num_fourccs; @@ -1247,7 +1240,7 @@ int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport, /* OVR */ static void dispc_k2g_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane, u32 hw_videoport, - u32 x, u32 y, u32 zpos) + u32 x, u32 y, u32 layer) { /* On k2g there is only one plane and no need for ovr */ dispc_vid_write(dispc, hw_plane, DISPC_VID_K2G_POSITION, @@ -1256,44 +1249,43 @@ static void dispc_k2g_ovr_set_plane(struct dispc_device *dispc, static void dispc_am65x_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane, u32 hw_videoport, - u32 x, u32 y, u32 zpos) + u32 x, u32 y, u32 layer) { - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos), + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer), hw_plane, 4, 1); - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos), + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer), x, 17, 6); - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos), + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer), y, 30, 19); } static void dispc_j721e_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane, u32 hw_videoport, - u32 x, u32 y, u32 zpos) + u32 x, u32 y, u32 layer) { - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos), + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer), hw_plane, 4, 1); - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(zpos), + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(layer), x, 13, 0); - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(zpos), + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES2(layer), y, 29, 16); } -static void dispc_ovr_set_plane(struct dispc_device *dispc, - u32 hw_plane, u32 hw_videoport, - u32 x, u32 y, u32 zpos) +void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane, + u32 hw_videoport, u32 x, u32 y, u32 layer) { switch (dispc->feat->subrev) { case DISPC_K2G: dispc_k2g_ovr_set_plane(dispc, hw_plane, hw_videoport, - x, y, zpos); + x, y, layer); break; case DISPC_AM65X: dispc_am65x_ovr_set_plane(dispc, hw_plane, hw_videoport, - x, y, zpos); + x, y, layer); break; case DISPC_J721E: dispc_j721e_ovr_set_plane(dispc, hw_plane, hw_videoport, - x, y, zpos); + x, y, layer); break; default: WARN_ON(1); @@ -1301,10 +1293,13 @@ static void dispc_ovr_set_plane(struct dispc_device *dispc, } } -static void dispc_ovr_enable_plane(struct dispc_device *dispc, - u32 hw_videoport, u32 zpos, bool enable) +void dispc_ovr_enable_layer(struct dispc_device *dispc, + u32 hw_videoport, u32 layer, bool enable) { - OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(zpos), + if (dispc->feat->subrev == DISPC_K2G) + return; + + OVR_REG_FLD_MOD(dispc, hw_videoport, DISPC_OVR_ATTRIBUTES(layer), !!enable, 0, 0); } @@ -2070,21 +2065,11 @@ int dispc_plane_setup(struct dispc_device *dispc, u32 hw_plane, VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, 0, 28, 28); - dispc_ovr_set_plane(dispc, hw_plane, hw_videoport, - state->crtc_x, state->crtc_y, - state->normalized_zpos); - - dispc->plane_data[hw_plane].zorder = state->normalized_zpos; - dispc->plane_data[hw_plane].hw_videoport = hw_videoport; - return 0; } int dispc_plane_enable(struct dispc_device *dispc, u32 hw_plane, bool enable) { - dispc_ovr_enable_plane(dispc, dispc->plane_data[hw_plane].hw_videoport, - dispc->plane_data[hw_plane].zorder, enable); - VID_REG_FLD_MOD(dispc, hw_plane, DISPC_VID_ATTRIBUTES, !!enable, 0, 0); return 0; diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h index e65e6a2bb821..a4a68249e44b 100644 --- a/drivers/gpu/drm/tidss/tidss_dispc.h +++ b/drivers/gpu/drm/tidss/tidss_dispc.h @@ -94,6 +94,11 @@ extern const struct dispc_features dispc_j721e_feats; void dispc_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask); dispc_irq_t dispc_read_and_clear_irqstatus(struct dispc_device *dispc); +void dispc_ovr_set_plane(struct dispc_device *dispc, u32 hw_plane, + u32 hw_videoport, u32 x, u32 y, u32 layer); +void dispc_ovr_enable_layer(struct dispc_device *dispc, + u32 hw_videoport, u32 layer, bool enable); + void dispc_vp_prepare(struct dispc_device *dispc, u32 hw_videoport, const struct drm_crtc_state *state); void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
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@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(-) -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel