| Message ID | a6dfec7ea3598c64f6653e9b8b5febac79172341.1499956639.git-series.maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi, On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > In the earlier display engine designs, any register access while a commit > is pending is forbidden. > > One of the symptoms is that reading a register will return another, random, > register value which can lead to register corruptions if we ever do a > read/modify/write cycle. Alternatively, if changes to the backend (layers) are guaranteed to happen while the CRTC is disabled (which seems to be the case after looking at drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we could just turn on register auto-commit all the time and not deal with this. ChenYu > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/gpu/drm/sun4i/sun4i_backend.c | 14 ++++++++++++++ > drivers/gpu/drm/sun4i/sun4i_crtc.c | 1 + > 2 files changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c > index cf480218daa5..ce1f40f7511e 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c > @@ -67,6 +67,19 @@ static void sun4i_backend_commit(struct sunxi_engine *engine) > SUN4I_BACKEND_REGBUFFCTL_LOADCTL); > } > > +static int sun4i_backend_commit_poll(struct sunxi_engine *engine) > +{ > + u32 val; > + > + DRM_DEBUG_DRIVER("Polling for the commit to end\n"); > + > + return regmap_read_poll_timeout(engine->regs, > + SUN4I_BACKEND_REGBUFFCTL_REG, > + val, > + !(val & SUN4I_BACKEND_REGBUFFCTL_LOADCTL), > + 100, 50000); > +} > + > void sun4i_backend_layer_enable(struct sun4i_backend *backend, > int layer, bool enable) > { > @@ -330,6 +343,7 @@ static int sun4i_backend_of_get_id(struct device_node *node) > > static const struct sunxi_engine_ops sun4i_backend_engine_ops = { > .commit = sun4i_backend_commit, > + .commit_poll = sun4i_backend_commit_poll, > .layers_init = sun4i_layers_init, > .apply_color_correction = sun4i_backend_apply_color_correction, > .disable_color_correction = sun4i_backend_disable_color_correction, > diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c > index 2eba1d8639d8..31550493fa1d 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c > +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c > @@ -34,6 +34,7 @@ static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc, > struct drm_crtc_state *old_state) > { > struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc); > + struct sunxi_engine *engine = scrtc->engine; > struct drm_device *dev = crtc->dev; > unsigned long flags; > > -- > git-series 0.9.1
On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote: > Hi, > > On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > In the earlier display engine designs, any register access while a commit > > is pending is forbidden. > > > > One of the symptoms is that reading a register will return another, random, > > register value which can lead to register corruptions if we ever do a > > read/modify/write cycle. > > Alternatively, if changes to the backend (layers) are guaranteed to happen > while the CRTC is disabled (which seems to be the case after looking at > drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we > could just turn on register auto-commit all the time and not deal with > this. As far as I understand, it will only be the case if we need a new modeset or we changed the active CRTC or connectors. But if you change only the format, buffers or properties it won't be the case, and we'll need to commit. Maxime
On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote: >> Hi, >> >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >> > In the earlier display engine designs, any register access while a commit >> > is pending is forbidden. >> > >> > One of the symptoms is that reading a register will return another, random, >> > register value which can lead to register corruptions if we ever do a >> > read/modify/write cycle. >> >> Alternatively, if changes to the backend (layers) are guaranteed to happen >> while the CRTC is disabled (which seems to be the case after looking at >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we >> could just turn on register auto-commit all the time and not deal with >> this. > > As far as I understand, it will only be the case if we need a new > modeset or we changed the active CRTC or connectors. But if you change > only the format, buffers or properties it won't be the case, and we'll > need to commit. So in other words, if someone were to use it for actual compositing and moved the upper composited layer around, we would need commit support to be safe. Sounds more or less like something a video player would do. Thanks ChenYu
On Mon, Jul 17, 2017 at 02:57:19PM +0800, Chen-Yu Tsai wrote: > On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote: > >> Hi, > >> > >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard > >> <maxime.ripard@free-electrons.com> wrote: > >> > In the earlier display engine designs, any register access while a commit > >> > is pending is forbidden. > >> > > >> > One of the symptoms is that reading a register will return another, random, > >> > register value which can lead to register corruptions if we ever do a > >> > read/modify/write cycle. > >> > >> Alternatively, if changes to the backend (layers) are guaranteed to happen > >> while the CRTC is disabled (which seems to be the case after looking at > >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we > >> could just turn on register auto-commit all the time and not deal with > >> this. > > > > As far as I understand, it will only be the case if we need a new > > modeset or we changed the active CRTC or connectors. But if you change > > only the format, buffers or properties it won't be the case, and we'll > > need to commit. > > So in other words, if someone were to use it for actual compositing and > moved the upper composited layer around, we would need commit support to be > safe. > > Sounds more or less like something a video player would do. Not only that. A change of buffer will happen every frame or so, and we can change the format whenever we want too (even if it's usually going to be in sync with a new buffer). Changing a property can happen any time too (like zpos for example). Maxime
On Tue, Jul 18, 2017 at 9:07 AM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Mon, Jul 17, 2017 at 02:57:19PM +0800, Chen-Yu Tsai wrote: >> On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >> > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote: >> >> Hi, >> >> >> >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard >> >> <maxime.ripard@free-electrons.com> wrote: >> >> > In the earlier display engine designs, any register access while a commit >> >> > is pending is forbidden. >> >> > >> >> > One of the symptoms is that reading a register will return another, random, >> >> > register value which can lead to register corruptions if we ever do a >> >> > read/modify/write cycle. >> >> >> >> Alternatively, if changes to the backend (layers) are guaranteed to happen >> >> while the CRTC is disabled (which seems to be the case after looking at >> >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we >> >> could just turn on register auto-commit all the time and not deal with >> >> this. >> > >> > As far as I understand, it will only be the case if we need a new >> > modeset or we changed the active CRTC or connectors. But if you change >> > only the format, buffers or properties it won't be the case, and we'll >> > need to commit. >> >> So in other words, if someone were to use it for actual compositing and >> moved the upper composited layer around, we would need commit support to be >> safe. >> >> Sounds more or less like something a video player would do. > > Not only that. A change of buffer will happen every frame or so, and > we can change the format whenever we want too (even if it's usually > going to be in sync with a new buffer). Changing a property can happen > any time too (like zpos for example). You can upgrade any property change to an atomic modeset by e.g. setting connector->mode_changed (and then making sure to call check_modeset() helper again perhaps). This is for cases where your hw can't handle a property change within 1 vblank. The default is just the solution for most common hw. The other way round works too, you can clear these flags in your atomic_check callbacks. But that requires a bit more care (to make sure you never clear it when there's something else also changing that still needs a full modeset sequence to commit to hw). -Daniel
Hi Daniel, On Tue, Jul 18, 2017 at 09:35:03AM +0200, Daniel Vetter wrote: > On Tue, Jul 18, 2017 at 9:07 AM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > On Mon, Jul 17, 2017 at 02:57:19PM +0800, Chen-Yu Tsai wrote: > >> On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard > >> <maxime.ripard@free-electrons.com> wrote: > >> > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote: > >> >> Hi, > >> >> > >> >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard > >> >> <maxime.ripard@free-electrons.com> wrote: > >> >> > In the earlier display engine designs, any register access while a commit > >> >> > is pending is forbidden. > >> >> > > >> >> > One of the symptoms is that reading a register will return another, random, > >> >> > register value which can lead to register corruptions if we ever do a > >> >> > read/modify/write cycle. > >> >> > >> >> Alternatively, if changes to the backend (layers) are guaranteed to happen > >> >> while the CRTC is disabled (which seems to be the case after looking at > >> >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we > >> >> could just turn on register auto-commit all the time and not deal with > >> >> this. > >> > > >> > As far as I understand, it will only be the case if we need a new > >> > modeset or we changed the active CRTC or connectors. But if you change > >> > only the format, buffers or properties it won't be the case, and we'll > >> > need to commit. > >> > >> So in other words, if someone were to use it for actual compositing and > >> moved the upper composited layer around, we would need commit support to be > >> safe. > >> > >> Sounds more or less like something a video player would do. > > > > Not only that. A change of buffer will happen every frame or so, and > > we can change the format whenever we want too (even if it's usually > > going to be in sync with a new buffer). Changing a property can happen > > any time too (like zpos for example). > > You can upgrade any property change to an atomic modeset by e.g. > setting connector->mode_changed (and then making sure to call > check_modeset() helper again perhaps). This is for cases where your hw > can't handle a property change within 1 vblank. The default is just > the solution for most common hw. > > The other way round works too, you can clear these flags in your > atomic_check callbacks. But that requires a bit more care (to make > sure you never clear it when there's something else also changing that > still needs a full modeset sequence to commit to hw). Hmm, that's good to know, but that would imply disabling the CRTC each time we change even a small property, with all the visual artifacts it might imply, right? Maxime
On Thu, Jul 20, 2017 at 11:53:39AM +0200, Maxime Ripard wrote: > Hi Daniel, > > On Tue, Jul 18, 2017 at 09:35:03AM +0200, Daniel Vetter wrote: > > On Tue, Jul 18, 2017 at 9:07 AM, Maxime Ripard > > <maxime.ripard@free-electrons.com> wrote: > > > On Mon, Jul 17, 2017 at 02:57:19PM +0800, Chen-Yu Tsai wrote: > > >> On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard > > >> <maxime.ripard@free-electrons.com> wrote: > > >> > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote: > > >> >> Hi, > > >> >> > > >> >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard > > >> >> <maxime.ripard@free-electrons.com> wrote: > > >> >> > In the earlier display engine designs, any register access while a commit > > >> >> > is pending is forbidden. > > >> >> > > > >> >> > One of the symptoms is that reading a register will return another, random, > > >> >> > register value which can lead to register corruptions if we ever do a > > >> >> > read/modify/write cycle. > > >> >> > > >> >> Alternatively, if changes to the backend (layers) are guaranteed to happen > > >> >> while the CRTC is disabled (which seems to be the case after looking at > > >> >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we > > >> >> could just turn on register auto-commit all the time and not deal with > > >> >> this. > > >> > > > >> > As far as I understand, it will only be the case if we need a new > > >> > modeset or we changed the active CRTC or connectors. But if you change > > >> > only the format, buffers or properties it won't be the case, and we'll > > >> > need to commit. > > >> > > >> So in other words, if someone were to use it for actual compositing and > > >> moved the upper composited layer around, we would need commit support to be > > >> safe. > > >> > > >> Sounds more or less like something a video player would do. > > > > > > Not only that. A change of buffer will happen every frame or so, and > > > we can change the format whenever we want too (even if it's usually > > > going to be in sync with a new buffer). Changing a property can happen > > > any time too (like zpos for example). > > > > You can upgrade any property change to an atomic modeset by e.g. > > setting connector->mode_changed (and then making sure to call > > check_modeset() helper again perhaps). This is for cases where your hw > > can't handle a property change within 1 vblank. The default is just > > the solution for most common hw. > > > > The other way round works too, you can clear these flags in your > > atomic_check callbacks. But that requires a bit more care (to make > > sure you never clear it when there's something else also changing that > > still needs a full modeset sequence to commit to hw). > > Hmm, that's good to know, but that would imply disabling the CRTC each > time we change even a small property, with all the visual artifacts it > might imply, right? This isn't black&white, you only need to set this when needed. Of course that means you need to have code (and hopefully testcases) to make sure you only set it when needed. And userspace can ask the driver whether a given change requires a full modeset or not and then decided whether it wants to do that switch or not. -Daniel
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c index cf480218daa5..ce1f40f7511e 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.c +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c @@ -67,6 +67,19 @@ static void sun4i_backend_commit(struct sunxi_engine *engine) SUN4I_BACKEND_REGBUFFCTL_LOADCTL); } +static int sun4i_backend_commit_poll(struct sunxi_engine *engine) +{ + u32 val; + + DRM_DEBUG_DRIVER("Polling for the commit to end\n"); + + return regmap_read_poll_timeout(engine->regs, + SUN4I_BACKEND_REGBUFFCTL_REG, + val, + !(val & SUN4I_BACKEND_REGBUFFCTL_LOADCTL), + 100, 50000); +} + void sun4i_backend_layer_enable(struct sun4i_backend *backend, int layer, bool enable) { @@ -330,6 +343,7 @@ static int sun4i_backend_of_get_id(struct device_node *node) static const struct sunxi_engine_ops sun4i_backend_engine_ops = { .commit = sun4i_backend_commit, + .commit_poll = sun4i_backend_commit_poll, .layers_init = sun4i_layers_init, .apply_color_correction = sun4i_backend_apply_color_correction, .disable_color_correction = sun4i_backend_disable_color_correction, diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c index 2eba1d8639d8..31550493fa1d 100644 --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c @@ -34,6 +34,7 @@ static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_crtc_state *old_state) { struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc); + struct sunxi_engine *engine = scrtc->engine; struct drm_device *dev = crtc->dev; unsigned long flags;
In the earlier display engine designs, any register access while a commit is pending is forbidden. One of the symptoms is that reading a register will return another, random, register value which can lead to register corruptions if we ever do a read/modify/write cycle. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- drivers/gpu/drm/sun4i/sun4i_backend.c | 14 ++++++++++++++ drivers/gpu/drm/sun4i/sun4i_crtc.c | 1 + 2 files changed, 15 insertions(+)