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

Tue Feb 1 01:42:36 UTC 2022

Hi Suraj

I think there are more places affected with this change. I can get below 
compilation issues while trying to compile my branch:

drivers/gpu/drm/vc4/vc4_txp.c: In function ‘encoder_to_vc4_txp’:
./include/linux/build_bug.h:78:41: error: static assertion failed: 
"pointer type mismatch in container_of()"
  #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
                                          ^
./include/linux/build_bug.h:77:34: note: in expansion of macro 
‘__static_assert’
  #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, 
#expr)
                                   ^
./include/linux/container_of.h:19:2: note: in expansion of macro 
‘static_assert’
   static_assert(__same_type(*(ptr), ((type *)0)->member) || \
   ^
drivers/gpu/drm/vc4/vc4_txp.c:162:9: note: in expansion of macro 
‘container_of’
   return container_of(encoder, struct vc4_txp, connector.encoder);
          ^
drivers/gpu/drm/vc4/vc4_txp.c: In function ‘connector_to_vc4_txp’:
./include/linux/build_bug.h:78:41: error: static assertion failed: 
"pointer type mismatch in container_of()"
  #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
                                          ^
./include/linux/build_bug.h:77:34: note: in expansion of macro 
‘__static_assert’
  #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, 
#expr)
                                   ^
./include/linux/container_of.h:19:2: note: in expansion of macro 
‘static_assert’
   static_assert(__same_type(*(ptr), ((type *)0)->member) || \
   ^
drivers/gpu/drm/vc4/vc4_txp.c:167:9: note: in expansion of macro 
‘container_of’
   return container_of(conn, struct vc4_txp, connector.base);
          ^
drivers/gpu/drm/vc4/vc4_txp.c: In function ‘vc4_txp_bind’:
drivers/gpu/drm/vc4/vc4_txp.c:495:27: error: passing argument 1 of 
‘drm_connector_helper_add’ from incompatible pointer type 
[-Werror=incompatible-pointer-types]
   drm_connector_helper_add(&txp->connector.base,
                            ^
In file included from ./include/drm/drm_atomic_helper.h:32:0,
                  from drivers/gpu/drm/vc4/vc4_txp.c:17:
./include/drm/drm_modeset_helper_vtables.h:1153:20: note: expected 
‘struct drm_connector *’ but argument is of type ‘struct drm_connector **’
  static inline void drm_connector_helper_add(struct drm_connector 
*connector,
                     ^
drivers/gpu/drm/vc4/vc4_txp.c:509:10: error: assignment from 
incompatible pointer type [-Werror=incompatible-pointer-types]
   encoder = &txp->connector.encoder;
           ^
drivers/gpu/drm/vc4/vc4_txp.c: In function ‘vc4_txp_unbind’:
drivers/gpu/drm/vc4/vc4_txp.c:532:28: error: passing argument 1 of 
‘vc4_txp_connector_destroy’ from incompatible pointer type 
[-Werror=incompatible-pointer-types]
   vc4_txp_connector_destroy(&txp->connector.base);
                             ^
drivers/gpu/drm/vc4/vc4_txp.c:333:13: note: expected ‘struct 
drm_connector *’ but argument is of type ‘struct drm_connector **’
  static void vc4_txp_connector_destroy(struct drm_connector *connector)

Looks like vc4 doesnt have its own encoder so we might have to move it
to have its own?

struct vc4_txp {
     struct vc4_crtc base;

     struct platform_device *pdev;

     struct drm_writeback_connector connector;

     void __iomem *regs;
     struct debugfs_regset32 regset;
};

static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder 
*encoder)
{
     return container_of(encoder, struct vc4_txp, connector.encoder);
}

static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector 
*conn)
{
     return container_of(conn, struct vc4_txp, connector.base);
}

One more thing, it looks like to me, we need to do one more thing.

Like intel does and what MSM will do, the encoder pointer of the wb 
connector has to point to the encoder struct .

 >> wb_conn = &intel_connector->wb_conn;
 >> wb_conn->base = &intel_connector->base;
 >> wb_conn->encoder = &intel_wd->base.base;

Thanks

Abhinav
On 1/27/2022 10:17 AM, Abhinav Kumar wrote:
> 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
>>