> > + laurent on this > > Hi Suraj > Jani pointed me to this thread as i had posted something similar here : > https://patchwork.freedesktop.org/patch/470296/ but since this thread was > posted earlier, we can discuss further here. > > Overall, its similar to what I had posted in the RFC and your commit text also > covers my concerns too. > > One question I have about your change is since you have changed > wb_connector::encoder to be a pointer, i saw the other changes in the series > but they do not allocate an encoder. Would this not affect the other drivers > which are assuming that the encoder in wb_connector is struct drm_encoder > encoder and not struct drm_encoder* encoder. > > Your changes fix the compilation issue but wouldnt this crash as encoder > wasnt allocated for other drivers. > Hi Abhinav, That shouldn't be an issue as normally drivers tend to have their own output structure which has drm_connector and drm_encoder embedded in it depending on the level of binding they have decided to give the connector and encoder in their respective output and the addresses of these are passed to the drm_connector* and drm_encoder* in drm_writeback_connector structure which then gets initialized in drm_writeback_connector_init(). In our i915 code we have intel_connector structure with drm_connector base field and intel_wd with a intel_encoder base which in turn has drm_encoder field and both intel_connector and intel_wd are initialized not requiring explicit alloc and dealloc for drm_encoder intel_wd = kzalloc(sizeof(*intel_wd), GFP_KERNEL); intel_connector = intel_connector_alloc(); wb_conn = &intel_connector->wb_conn; wb_conn->base = &intel_connector->base; wb_conn->encoder = &intel_wd->base.base; Similary for komeda driver has struct komeda_wb_connector { struct drm_connector conn; /** @base: &drm_writeback_connector */ struct drm_writeback_connector base; /** @wb_layer: represents associated writeback pipeline of komeda */ struct komeda_layer *wb_layer; }; static int komeda_wb_connector_add(struct komeda_kms_dev *kms, struct komeda_crtc *kcrtc) { struct komeda_wb_connector *kwb_conn; struct drm_writeback_connector *wb_conn; kwb_conn = kzalloc(sizeof(*kwb_conn), GFP_KERNEL); wb_conn = &kwb_conn->base; wb_conn->base = &kwb_conn->conn; and they do not depend on the encoder binding so changes will work for them Also in vkms driver we have the struct vkms_output { struct drm_crtc crtc; struct drm_encoder encoder; struct drm_connector connector; struct drm_writeback_connector wb_connector; struct hrtimer vblank_hrtimer; ktime_t period_ns; struct drm_pending_vblank_event *event; /* ordered wq for composer_work */ struct workqueue_struct *composer_workq; /* protects concurrent access to composer */ spinlock_t lock; /* protected by @lock */ bool composer_enabled; struct vkms_crtc_state *composer_state; spinlock_t composer_lock; }; Which gets allocated covering for the drm_encoder alloc and dealloc Regards, Suraj Kandpal
