I am exploring the way of implementing the pipe writeback feature in i915 and would like to get early feedback on design. We have a Wireless display(WD) transcoder which can be used for capturing display pipe output to memory. It is generally intended for wireless display, but can be used for other functions such as in validation automation where crc based comparison is not feasible. Bspec: 49275 DRM core provides writeback connectors framework (drm_writeback.c) which can be used to expose hardware which can write the output from a pipe to a memory buffer. Writeback connectors have some additional properties, which userspace can use to query and control them, For more details, please refer [1] [1] https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html#writeback-connectors In order to implement pipe writeback feature in i915 using drm writeback connector framework, I am exploring below possibilities. 1. Extend the intel_connector to support writeback 2. Introduce new intel_writeback_connector type 3. ?? (any other better way?) 1# Extend the intel_connector to support writeback -------------------------------------------------- drm_writeback connector is of drm_connector type and intel_connector is also of drm_connector type. +-----------------------------------------------------------------------------+ | | | | struct drm_writeback_connector { | struct intel_connector { | | struct drm_connector base; | struct drm_connector base; | | . | . | | . | . | | . | . | | }; | }; | | | | +-----------------------------------------------------------------------------+ I see below issues for extending intel_connector to support drm_writeback_connector - extending intel_connector as drm_writeback connector will introduce 2 copies of drm_connector. struct intel_connector { struct drm_connector base; // new addition to handle wirteback struct drm_writeback_connector wb_conn; . . . }; - drm_writeback_connector_init() will initalize wb_conn.base (drm_connector) and we extract intel_encoder related functions (update_prepare, pre_enable, disable, etc) from intel_connector.base (which will never get initialized for writeback connector use case) e.g. intel_display.c static void intel_encoders_update_prepare(struct intel_atomic_state *state) { . . . intel_connector = to_intel_connector(connector); encoder = intel_connector_primary_encoder(intel_connector); if (!encoder->update_prepare) continue; crtc = new_conn_state->crtc ? to_intel_crtc(new_conn_state->crtc) : NULL; encoder->update_prepare(state, encoder, crtc); } Extending intel_connector to support drm_writeback_connector does not seem to be logical to me. Am I missing anything here? Can someone suggest a better approach? 2. Introduce new intel_writeback_connector connector type --------------------------------------------------------- I feel introducing the intel_writeback_connector is a logical approach, as it will follow the standard way of derivation from drm core structs (like intel_connector -> drm_connector, intel_encoder -> drm_encoder, etc) struct intel_writeback_connector { struct drm_writeback_connector base; . . . }; And, I am thinking of below design - +--------------------------------------------------------------+ | DRM CORE | | | | +-------------------------+ +--------------------+ | | | drm writeback connector |------>| drm connector | | | +-------------------------+ +--------------------+ | | ^ ^ | +------|--------------------------------------------------|----+ | ^ | | | | | v | | +--------------------------------+ | | | intel display | | | | (intel_display.c) | | | +--------------------------------+ | | ^ ^ | | | | | | v v | +-----------+ +-----------+ +-----------+ +-----------+ | intel | | | | intel | | intel | | writeback |<------| intel wd | ... | hdmi |------>| connector| | connector | | | | | | | +-----------+ +-----------+ +-----------+ +-----------+ (intel_writeback (intel_wd.c) (intel_hdmi.c) (intel_connector.c) _connector.c) - Introduce intel_writeback_connector struct which will be of drm_writeback_connector type. - intel_writeback_connector.c: Will have intel writeback connector helper functions. - intel_wd.c: will register the drm_writeback_connector and will have WD transcoder implementation To support this design, we will have to modify the intel_connector related macros and portion display driver where intel_connector related calls are made(and may be some other part of code). Ex: - Identify the drm_connector type (drm_connector.connector_type, DRM_MODE_CONNECTOR_WRITEBACK) and based on connector type select either intel_connector or intel_writeback_connector and identify correct intel_encoder to make encoder related calls (update_prepare, pre_enable, disable, etc) Is this the right approach? Do you see any issues/challenges with this? I would like to get early feedback before I really dive into code. Your thoughts and suggestions are much appreciated. Thanks, Pankaj _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
