Hi Liviu,
On Wednesday 02 Aug 2017 14:57:30 Liviu Dudau wrote:
On Wed, Aug 02, 2017 at 04:49:16PM +0300, Laurent Pinchart wrote:
On Wednesday 02 Aug 2017 14:32:06 Liviu Dudau wrote:
On Wed, Aug 02, 2017 at 04:27:27PM +0300, Laurent Pinchart wrote:
On Wednesday 02 Aug 2017 13:46:48 Liviu Dudau wrote:
On Wed, Aug 02, 2017 at 01:27:23PM +0200, Daniel Vetter wrote:
On Wed, Aug 2, 2017 at 1:20 PM, Liviu Dudau wrote: > On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: >> +/** >> + * drm_atomic_helper_commit_tail_rpm - commit atomic update to >> hardware >> + * @old_state: new modeset state to be committed >> + * >> + * This is an alternative implementation for the >> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for >> drivers >> + * that support runtime_pm or need the CRTC to be enabled to >> perform a >> + * commit. Otherwise, one should use the default implementation >> + * drm_atomic_helper_commit_tail(). >> + */ >> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state >> *old_state) >> +{ >> + struct drm_device *dev = old_state->dev; >> + >> + drm_atomic_helper_commit_modeset_disables(dev, old_state); >> + >> + drm_atomic_helper_commit_modeset_enables(dev, old_state); >> + >> + drm_atomic_helper_commit_planes(dev, old_state, >> + >> DRM_PLANE_COMMIT_ACTIVE_ONLY); >> + >> + drm_atomic_helper_commit_hw_done(old_state); >> + >> + drm_atomic_helper_wait_for_vblanks(dev, old_state); >> + >> + drm_atomic_helper_cleanup_planes(dev, old_state); >> +} >> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); >> + > > Given that this function is supposed to be used by runtime PM aware > drivers and that the hook is called from the DRM core, should there > not be some pm_runtime_{get,put} calls wrapping the body of this > function?
Hi Daniel,
No, because the drm atomic helpers have no idea which device is backing which part of the drm device. Some drivers might on have one device for the entire driver, some one device for just the display part (which might or might not match drm_device->dev). And many arm drivers have a device for each block separately (and you need to call rpm_get/put on each). And some something in-between (e.g. core device + external encoders).
Hmm, I understand your point about this function not having to care about pm_runtime_{get,put}, but I do not agree with your view that if it wanted to care about it, it wouldn't be able to do the right thing because it doesn't have the right device. After all, this function is about handling the updates that this atomic commit is concerned about, so having the old_state->dev drm_device pointer and its associated device should be enough. Am I missing something?
In embedded system it's quite common for display hardware to be made of multiple IP cores. Depending on the SoC you could have to manage runtime PM at the CRTC level for instance. The DRM core doesn't know about that, and sees a single device only.
I've expressed my doubts previously about the need for a RPM version of drm_atomic_helper_commit_tail(), as the resulting order of CRTC enable/disable and plane update operations can lead to corrupt frames when enabling the CRTC. I had a commit tail implementation in the rcar-du driver that was very similar to drm_atomic_helper_commit_tail_rpm(), and had to move back to the standard order to fix the corrupt frame issue. The result isn't as clean as I would like, as power handling is split between the CRTC enable/disable and the atomic begin/flush in a way that is not straightforward.
It now occurred to me that a simpler implementation could be possible. I'll have to experiment with it first, but the idea is as follows.
1 Call pm_runtime_get_sync() at the beginning of .commit_tail() and pm_runtime_put() at the end.
- Use the standard CRTC disable, plane update, CRTC enable order in
.commit_tail().
- Call pm_runtime_get() in the CRTC .enable() handler and
pm_runtime_put() in the CRTC .disable() handler;
Well, that is what mali-dp driver currently does, but according to Daniel (and I can see his POV) that is wrong.
Is it ? I might not have understood his arguments the same way (or possibly failed to even see them). Are you referring to this comments in this mail thread, or to something else ?
I'm talking about his reply above. My understanding: you can't do pm_runtime_{get,set} in the atomic_commit_tail hook because:
- you don't know if you have the correct drm_device->dev (or all the
relevant devices) to call pm_runtime_get() on.
You can't call pm_runtime_get() in the DRM core for that exact reason, but you can call it in a driver's implementation of .atomic_commit_tail(), which was my proposal. The .atomic_commit_tail() handler would then become something like
{ for_each_ip_affected(ip, ...) pm_runtime_get_sync(ip);
drm_atomic_helper_commit_tail(...);
for_each_ip_affected(ip, ...) pm_runtime_put(ip); }
- If pm_runtime_get() affects in any way the atomic commit behaviour by
being at the top of the commit_tail_rpm() function then you are: a) papering over bugs in one's driver b) going to turn off things at the end of commit_tail_rpm() function when you call pm_runtime_put() so your changes are going to be lost.
You won't, because you also call pm_runtime_get() in the CRTC .enable() handler.
I'm playing with removing all of that to see if there are any side effects in Mali DP like the ones you mentioned for RCAR.
Note that the first frame will usually not be noticed as it often takes a few frames for the display to turn on.
Yes, and I have a TV connected to the output that takes ages to sync. But I can usually run some quick rmmmod+insmod tests and the TV would be too slow to turn off the output, so I can see if there are any artifacts.
One good way to test this is to implement support for CRC calculation if your hardware supports it.
This should guarantee that the device won't be suspended between CRTC disable and CRTC enable during a mode set, without requiring any special collaboration between CRTC enable/disable and atomic begin/flush to handle runtime PM. If drivers implement this, there should be no need for drm_atomic_helper_commit_tail_rpm().
Maxime, Daniel, what do you think about this ?
[snip]