[PATCH v2 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users

Daniel Vetter daniel at ffwll.ch
Wed Aug 2 07:28:16 PDT 2017

On Wed, Aug 2, 2017 at 3:27 PM, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
> Hi Livu,
> 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 <Liviu.Dudau at arm.com> 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.
> 2. Use the standard CRTC disable, plane update, CRTC enable order in
> .commit_tail().
> 3. Call pm_runtime_get() in the CRTC .enable() handler and pm_runtime_put() in
> the CRTC .disable() handler;
> 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 ?

Ok, since you just said on irc that the corrupted frame is some random
color, that's indeed not cool. What all other drivers (well at least
i915) do is:

1. Enable the screen, but scan out nothing as a pure black. This might
mean you need to set up the blend unit with a background color of
2. Do an atomic flip to the new screen contents.

This way you get the sequence implement in the new _rpm
implementation. And for most normal hw this gives you the simplest
implementation, and has the benefit that writing to disabled hw blocks
indicates a bug. Wrapping an entire sequence with rpm_get/put like
Laurent describes is a really good way to hide bugs (e.g. write new
stuff to hw you're about to disable and then drop the hw settings on
the floor). Yes it makes implementation a bit easier, but easier !=
more correct in many cases. Nasty self-checks are good for getting kms
implementation rights.

Another benefit of this sequence is that the initial plane enable
after a crtc enable is like any other "background black -> planes"
atomic switch, and atomic does allow you to disable all planes. So
your driver better supports that (or is one of the few which has to
reject a config without any planes), since userspace might want to
enable the CRTC with no planes, and then you have the exact same bug

It sounds like Laurent's 3rd gen rcar-du is special, and so needs
special code (or well just a bugfix to enable the composer with all
black in the CRTC enable code.

Cheers, Daniel

>> > I don't think there's anything the helpers can to to help support this.
>> >
>> > Also, just wrapping functions with rpm_get/put only papers over bugs
>> > in your driver - either you enable something, then the rpm_get needs
>> > to be done anyway (since the hw will be shut down otherwise), or you
>> > disable something, same reasons. The only thing a rpm_get/put does is
>> > paper over the bugs where you try to access the hw when it's off. As
>> > soon as this function finishes, the hw is shut down again, drops all
>> > register values on the floor, so either that access wasn't needed, or
>> > your driver has a bug (because it assumes the register values survive
>> > when they don't).
>> >
>> > So imo all around a bad idea, at least from my experience of doing rpm
>> > enabling on i915 hw.
>> Understood. Thanks!
> --
> Regards,
> Laurent Pinchart

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

More information about the Linux-rockchip mailing list