On Fri, 01 Aug 2025, Dmitry Baryshkov <dmitry.barysh...@oss.qualcomm.com> wrote: > Thinking in OOP terms, the encoder is just a field in the struct. > drm_connector is a base class for drm_writeback_connector. By making it > optional, you are definitely semi-breaking the abstraction.
The trouble is, in OOP terms, drm_connector is the "base class" for both drm_writeback_connector and intel_connector. We're already stretching what we can do with C. Currently, it's always guaranteed all drm_connectors i915 ever sees are embedded within intel_connector. Changing from one pointer to another is trivial, guaranteed to work, and is never NULL if the source pointer is non-NULL. This is a design that's been around for the longest time. The current writeback implementation forces a different design by always embedding drm_connector itself. We can't use a drm_connector that's embedded within an intel_connector with it. If we want to have our own stuff, we'd need an intel_writeback_connector wrapping drm_writeback_connector, and it gets even more complicated with all the interfaces that use intel_connector. It really shouldn't have to be this way. Using the current drm_writeback_connector in i915 requires careful auditing of all drm_connector <-> intel_connector conversions, NULL checks, and graceful error handling, also in places that have no convenient way to return errors at all. The OOP abstractions just break hard with C, we can't have multiple inheritance, and IMO the pragmatic approach is to let *drivers* do what they want, instead of having a midlayer helper design force something on them. BR, Jani. -- Jani Nikula, Intel
