On Fri, Aug 01, 2025 at 02:57:48PM +0300, Jani Nikula wrote: > 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.
Yes. Basically, that's why I suggest refacoring drm_writeback_connector to mvoe drm_connector_writeback into drm_connector itself (like it's done for drm_connector_hdmi). -- With best wishes Dmitry
