Received-SPF: pass (google.com: domain of linux-kernel+bounces-98556-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Frank Oltmanns <frank@oltmanns.dev>
To: =?utf-8?Q?Ond=C5=99ej?= Jirman <x@xnux.eu>
Cc: Maxime Ripard <mripard@kernel.org>,  Chen-Yu Tsai <wens@csie.org>,
  Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,  Thomas Zimmermann
 <tzimmermann@suse.de>,  David Airlie <airlied@gmail.com>,  Daniel Vetter
 <daniel@ffwll.ch>,  Jernej Skrabec <jernej.skrabec@gmail.com>,  Samuel
 Holland <samuel@sholland.org>,  Icenowy Zheng <uwu@icenowy.me>,
  dri-devel@lists.freedesktop.org,  linux-arm-kernel@lists.infradead.org,
  linux-sunxi@lists.linux.dev,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/sun4i: tcon: Support keeping dclk rate upon
 ancestor clock changes
In-Reply-To: <s3iqqnqiqnlujncbb6vnip7hvofgbom54on7fx4hxmyfsk2v2w@gbvpkptsa5g3>
	(=?utf-8?Q?=22Ond=C5=99ej?= Jirman"'s message of "Sun, 10 Mar 2024 23:23:57
 +0100")
References: <20240310-tcon_keep_stable_rate-v1-1-0296b0a85c02@oltmanns.dev>
	<s3iqqnqiqnlujncbb6vnip7hvofgbom54on7fx4hxmyfsk2v2w@gbvpkptsa5g3>
Date: Mon, 11 Mar 2024 09:53:41 +0100
Message-ID: <87le6pcfoq.fsf@oltmanns.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Hello Ond=C5=99ej,

On 2024-03-10 at 23:23:57 +0100, Ond=C5=99ej Jirman <x@xnux.eu> wrote:
> Hello Frank,
>
> On Sun, Mar 10, 2024 at 02:32:29PM +0100, Frank Oltmanns wrote:
>> +static int sun4i_rate_reset_notifier_cb(struct notifier_block *nb,
>> +				      unsigned long event, void *data)
>> +{
>> +	struct sun4i_rate_reset_nb *rate_reset =3D to_sun4i_rate_reset_nb(nb);
>> +
>> +	if (event =3D=3D POST_RATE_CHANGE)
>> +		schedule_delayed_work(&rate_reset->reset_rate_work, msecs_to_jiffies(=
100));
>
> If you get multiple reset notifier calls within 100ms of the first one,
> the delay from the last one will not be 100ms, so this may violate expect=
ations
> you're describing in the commit message.

Let me start by saying, the implicit expectation is that the new user of
pll-video0 will get an exclusive lock on pll-video0 (otherwise,
data-clock would reset pll-video0 after those 100ms rendering the whole
endeavor of the new user pointless). This constraint makes the chances
that the above event (two consecutive rate changes within 100ms) occurs
very slim.

That being said, I don't see a problem with cancelling the delayed work
on PRE_RATE_CHANGE and restarting it on ABORT_RATE_CHANGE like this:

+static int sun4i_rate_reset_notifier_cb(struct notifier_block *nb,
+				      unsigned long event, void *data)
+{
+	struct sun4i_rate_reset_nb *rate_reset =3D to_sun4i_rate_reset_nb(nb);
+
+	if (event =3D=3D PRE_RATE_CHANGE) {
+		rate_reset->is_cancelled =3D cancel_delayed_work(&rate_reset->reset_rate=
_work);
+	} else if ((event =3D=3D POST_RATE_CHANGE) ||
+		 (event =3D=3D ABORT_RATE_CHANGE) && rate_reset->is_cancelled) {
+		schedule_delayed_work(&rate_reset->reset_rate_work, msecs_to_jiffies(100=
));
+		rate_reset->is_cancelled =3D false;
+	}
+
+	return NOTIFY_DONE;
+}

The need for this new code is slim (IMHO) but on the other hand it
doesn't add much complexity. I'll add it in V2 including the
is_cancelled member in sun4i_rate_reset_nb if I don't receive any
objections during this week.

Thanks,
  Frank

>
> schedule_delayed_work doesn't re-schedule the work if it's already pendin=
g.
>
> Kind regards,
> 	o.