Hi Suraj
Thanks for the response.
I was not too worried about the intel driver as I am sure you must have
validated this change with that :)
My question was more for the other vendor writeback drivers.
Thanks for looking into the others and providing the snippets.
After looking at those, yes I also think it should work.
So, if others do not have any concern with this change,
Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
On 1/27/2022 1:33 AM, Kandpal, Suraj wrote:
+ 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
