Fix for cursor corruption with overscan #4776
Conversation
During a commit, the core clock, which feeds the HVS, needs to run at a minimum of 500MHz. While doing that commit, we can also change the mode to one that requires a higher core clock, so we take the core clock rate associated to that new state into account for that boost. However, the old state also needs to be taken into account if it requires a core clock higher that the new one and our 500MHz limit, since it's still live in hardware at the beginning of our commit. Fixes: 16e1010 ("drm/vc4: Increase the core clock based on HVS load") Signed-off-by: Maxime Ripard <[email protected]>
The assigned_channel field of our vc4_crtc_state structure is accessed multiple times in vc4_hvs_atomic_flush, so let's move it to a variable that can be used in all those places. Signed-off-by: Maxime Ripard <[email protected]>
Setting the DISPLISTx register needs to occur in every case, and we don't need to protect the register using the event_lock, so we can just move it after the if branches and simplify a bit the function. Signed-off-by: Maxime Ripard <[email protected]>
The vc4_hvs_update_dlist function mostly deals with setting up the vblank events and setting up the dlist entry pointer to our current active one. We'll want to do the former separately from the vblank handling in later patches, so let's move it to a function of its own. Signed-off-by: Maxime Ripard <[email protected]>
atomic_flush will be called for each CRTC even if they aren't enabled. The whole code we have there will thus run without a properly affected channel, which can then result in all sorts of weird behaviour. Signed-off-by: Maxime Ripard <[email protected]>
The current code passes the dlist pointer to vc4_plane_write_dlist to tell where in the dlist RAM to store the plane dlist content. In order to ease later changes, let's move that direct pointer to a base offset from the start of the dlist RAM. Signed-off-by: Maxime Ripard <[email protected]>
During normal operations, the cursor position update is done through an asynchronous plane update, which on the vc4 driver basically just modifies the right dlist word to move the plane to the new coordinates. However, when we have the overscan margins setup, we fall back to a regular commit when we are next to the edges. And since that commit happens to be on a cursor plane, it's considered a legacy cursor update by KMS. The main difference it makes is that it won't wait for its completion (ie, next vblank) before returning. This means if we have multiple commits happening in rapid succession, we can have several of them happening before the next vblank. In parallel, our dlist allocation is tied to a CRTC state, and each time we do a commit we end up with a new CRTC state, with the previous one being freed. This means that we free our previous dlist entry (but don't clear it though) every time a new one is being committed. Now, if we were to have two commits happening before the next vblank, we could end up freeing reusing the same dlist entries before the next vblank. Indeed, we would start from an initial state taking, for example, the dlist entries 10 to 20, then start a commit taking the entries 20 to 30 and setting the dlist pointer to 20, and freeing the dlist entries 10 to 20. However, since we haven't reach vblank yet, the HVS is still using the entries 10 to 20. If we were to make a new commit now, chances are the allocator are going to give the 10 to 20 entries back, and we would change their content to match the new state. If vblank hasn't happened yet, we just corrupted the active dlist entries. In order to avoid this, let's create an intermediate dlist buffer to store the current (ie, as of the last commit) dlist content, that we will update each time the HVS is done with a frame. This will avoid overwriting the dlist being used in hardware while it's live. Signed-off-by: Maxime Ripard <[email protected]>
mripard
force-pushed
the
rpi/5.15-cursor-corruption
branch
from
09d4ec3 to
bc7aafd
Compare
|
I see this is a PR against 5.15 - will it port back to 5.10 or will that require a rewrite? |
|
I should have mentioned that indeed. 5.10 seems immune to it as long as we have the reverts @popcornmix did last summer, and especially 2224168. So we could reintroduce the original commits and apply that on top, but I don't think it's worth the trouble. |
|
Thanks - that fixes it for me. |
|
While this fixes the cursor close to overscan issue, the other (seemingly related) issue is still present. Using BRANCH=next rpi-update kernel (which includes this PR) If I transverse screens with the mouse slowly, no glitch occurs. Rapidly switching mouse between screens makes the glitch very frequent. |
|
With 5.10 kernel the issue does not occur: |
kernel: Add V4L2_CID_NOTIFY_GAINS control See: raspberrypi/linux#4782 kernel: Fix for cursor corruption with overscan See: raspberrypi/linux#4776 kernel: Add panel driver NWE080 and overlay for CutiePi See: raspberrypi/linux#4742 firmware: ldconfig: Discard subsequent chunks from a truncated line See: #1669 firmware: cec: Fail set_passive_mode when running with kms firmware: Firmware: Remove PWM/audio traits for CM4 firmware: usb: Fix non-BCM2711 MSD support See: raspberrypi/usbboot#102
kernel: Add V4L2_CID_NOTIFY_GAINS control See: raspberrypi/linux#4782 kernel: Fix for cursor corruption with overscan See: raspberrypi/linux#4776 kernel: Add panel driver NWE080 and overlay for CutiePi See: raspberrypi/linux#4742 firmware: ldconfig: Discard subsequent chunks from a truncated line See: raspberrypi/firmware#1669 firmware: cec: Fail set_passive_mode when running with kms firmware: Firmware: Remove PWM/audio traits for CM4 firmware: usb: Fix non-BCM2711 MSD support See: raspberrypi/usbboot#102
|
I spent some time looking into this. It seems that it only occurs on the edge that is next to the other monitor, but the cursor doesn't have to cross from one display to the other. If you just move the cursor along the edge, some corruptions occur as well. I'm not quite sure what's going on at that point. The interrupts fire properly, the dlist seem to be copied over properly, and it really only occurs when we're close to the other display I'm wondering if it's not due to the cursor plane being larger than the actual image displayed in it, and we have it shared between both displays |
|
There should be no issue with a source element being displayed on two displays from a hardware perspective. As far as the hardware is concerned it's just the source address being used for reads. It really doesn't care where they come from in memory. Now if the images are being resized vertically then the context memory used by hardware for this needs to distinct for each element. |
Hi,
Here's a PR that fixes #4475.
The last commit has most of the details, but the basic idea is that when we move the cursor along the edges, we end up reusing and corrupting the active dlist.
Let me know what you think,
Maxime