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@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
