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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.
> >>> 
> >>> 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;
> >> 
> >> 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:
> 
>  1. 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);
}

>  2. 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]

-- 
Regards,

Laurent Pinchart