Some devices might require keeping an interconnect path alive so that
the framebuffer continues working. Add support for that by setting the
bandwidth requirements appropriately for all provided interconnect
paths.
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
drivers/gpu/drm/sysfb/simpledrm.c | 83 +++++++++++++++++++++++++++++++++++++++
1 file changed, 83 insertions(+)
diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index 349219330314e3421a6bb26ad5cf39a679a5cb7a..47d213e20cab1dd1e19528674a95edea00f4bb30 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -2,6 +2,7 @@
#include <linux/aperture.h>
#include <linux/clk.h>
+#include <linux/interconnect.h>
#include <linux/minmax.h>
#include <linux/of_address.h>
#include <linux/of_clk.h>
@@ -225,6 +226,10 @@ struct simpledrm_device {
struct device **pwr_dom_devs;
struct device_link **pwr_dom_links;
#endif
+#if defined CONFIG_OF && defined CONFIG_INTERCONNECT
+ unsigned int icc_count;
+ struct icc_path **icc_paths;
+#endif
/* modesetting */
u32 formats[DRM_SYSFB_PLANE_NFORMATS(1)];
@@ -547,6 +552,81 @@ static int simpledrm_device_attach_genpd(struct simpledrm_device *sdev)
}
#endif
+#if defined CONFIG_OF && defined CONFIG_INTERCONNECT
+/*
+ * Generic interconnect path handling code.
+ */
+static void simpledrm_device_detach_icc(void *res)
+{
+ struct simpledrm_device *sdev = res;
+ int i;
+
+ for (i = sdev->icc_count - 1; i >= 0; i--) {
+ if (!IS_ERR_OR_NULL(sdev->icc_paths[i]))
+ icc_put(sdev->icc_paths[i]);
+ }
+}
+
+static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
+{
+ struct device *dev = sdev->sysfb.dev.dev;
+ int ret, count, i;
+
+ count = of_count_phandle_with_args(dev->of_node, "interconnects",
+ "#interconnect-cells");
+ if (count < 0)
+ return 0;
+
+ /* An interconnect path consists of two elements */
+ if (count % 2) {
+ drm_err(&sdev->sysfb.dev,
+ "invalid interconnects value\n");
+ return -EINVAL;
+ }
+ sdev->icc_count = count / 2;
+
+ sdev->icc_paths = devm_kcalloc(dev, sdev->icc_count,
+ sizeof(*sdev->icc_paths),
+ GFP_KERNEL);
+ if (!sdev->icc_paths)
+ return -ENOMEM;
+
+ for (i = 0; i < sdev->icc_count; i++) {
+ sdev->icc_paths[i] = of_icc_get_by_index(dev, i);
+ if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
+ ret = PTR_ERR(sdev->icc_paths[i]);
+ if (ret == -EPROBE_DEFER)
+ goto err;
+ drm_err(&sdev->sysfb.dev, "failed to get interconnect path %u: %d\n",
+ i, ret);
+ continue;
+ }
+
+ ret = icc_set_bw(sdev->icc_paths[i], 0, UINT_MAX);
+ if (ret) {
+ drm_err(&sdev->sysfb.dev, "failed to set interconnect bandwidth %u: %d\n",
+ i, ret);
+ continue;
+ }
+ }
+
+ return devm_add_action_or_reset(dev, simpledrm_device_detach_icc, sdev);
+
+err:
+ while (i) {
+ --i;
+ if (!IS_ERR_OR_NULL(sdev->icc_paths[i]))
+ icc_put(sdev->icc_paths[i]);
+ }
+ return ret;
+}
+#else
+static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
+{
+ return 0;
+}
+#endif
+
/*
* Modesetting
*/
@@ -633,6 +713,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
if (ret)
return ERR_PTR(ret);
ret = simpledrm_device_attach_genpd(sdev);
+ if (ret)
+ return ERR_PTR(ret);
+ ret = simpledrm_device_attach_icc(sdev);
if (ret)
return ERR_PTR(ret);
--
2.50.0On Mon, Jun 23, 2025 at 08:44:47AM +0200, Luca Weiss wrote: > Some devices might require keeping an interconnect path alive so that > the framebuffer continues working. Add support for that by setting the > bandwidth requirements appropriately for all provided interconnect > paths. > > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- > drivers/gpu/drm/sysfb/simpledrm.c | 83 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 83 insertions(+) If simpledrm is being replaced by a proper DRM driver (thus removing those extra votes), will it prevent ICC device from reaching the sync state? -- With best wishes Dmitry
Hi Dmitry, On Sun Jul 6, 2025 at 1:14 PM CEST, Dmitry Baryshkov wrote: > On Mon, Jun 23, 2025 at 08:44:47AM +0200, Luca Weiss wrote: >> Some devices might require keeping an interconnect path alive so that >> the framebuffer continues working. Add support for that by setting the >> bandwidth requirements appropriately for all provided interconnect >> paths. >> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> >> --- >> drivers/gpu/drm/sysfb/simpledrm.c | 83 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 83 insertions(+) > > If simpledrm is being replaced by a proper DRM driver (thus removing > those extra votes), will it prevent ICC device from reaching the sync > state? How can I try this out? On Milos I don't have MDSS yet but I can add an interconnect path on another device to try it. Are there some prints I can enable, or check sysfs/debugfs to see if it reached sync state? I only recall seeing sync_state pending warnings in some instances, but never that it's synced. Regards Luca
Luca Weiss <luca.weiss@fairphone.com> writes:
> Some devices might require keeping an interconnect path alive so that
> the framebuffer continues working. Add support for that by setting the
> bandwidth requirements appropriately for all provided interconnect
> paths.
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
> drivers/gpu/drm/sysfb/simpledrm.c | 83 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 83 insertions(+)
>
> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index 349219330314e3421a6bb26ad5cf39a679a5cb7a..47d213e20cab1dd1e19528674a95edea00f4bb30 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -2,6 +2,7 @@
>
> #include <linux/aperture.h>
> #include <linux/clk.h>
> +#include <linux/interconnect.h>
> #include <linux/minmax.h>
> #include <linux/of_address.h>
> #include <linux/of_clk.h>
> @@ -225,6 +226,10 @@ struct simpledrm_device {
> struct device **pwr_dom_devs;
> struct device_link **pwr_dom_links;
> #endif
Can you add a /* interconnects */ comment here? Similarly how there is one
for clocks, regulators, power domains, etc.
> +#if defined CONFIG_OF && defined CONFIG_INTERCONNECT
> + unsigned int icc_count;
> + struct icc_path **icc_paths;
> +#endif
>
...
> +static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
> +{
> + struct device *dev = sdev->sysfb.dev.dev;
> + int ret, count, i;
> +
> + count = of_count_phandle_with_args(dev->of_node, "interconnects",
> + "#interconnect-cells");
> + if (count < 0)
> + return 0;
> +
> + /* An interconnect path consists of two elements */
> + if (count % 2) {
> + drm_err(&sdev->sysfb.dev,
> + "invalid interconnects value\n");
> + return -EINVAL;
> + }
> + sdev->icc_count = count / 2;
> +
> + sdev->icc_paths = devm_kcalloc(dev, sdev->icc_count,
> + sizeof(*sdev->icc_paths),
> + GFP_KERNEL);
> + if (!sdev->icc_paths)
> + return -ENOMEM;
> +
> + for (i = 0; i < sdev->icc_count; i++) {
> + sdev->icc_paths[i] = of_icc_get_by_index(dev, i);
> + if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
> + ret = PTR_ERR(sdev->icc_paths[i]);
> + if (ret == -EPROBE_DEFER)
> + goto err;
> + drm_err(&sdev->sysfb.dev, "failed to get interconnect path %u: %d\n",
> + i, ret);
You could use dev_err_probe() instead that already handles the -EPROBE_DEFER
case and also will get this message in the /sys/kernel/debug/devices_deferred
debugfs entry, as the reason why the probe deferral happened.
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Hi Javier,
On Fri Jun 27, 2025 at 9:51 AM CEST, Javier Martinez Canillas wrote:
> Luca Weiss <luca.weiss@fairphone.com> writes:
>
>> Some devices might require keeping an interconnect path alive so that
>> the framebuffer continues working. Add support for that by setting the
>> bandwidth requirements appropriately for all provided interconnect
>> paths.
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>> drivers/gpu/drm/sysfb/simpledrm.c | 83 +++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 83 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
>> index 349219330314e3421a6bb26ad5cf39a679a5cb7a..47d213e20cab1dd1e19528674a95edea00f4bb30 100644
>> --- a/drivers/gpu/drm/sysfb/simpledrm.c
>> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
>> @@ -2,6 +2,7 @@
>>
>> #include <linux/aperture.h>
>> #include <linux/clk.h>
>> +#include <linux/interconnect.h>
>> #include <linux/minmax.h>
>> #include <linux/of_address.h>
>> #include <linux/of_clk.h>
>> @@ -225,6 +226,10 @@ struct simpledrm_device {
>> struct device **pwr_dom_devs;
>> struct device_link **pwr_dom_links;
>> #endif
>
> Can you add a /* interconnects */ comment here? Similarly how there is one
> for clocks, regulators, power domains, etc.
Sure!
>
>> +#if defined CONFIG_OF && defined CONFIG_INTERCONNECT
>> + unsigned int icc_count;
>> + struct icc_path **icc_paths;
>> +#endif
>>
>
> ...
>
>> +static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
>> +{
>> + struct device *dev = sdev->sysfb.dev.dev;
>> + int ret, count, i;
>> +
>> + count = of_count_phandle_with_args(dev->of_node, "interconnects",
>> + "#interconnect-cells");
>> + if (count < 0)
>> + return 0;
>> +
>> + /* An interconnect path consists of two elements */
>> + if (count % 2) {
>> + drm_err(&sdev->sysfb.dev,
>> + "invalid interconnects value\n");
>> + return -EINVAL;
>> + }
>> + sdev->icc_count = count / 2;
>> +
>> + sdev->icc_paths = devm_kcalloc(dev, sdev->icc_count,
>> + sizeof(*sdev->icc_paths),
>> + GFP_KERNEL);
>> + if (!sdev->icc_paths)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < sdev->icc_count; i++) {
>> + sdev->icc_paths[i] = of_icc_get_by_index(dev, i);
>> + if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
>> + ret = PTR_ERR(sdev->icc_paths[i]);
>> + if (ret == -EPROBE_DEFER)
>> + goto err;
>> + drm_err(&sdev->sysfb.dev, "failed to get interconnect path %u: %d\n",
>> + i, ret);
>
> You could use dev_err_probe() instead that already handles the -EPROBE_DEFER
> case and also will get this message in the /sys/kernel/debug/devices_deferred
> debugfs entry, as the reason why the probe deferral happened.
Not quite sure how to implement dev_err_probe, but I think this should
be quite okay?
if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
ret = dev_err_probe(dev, PTR_ERR(sdev->icc_paths[i]),
"failed to get interconnect path %u\n", i);
if (ret == -EPROBE_DEFER)
goto err;
continue;
}
That would still keep the current behavior for defer vs permanent error
while printing when necessary and having it for devices_deferred for the
defer case.
Not sure what the difference between drm_err and dev_err are, but I
trust you on that.
Let me know.
Regards
Luca
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
"Luca Weiss" <luca.weiss@fairphone.com> writes:
Hello Luca,
> Hi Javier,
>
> On Fri Jun 27, 2025 at 9:51 AM CEST, Javier Martinez Canillas wrote:
[...]
>>> +static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
>>> +{
>>> + struct device *dev = sdev->sysfb.dev.dev;
>>> + int ret, count, i;
>>> +
>>> + count = of_count_phandle_with_args(dev->of_node, "interconnects",
>>> + "#interconnect-cells");
>>> + if (count < 0)
>>> + return 0;
>>> +
You are already checking here the number of interconnects phandlers. IIUC
this should return -ENOENT if there's no "interconects" property and your
logic returns success in that case.
[...]
>>
>> You could use dev_err_probe() instead that already handles the -EPROBE_DEFER
>> case and also will get this message in the /sys/kernel/debug/devices_deferred
>> debugfs entry, as the reason why the probe deferral happened.
>
> Not quite sure how to implement dev_err_probe, but I think this should
> be quite okay?
>
And of_icc_get_by_index() should only return NULL if CONFIG_INTERCONNECT
is disabled but you have ifdef guards already for this so it should not
happen.
> if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
Then here you could just do a IS_ERR() check and not care about being NULL.
> ret = dev_err_probe(dev, PTR_ERR(sdev->icc_paths[i]),
> "failed to get interconnect path %u\n", i);
> if (ret == -EPROBE_DEFER)
> goto err;
Why you only want to put the icc_paths get for the probe deferral case? I
think that you want to do it for any error?
> continue;
I'm not sure why you need this?
> }
>
> That would still keep the current behavior for defer vs permanent error
> while printing when necessary and having it for devices_deferred for the
> defer case.
>
As mentioned I still don't understand why you want the error path to only
be called for probe deferral. I would had thought that any failure to get
an interconnect would led to an error and cleanup.
> Not sure what the difference between drm_err and dev_err are, but I
> trust you on that.
>
The drm_err() adds DRM specific info but IMO the dev_err_probe() is better
to avoid printing errors in case of probe deferral and also to have it in
the devices_deferred debugfs entry.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Hi Javier,
On Fri Jul 11, 2025 at 11:21 AM CEST, Javier Martinez Canillas wrote:
> "Luca Weiss" <luca.weiss@fairphone.com> writes:
>
> Hello Luca,
>
>> Hi Javier,
>>
>> On Fri Jun 27, 2025 at 9:51 AM CEST, Javier Martinez Canillas wrote:
>
> [...]
>
>>>> +static int simpledrm_device_attach_icc(struct simpledrm_device *sdev)
>>>> +{
>>>> + struct device *dev = sdev->sysfb.dev.dev;
>>>> + int ret, count, i;
>>>> +
>>>> + count = of_count_phandle_with_args(dev->of_node, "interconnects",
>>>> + "#interconnect-cells");
>>>> + if (count < 0)
>>>> + return 0;
>>>> +
>
> You are already checking here the number of interconnects phandlers. IIUC
> this should return -ENOENT if there's no "interconects" property and your
> logic returns success in that case.
We shouldn't error out in case there's no interconnects defined for this
simple-framebuffer though? That'd break all other usages of it?
>
> [...]
>
>>>
>>> You could use dev_err_probe() instead that already handles the -EPROBE_DEFER
>>> case and also will get this message in the /sys/kernel/debug/devices_deferred
>>> debugfs entry, as the reason why the probe deferral happened.
>>
>> Not quite sure how to implement dev_err_probe, but I think this should
>> be quite okay?
>>
>
> And of_icc_get_by_index() should only return NULL if CONFIG_INTERCONNECT
> is disabled but you have ifdef guards already for this so it should not
> happen.
>
>> if (IS_ERR_OR_NULL(sdev->icc_paths[i])) {
>
> Then here you could just do a IS_ERR() check and not care about being NULL.
But checking also for NULL shouldn't hurt either, in case the compile
guards get removed in the future or something?
Quote:
> * Return: icc_path pointer on success or ERR_PTR() on error. NULL is returned
> * when the API is disabled or the "interconnects" DT property is missing.
>
>> ret = dev_err_probe(dev, PTR_ERR(sdev->icc_paths[i]),
>> "failed to get interconnect path %u\n", i);
>> if (ret == -EPROBE_DEFER)
>> goto err;
>
> Why you only want to put the icc_paths get for the probe deferral case? I
> think that you want to do it for any error?
This is the same logic as e.g. for the regulator code in simpledrm. The
idea seems to be that in case some regulator (or here interconnect)
doesn't probe correctly, we still try anyways. Just for EPROBE_DEFER we
defer and wait until the supplier is available.
So defer -> defer simpledrm probe
So error -> ignore error and continue probe
>
>> continue;
>
> I'm not sure why you need this?
For the above behavior.
I guess there were some original design decisions behind handling it
this way, so I don't see a reason to handle it differently for
interconnects.
>
>> }
>>
>> That would still keep the current behavior for defer vs permanent error
>> while printing when necessary and having it for devices_deferred for the
>> defer case.
>>
>
> As mentioned I still don't understand why you want the error path to only
> be called for probe deferral. I would had thought that any failure to get
> an interconnect would led to an error and cleanup.
See above.
Regards
Luca
>
>> Not sure what the difference between drm_err and dev_err are, but I
>> trust you on that.
>>
>
> The drm_err() adds DRM specific info but IMO the dev_err_probe() is better
> to avoid printing errors in case of probe deferral and also to have it in
> the devices_deferred debugfs entry.
© 2016 - 2025 Red Hat, Inc.