[Intel-gfx] [PATCH 1/3] drm: add writeback pointers to drm_connector

[Abhinav Kumar]

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 at quicinc.com>

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
>