Re: [PATCH 4/4] drm/sun4i: make sure we don't have a commit pending

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 4/4] drm/sun4i: make sure we don't have a commit pending

[Daniel Vetter]

On Tue, Jul 18, 2017 at 9:07 AM, Maxime Ripard
 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
>>  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
>> >>  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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/sun4i: make sure we don't have a commit pending

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 4/4] drm/sun4i: make sure we don't have a commit pending

[Chen-Yu Tsai]

On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard
 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
>>  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

Re: [PATCH 4/4] drm/sun4i: make sure we don't have a commit pending

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 4/4] drm/sun4i: make sure we don't have a commit pending

[Chen-Yu Tsai]

Hi,

On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard
 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 
> ---
>  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, 5);
> +}
> +
>  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

[PATCH 4/4] drm/sun4i: make sure we don't have a commit pending

[Maxime Ripard]

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 
---
 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, 5);
+}
+
 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