nv2a: Update Conexant field counter on vblank

[abaire]

DreamWorks: Over the Hedge performs D3D resets before playing FMVs at startup. As a part of this process, an initialization is performed that checks to see if a DirectX frame counter aligns with the counter reported by the TV encoder (at least if Conexant is the encoder, it looks like it is bypassed for other cases). If there is a mismatch, the DirectX frame counter is incremented.

This can cause an issue where the vblank interrupt handler misses the expected frame counter, causing it to fail to update the nv2a buffer read index via the NV_PGRAPH_INCREMENT PGRAPH register. In this particular title, this leads to a livelock where the pushbuffer is waiting on a buffer flip that cannot be triggered until the current buffer is marked as read via the register write.

This change populates the low nibble of the Conexant device with a simple frame counter. See https://xboxdevwiki.net/Video_Encoder for a link to the datasheet.

Unfortunately testing this on HW has proved difficult, as some state is changed during D3D Present calls that causes SMBus reads to stop providing accurate data. Thus, this change is mostly a guess at the actual behavior along with some light testing without using D3D that shows the field value incrementing from 0-3 and wrapping around forever, presumably aligned with vsync.

Fixes #1962

[abaire]

When investigating #1962 it appeared that the code checking for the field count was only run for the Conexant encoder. That said, it seems plausible that the other encoders might have a similar mechanism that could be queried in other cases.

I wonder if there's a callback mechanism that we could use to notify (at least TV encoder) devices of vblank instead of calling out from here? That would potentially let us remove the object_resolve_path_type call as well.

[abaire]

Verified that the report of the fix not working is related to M8+ bios use. I suspect the bios does something that stops it from checking the field count.

[abaire]

/gemini review

(Testing to see if code assist is enabled)

[gemini-code-assist]

Code Review

This pull request implements a VBLANK notification mechanism for the Conexant CX25871 SMBus device to update its internal field counter. The feedback identifies that the notification call should be moved outside the interrupt-pending check to ensure the counter increments independently of the guest's interrupt handling. Additionally, the implementation should cache the device object lookup to improve performance in the high-frequency VBLANK path and refine the register masking logic to avoid clearing unrelated status bits.

[gemini-code-assist]

The smbus_cx25871_notify_vblank() call is currently inside the if block that checks if a VBLANK interrupt is already pending. This means the field counter will stop incrementing if the guest is slow to acknowledge the interrupt. Since the hardware field counter should tick independently of the interrupt status, it is better to move the notification outside the conditional block.

    smbus_cx25871_notify_vblank();
    if (!(d->pcrtc.pending_interrupts & NV_PCRTC_INTR_0_VBLANK)) {
        d->pcrtc.pending_interrupts |= NV_PCRTC_INTR_0_VBLANK;
    }

[abaire]

I don't think it's guaranteed that nv2a_vga_gfx_update is only called during a vblank; if we're missing a vblank interrupt I would imagine that the guest software would believe that the FIELD_CNT should not have been increased yet and it would be erroneous to let it increment late (arguably early from the guest point of view?).

[gemini-code-assist]

The current implementation performs a full object tree lookup (object_resolve_path_type) on every vblank, which is inefficient for a high-frequency path (typically 60Hz). Caching the resolved object pointer would improve performance.

Additionally, the register update logic (cx->registers[0x06] & 0xF0) | ((cx->registers[0x06] + 1) & 0x03) clears bits 2 and 3 of the register. Since the field counter is described as a 2-bit value (0-3), it is safer to use a mask that only affects those two bits (e.g., ~0x03) to avoid side effects on other status bits in the same register.

    static SMBusCX25871Device *cx;
    static bool searched = false;

    if (unlikely(!searched)) {
        Object *obj = object_resolve_path_type("", TYPE_SMBUS_CX25871, NULL);
        cx = obj ? SMBUS_CX25871(obj) : NULL;
        searched = true;
    }

    if (cx) {
        cx->registers[0x06] = (cx->registers[0x06] & ~0x03) |
                              ((cx->registers[0x06] + 1) & 0x03);
    }

[abaire]

Caching seems like a reasonable thing to do; implemented. I believe the object will only be created due to smbus_cx25871_init being called so we could probably cache in the init function itself rather than doing a deferred static hack in the vsync handler.

I'm not sure if objects can move around or be reallocated; @mborgerson do you know if it's safe to permacache the object given that it's not a removable device?

The mask was technically overly conservative (FIELD_CNT is the low nibble of register 6 based on the datasheet) though in practice the kernel only seems to use it as an even/odd tracker. Expanded and added a symbolic constant since I couldn't remember the name to verify it.

[MasonT8198]

I triggered this then deleted the message bc I figured I wouldn't have perms to get it going lol, sorry if that was confusing as to why the AI randomly decided to start going

[abaire]

I'm happy you did, the suggestions were mostly good ones