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.
Thanks,
Leo
