On 2025-12-10 12:55, Leo Li wrote:
>
>
> On 2025-12-09 05:47, Ville Syrjälä wrote:
>> On Tue, Dec 09, 2025 at 12:05:31PM +0200, Jani Nikula wrote:
>>> On Mon, 01 Dec 2025, <[email protected]> wrote:
>>>> From: Leo Li <[email protected]>
>>>>
>>>> Some drivers need to perform blocking operations prior to enabling
>>>> vblank interrupts. A display hardware spin-up from a low-power state
>>>> that requires synchronization with the rest of the driver via a mutex,
>>>> for example.
>>>>
>>>> To support this, introduce a new drm_crtc_vblank_prepare() helper that
>>>> calls back into the driver -- if implemented -- for the driver to do
>>>> such preparation work.
>>>>
>>>> In DRM core, call this helper before drm_vblank_get(). Drivers can
>>>> choose to call this if they implement the callback in the future.
>>>
>>> Have you considered hiding all of this inside drm_vblank.c? Call prepare
>>> in drm_crtc_vblank_get() and a couple of other places? And actually
>>> don't call it on !drm_dev_has_vblank(dev)?
>>>
>>> There's just so much littering all over the place with the prepare, and
>>> it seems brittle. Especially when you expect not only the drm core but
>>> also the relevant drivers to call drm_crtc_vblank_prepare() when needed.
>>>
>>> There does seem to be a few places in amdgpu that wrap the
>>> drm_crtc_vblank_get() inside dev->event_lock, but is there really any
>>> need to do so? Do the get first, and grab the event_lock after?
>>>
>>> Some random comments inline.
>>>
>>> Cc: Ville
>>
>> drm_vblank_get() can get called from any kind of context.
>> The only workable solution might be the schedule a work from
>> .vblank_enable() and do whatever is needed from there.
>>
> Yeah, drm_vblank_get() can be called from interrupt context, so the sleeping
> work has to happen outside of it.
>
> The issue with deferring work from .enable_vblank() is drm_vblank_enable()
> follows up immediately with a drm_update_vblank_count(); it expects HW to be
> online for reading the vblank counter. Meanwhile, the prepare work can be
> pending, and waiting for HW to be online from drm_update_vblank_count()
> wouldn't
> be ideal.
>
> I've thought about implementing a sw vblank counter while HW is waking up, but
> that sounds overly complex. It would be simpler to somehow signal prepare work
> before we enter non-sleeping context.
>
> Would a function like drm_crtc_get_vblank_with_prepare() help?
> vblank_prepare()
> can be wrapped in it before calling get_vblank(). drm_crtc_get_vblank()
> call-sites outside of drm_vblank.c -- called from process context -- can be
> replaced with it.
>
> The only case it's not called from process context (within drm core) is in
> drm_crtc_vblank_on_config(), which is in drm_vblank.c itself. I think
> vblank_prepare() can be open coded there. Drivers can choose to call
> get_vblank_with_prepare() if they actually have blocking prepare work.
Sorry for the delay, just had some time to look at this again.
It seems I missed a place in my last comment about combining prepare() and get()
into one function. drm_vblank_work_schedule() in drm_vblank_work.c calls
vblank_get() with the event_lock, so an option to vblank_prepare() separately is
still needed.
I don't think we actually need to export drm_crtc_vblank_prepare() to drivers.
It's purpose is for DRM to run blocking driver work to prepare for vblank
programming, not the other way around. Drivers can sequence prepare work
themselves, if they need to, before doing any programming.
I think vblank_prepare() can be hidden in drm_internal.h, to limit it's use to
DRM core. I'll spin up a v2 rebased on Jani's series with this and some other
comments addressed.
Thanks,
Leo
>
> Thanks,
> Leo
>
