Re: [PATCH v5 04/15] drm/rockchip: vop2: Send pending event when atomic_enable() fails

[Cristian Ciocaltea]

Hi Daniel,

Thanks for reviewing the series!

On 10/30/26 1:21 PM, Daniel Stone wrote:
> Hi Cristian,
> 
> On Tue, 13 Oct 2026 at 23:51, Cristian Ciocaltea
> <cristian.ciocaltea@xxxxxxxxxxxxx> wrote:
>> vop2_crtc_atomic_flush() stashes the CRTC completion event in vp->event
>> and relies on the frame start interrupt (VP_INT_FS_FIELD) to deliver it
>> later via drm_crtc_send_vblank_event().  Since atomic_enable() runs
>> before atomic_flush(), bailing on an error path makes it skip
>> drm_crtc_vblank_on() and leave the video port disabled.  atomic_flush()
>> still traps the event in vp->event, but since the port never scans out,
>> the frame start interrupt does not fire and the event is not delivered.
>> Userspace waiting for the flip completion then blocks indefinitely.
>>
>> Note the event cannot be flushed from atomic_disable(), as userspace
>> waits for the completion before committing the next state, so the
>> disable does not arrive and draining it there would deadlock.
>>
>> Use the newly added vp->enabled flag to detect this in atomic_flush()
>> and send the event immediately when the port did not come up, rather
>> than deferring it to an interrupt that will never occur.  This also
>> avoids the spurious drm_crtc_vblank_get() WARN previously hit on the
>> failed enable path, where the vblank is not on.
> 
> I really don't know how I feel about this one.
> 
> atomic_enable() is not supposed to fail at all, hence returning void.
> (Obviously, there are scenarios where it will fail, which Sashiko
> keeps 'helpfully' pointing out ...)
> 
> I don't like how this just papers over the issue. It turns an eventual
> WARN on vblank timeout - which surfaces the issue - into a silent
> false-success. Userspace just sees commit completion requests arrive
> as soon as it's posted the commit, so it keeps on going on and
> everyone's happy. Having the flips time out is a messy splat of WARNs,
> but at least that messy splat points at the exact issues, rather than
> the screen just going black and userspace blithely trying to continue
> on.

Ack. Let's drop the patch.

Cristian