All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tony Lindgren <tony@atomide.com>
To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Sebastian Reichel <sre@kernel.org>,
	dri-devel@lists.freedesktop.org, linux-omap@vger.kernel.org
Subject: Re: [PATCHv2] drm/omap: Fix issue with clocks left on after resume
Date: Mon, 3 May 2021 14:16:04 +0300	[thread overview]
Message-ID: <YI/bdLkwtUNFKHyW@atomide.com> (raw)
In-Reply-To: <YI/UXqQbvdtC2HqI@atomide.com>

Hi,

* Tony Lindgren <tony@atomide.com> [210503 10:45]:
> * Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> [210503 08:04]:
> > On 29/04/2021 07:46, Tony Lindgren wrote:
> > > Decoupling the system suspend and resume from PM runtime calls for
> > > all the other dss components should still also be done IMO. But that
> > > can be done as a separate clean-up patches after we have fixed the
> > > $subject issue.
> > 
> > I don't think I still really understand why all this is needed. I mean,
> > obviously things don't work correctly at the moment, so maybe this patch can
> > be applied to fix the system suspend. But it just feels like a big hack (the
> > current pm_runtime_force_suspend/resume work-around feels like a big hack
> > too).
> 
> Well omapdrm is not handling the -EBUSY error during system resume.

Or rather something on the resume path is not handling and cannot handle
-EBUSY. And sounds like the reason is..

> > Slightly off topic, but I just noticed that we're using runtime_put_sync for
> > some reason. Found 0eaf9f52e94f756147dbfe1faf1f77a02378dbf9. I've been
> > fighting with system suspend for a long time =).
> > 
> > I wonder if using non-sync version would remove the EBUSY problem...
> 
> Worth trying, but it will only help if the -EBUSY error from
> pm_runtime_put() is handled somewhere for a retry..

..the use of pm_runtime_put_sync() like you suggested. I did a quick
test with the minimal change below and that works :) Seems like that's
probably the best minimal fix for the -rc cycle.

Regards,

Tony

8< ----------------
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -664,7 +664,7 @@ void dispc_runtime_put(struct dispc_device *dispc)
 
 	DSSDBG("dispc_runtime_put\n");
 
-	r = pm_runtime_put_sync(&dispc->pdev->dev);
+	r = pm_runtime_put(&dispc->pdev->dev);
 	WARN_ON(r < 0 && r != -ENOSYS);
 }
 

WARNING: multiple messages have this Message-ID (diff)
From: Tony Lindgren <tony@atomide.com>
To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: linux-omap@vger.kernel.org,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	dri-devel@lists.freedesktop.org,
	Sebastian Reichel <sre@kernel.org>
Subject: Re: [PATCHv2] drm/omap: Fix issue with clocks left on after resume
Date: Mon, 3 May 2021 14:16:04 +0300	[thread overview]
Message-ID: <YI/bdLkwtUNFKHyW@atomide.com> (raw)
In-Reply-To: <YI/UXqQbvdtC2HqI@atomide.com>

Hi,

* Tony Lindgren <tony@atomide.com> [210503 10:45]:
> * Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> [210503 08:04]:
> > On 29/04/2021 07:46, Tony Lindgren wrote:
> > > Decoupling the system suspend and resume from PM runtime calls for
> > > all the other dss components should still also be done IMO. But that
> > > can be done as a separate clean-up patches after we have fixed the
> > > $subject issue.
> > 
> > I don't think I still really understand why all this is needed. I mean,
> > obviously things don't work correctly at the moment, so maybe this patch can
> > be applied to fix the system suspend. But it just feels like a big hack (the
> > current pm_runtime_force_suspend/resume work-around feels like a big hack
> > too).
> 
> Well omapdrm is not handling the -EBUSY error during system resume.

Or rather something on the resume path is not handling and cannot handle
-EBUSY. And sounds like the reason is..

> > Slightly off topic, but I just noticed that we're using runtime_put_sync for
> > some reason. Found 0eaf9f52e94f756147dbfe1faf1f77a02378dbf9. I've been
> > fighting with system suspend for a long time =).
> > 
> > I wonder if using non-sync version would remove the EBUSY problem...
> 
> Worth trying, but it will only help if the -EBUSY error from
> pm_runtime_put() is handled somewhere for a retry..

..the use of pm_runtime_put_sync() like you suggested. I did a quick
test with the minimal change below and that works :) Seems like that's
probably the best minimal fix for the -rc cycle.

Regards,

Tony

8< ----------------
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -664,7 +664,7 @@ void dispc_runtime_put(struct dispc_device *dispc)
 
 	DSSDBG("dispc_runtime_put\n");
 
-	r = pm_runtime_put_sync(&dispc->pdev->dev);
+	r = pm_runtime_put(&dispc->pdev->dev);
 	WARN_ON(r < 0 && r != -ENOSYS);
 }
 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2021-05-03 11:16 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-28  9:25 [PATCHv2] drm/omap: Fix issue with clocks left on after resume Tony Lindgren
2021-04-28  9:25 ` Tony Lindgren
2021-04-28 14:10 ` Laurent Pinchart
2021-04-28 14:10   ` Laurent Pinchart
2021-04-29  4:46   ` Tony Lindgren
2021-04-29  4:46     ` Tony Lindgren
2021-05-03  8:04     ` Tomi Valkeinen
2021-05-03  8:04       ` Tomi Valkeinen
2021-05-03 10:45       ` Tony Lindgren
2021-05-03 10:45         ` Tony Lindgren
2021-05-03 11:16         ` Tony Lindgren [this message]
2021-05-03 11:16           ` Tony Lindgren
2021-05-03 12:17           ` Tony Lindgren
2021-05-03 12:17             ` Tony Lindgren
2021-05-05 11:12             ` Tony Lindgren
2021-05-05 11:12               ` Tony Lindgren
2021-05-07 13:26               ` Tomi Valkeinen
2021-05-07 13:26                 ` Tomi Valkeinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YI/bdLkwtUNFKHyW@atomide.com \
    --to=tony@atomide.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-omap@vger.kernel.org \
    --cc=sre@kernel.org \
    --cc=tomi.valkeinen@ideasonboard.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.