Re: [PATCH 03/28] drm/writeback: Define function to get drm_connector from writeback

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 01 Aug 2025, Dmitry Baryshkov <dmitry.baryshkov@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