On Mon, 28 Feb 2022 at 11:00, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Suraj, > > On Sat, Feb 26, 2022 at 05:10:06AM +0000, Kandpal, Suraj wrote: > > Hi Abhinav, > > > > > Based on the discussion in this thread [1] , it seems like having drm_encoder > > > as a pointer seems to have merits for both of us and also in agreement with > > > the folks on this thread and has a better chance of an ack. > > > > > > However drm_connector is not. > > > > > > I had a brief look at your implementation. Any reason you need to go > > > through intel_connector here and not drm_writeback_connector directly? > > > > > > The reason I ask is that I see you are not using prepare_writeback_job hook. > > > If you use that, you can use the drm_writeback_connector passed on from > > > there instead of looping through your list like you are doing in > > > intel_find_writeback_connector. > > > > > > Also, none of the other entries of struct intel_connector are being used for > > > the writeback implementation. So does the drm_writeback_connector in > > > your implementation need to be an intel_connector when its anyway not > > > using other fields? Why cant it be just stored as a drm_writeback_connector > > > itself in your struct intel_wd. > > > > The reason we can't do that is i915 driver always assumes that all connectors > > present in device list is an intel connector and since drm_writeback_connector > > even though a faux connector in it's initialization calls drm_connector_init() > > and gets added to the drm device list and hence the i915 driver also expects > > a corresponding intel connector to go with it. In case I do try to make writeback > > connector standalone a lot of checks, a lot will have to be added all around the > > driver as there could be a chance that one of the drm_connector in the drm device > > list may not be an intel_connector. > > Yes right now not all fields of intel_connector are being used but we will be working > > on filling them as we add more functionality to writeback for example introducing > > content protection. > > So even if I do float the patch series with just drm_encoder as pointer it won't help > > us with our implementation if drm_connector isn't a pointer too. > > This is a direct consequence of the decision to use connectors for > writeback in the userspace API. This disrupts any code that assumes that > a connector is a connector. The problem isn't limited to kernelspace, > userspace has the same exact problem, which resulted in a hack to avoid > breaking everything. Userspace software that needs to deal with > writeback needs to set the DRM_CLIENT_CAP_WRITEBACK_CONNECTORS > capability to get the writeback connectors exposed by the kernel, but > more than that, a large refactoring is then often needed to chase all > code paths that assume a connector is a connector. > > I'm afraid the same applies to the kernel. Drivers that don't use > writeback are largely unaffected. Drievrs that want to use writeback > need to be refactored to properly handle the fact that writeback > connectors are special. i915 will need to go that way. Laurent, you have frown upon the idea of separating the connector from the drm_writeback_connector struct. What about the encoder? The msm code in question can be found at the patchwork: https://patchwork.freedesktop.org/series/99724/. This series depends on Intel's patch, but should give you the overall feeling of the code being shared between to-the-display and writeback pipelines. -- With best wishes Dmitry
