drivers: display: mcux_elcdif: rework frame completion logic

[danieldegrasse]

Revert "drivers: display: elcdif: Modify interrupt enablement"

This reverts commit 2068976.

We must keep the frame completion interrupt disabled until we send a new
frame to the eLCDIF, as the frame completion interrupt fires at each
vertical blank interval. If we keep it enabled, then the semaphore we
use to indicate the frame has been loaded by the eLCDIF will be posted
to when we do not have a frame queued, and calls to display_write will
return before the eLCDIF has actually loaded the new framebuffer.

Fixes #80590

[danieldegrasse]

@ngphibang and @trunghieulenxp, I would appreciate a review on this change

[ngphibang]

I understand the issue. The difference between the LVGL sample and the "camera -> display" sample is the presence (or not) of the PxP which can introduce (or not) the extra timing leading to (or not) the tearing effect.

Unfortunately, this fix will surely reduce the frame rate (that I guess by a half ?) for the camera->display sample. It is a pain because, currently, the camera -> display pipeline on RT1170 (camera -> PxP / eLCDIF) has only ~15 fps on display where we expect 30 fps (Note that the camera output is 30 fps). With this fix, it will be lower.

I hope that we will have time and enough information to fix all these problems in the future.

[danieldegrasse]

I understand the issue. The difference between the LVGL sample and the "camera -> display" sample is the presence (or not) of the PxP which can introduce (or not) the extra timing leading to (or not) the tearing effect.

Unfortunately, this fix will surely reduce the frame rate (that I guess by a half ?) for the camera->display sample. It is a pain because, currently, the camera -> display pipeline on RT1170 (camera -> PxP / eLCDIF) has only ~15 fps on display where we expect 30 fps (Note that the camera output is 30 fps). With this fix, it will be lower.

I hope that we will have time and enough information to fix all these problems in the future.

Sure, I agree- if you can determine a better fix here, I'm open to it. Does the camera->display sample currently block while sending data to the display, or is it possible for camera frames to be streamed while the display is writing data? I guess in theory we should be able to use two buffers, and fill one buffer from the camera while the other buffer streams to the display? I think that only works if the camera and display take exactly the same amount of time to stream a frame- otherwise you need 3 buffers total.

[trunghieulenxp]

Hi @danieldegrasse , i understand your issue, we have tested on the target RT1170. Effectively, we observe the problem of tearing effect when CONFIG_MCUX_ELCDIF_LP=n. The main reason of tearing effect come from my patch, so why don't we just reverse my patch. Could it be more efficient than adding two semaphores waiting for two vblanks ?
Thanks.

[ngphibang]

Does the camera->display sample currently block while sending data to the display, or is it possible for camera frames to be streamed while the display is writing data? I guess in theory we should be able to use two buffers, and fill one buffer from the camera while the other buffer streams to the display? I think that only works if the camera and display take exactly the same amount of time to stream a frame- otherwise you need 3 buffers total.

Yes, in theory, in the application, the camera and the display should be in two threads with a buffer pool large enough so that they can enqueue / dequeue buffers in parallel.

I wanted to modify the video/capture/sample to do so in the past but for RT1170 case, it does not help much as most of the CPU time spent on PxP + eLCDIF (~67 ms then reduced to ~55 ms after some optimizations If I remember well). That's why I didn't do it (but maybe I will make this change because it is logically better anyways).

About a better fix, what I aim to is "decoupling the PxP and eLCDIF" + "reworking the eLCDIF internal buffer management to have a real double buffering using FIFO queue" + "understanding the eLCDIF interrupt / frame done mechanism". But this will take much time.

For the time-being, do you think it's better if we just revert @trunghieulenxp 's interrupt patch than waiting for 2 vblank cycles as in the current commit ? Otherwise, both solutions are OK for me.

[danieldegrasse]

Hi @danieldegrasse , i understand your issue, we have tested on the target RT1170. Effectively, we observe the problem of tearing effect when CONFIG_MCUX_ELCDIF_LP=n. The main reason of tearing effect come from my patch, so why don't we just reverse my patch. Could it be more efficient than adding two semaphores waiting for two vblanks ? Thanks.

Considering this more, I think reverting the patch is better. The initial intention here was to wait until the eLCDIF had streamed the entire framebuffer before returning from display_write, but that actually doesn't matter- there are two cases here:

1. the application provided framebuffer is the size of the display, and the eLCDIF uses it directly. In this case, the application cannot safely reuse the framebuffer until it supplies a new buffer. We still need to wait for a vertical blank interval, but only to make sure the frame has actually been loaded into the eLCDIF
2. the application provided framebuffer is not the same size as the display, so we simply copy the framebuffer into a driver framebuffer. The application can reuse the framebuffer immediately

Based on this, I'm not sure there is any value in waiting for the second vertical blank interval- the eLCDIF will still be using the framebuffer, even after that point.

@ngphibang or @trunghieulenxp, do either of you have a preference here? I would lean towards reverting the patch I think.

[trunghieulenxp]

@ngphibang or @trunghieulenxp, do either of you have a preference here? I would lean towards reverting the patch I think.

Thanks for your explanation. In this case, I would prefer the revert solution, as we are uncertain about how many vertical blank periods, we need to wait for.

[danieldegrasse]

@ngphibang or @trunghieulenxp, do either of you have a preference here? I would lean towards reverting the patch I think.

Thanks for your explanation. In this case, I would prefer the revert solution, as we are uncertain about how many vertical blank periods, we need to wait for.

Ok, updated the PR to simply revert 2068976

[dkalowsk]

@danieldegrasse @dleach02 is this needed for 4.0?

[danieldegrasse]

@danieldegrasse @dleach02 is this needed for 4.0?

Yes, let me add the tag (sorry!)