Re: [PATCH] drm/sun4i: tcon: Support keeping dclk rate upon ancestor clock changes

From: Frank Oltmanns <frank@oltmanns.dev>
To: "Ondřej 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
Date: Mon, 11 Mar 2024 09:53:41 +0100	[thread overview]
Message-ID: <87le6pcfoq.fsf@oltmanns.dev> (raw)
In-Reply-To: <s3iqqnqiqnlujncbb6vnip7hvofgbom54on7fx4hxmyfsk2v2w@gbvpkptsa5g3> ("Ondřej Jirman"'s message of "Sun, 10 Mar 2024 23:23:57 +0100")

Hello Ondřej,

On 2024-03-10 at 23:23:57 +0100, Ondřej 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 = to_sun4i_rate_reset_nb(nb);
>> +
>> +	if (event == 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 expectations
> 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 = to_sun4i_rate_reset_nb(nb);
+
+	if (event == PRE_RATE_CHANGE) {
+		rate_reset->is_cancelled = cancel_delayed_work(&rate_reset->reset_rate_work);
+	} else if ((event == POST_RATE_CHANGE) ||
+		 (event == ABORT_RATE_CHANGE) && rate_reset->is_cancelled) {
+		schedule_delayed_work(&rate_reset->reset_rate_work, msecs_to_jiffies(100));
+		rate_reset->is_cancelled = 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 pending.
>
> Kind regards,
> 	o.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel