On Sat, 22 Feb 2025, at 7:57 AM, Jernej Škrabec wrote: > Hi Ryan, > > sorry for very late review, but here we go... No problem, thanks for the review! > This patchset actually introduces 3 disticnt features, which should IMO > be separated > and thus making reviewing patches easier. > > 1. native 10-bit YUV420 output (without YUV->RGB->YUV conversions) - > this is used on > HDMI for proper 10-bit 4K@60 HDR output > 2. Display Engine 3.3 (DE33) support > 3. AFBC modifier/format support (used for more efficient GPU or VPU > output rendering) > > If I'm not mistaken, your goal is only DE33 support. This is my initial goal, but I would still like good HDMI and video support (including HW decode). It took some refactoring and resequencing of (your) existing driver work to get to this series, and I have left out the HDMI and separated the TCON code for the same reason, that initially I am intending to just support RGB operation to an LCD. I do intend however to use the HDMI code either in or out of tree but think that will be a much bigger/more complex change to mainline given the current HDMI code is more invasive to the generic driver. The YUV and AFBC changes seemed more self-contained and seemed reasonable approaches given that they were both features of the DE3 and up. I am happy either to split these into either 4 or 5 separate patches ie: 1a. refactor CSC code to prepare for YUV 1b. add YUV for DE3 2. refactor mixer enumeration and regmaps to support DE3+ 3. add DE33 4. Add AFBC My only reluctance is that that adds more work to manage multiple patches which are logically grouped and in terms of ease of review, all these 4 steps are in the current set in that order (apart from AFBC which is completely standalone), and I don't think submitting them separately reduces complexity. It however will reduce reviewer burden as you suggest, but this has been a slow process already. I am happy to accept either process but this has been some time already now with lots of stylistic feedback but not much on the correctness of the approach and code, and I am keen to get this landed. There is are two > reasons why > I've never sent these patches myself: > > 1. scaling YUV images doesn't work > > Not a problem for any user who doesn't use video player with DRM plane > rendering, > which I assume is most of them. We can get around of this issue to deny > scaling > YUV buffers until the issue is sorted out. Good to know, I hadn't appreciated that. Mostly relevant for video as you say and it would be good to land YUV support without scaling, then extend subsequently, possibly when we get to the video engine? > 2. plane allocations are hackish > > DE33 for the first time introduced option to move planes between the > mixers. DRM > has also support for this, but for time being static allocation is > acceptable and tbh, > even dynamic support need appropriate initial setting. > As you might guessed this is done in DE33 clock driver using magic > values. Proper > way would be to introduce some kind of connection between mixer/plane > and clock > driver, so initial configuration can be moved to appropriate module > instead of > being thrown into clock driver. I need to check where to put it and how > to properly > connect DT nodes. Maybe syscon phandle? I'll do some research. Thanks, I was not aware of this either. > There is one glaring issue (when you work on driver and test every > aspect of it). > DE33 introduced RCQ, which is basically DMA, which copies shadowed > registers > from memory buffer directly to hw registers. IIRC it does that at > vblank time. This > tells you that current concept of writing register values at any time > userspace feels > to do commit is wrong and we've been lucky that it works as well as it > does. So, > in order to fix this, driver would need quite a big reorganization. > Introducing > shadow buffers would solve most of the issues, most likely also with > YUV scaling. > Implementing RCQ would be then relatively simple and even improve > timings. > Note that even DE3 has occasional issue with YUV scaler and also > read-modify-read > access to some of its register can produce bogus value and thus corrupt > image > until next commit. Thanks, again I wasn't aware. All my testing has shown remarkable stability and there are some downstream users out-of-tree with good feedback, but would be happy to support an effort to improve this. Regards, Ryan
