| Message ID | 20220111101801.28310-1-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm: add writeback pointers to drm_connector | expand |
+ laurent on this Hi Suraj On 1/11/2022 2:17 AM, Kandpal, Suraj wrote: > Changing drm_connector and drm_encoder feilds to pointers in > drm_writeback_connector as the elements of struct > drm_writeback_connector are: > struct drm_writeback_connector { > struct drm_connector base; > struct drm_encoder encoder; > Similarly the elements of intel_encoder and intel_connector > are: > struct intel_encoder { > struct drm_encoder base; > > struct intel_connector { > struct drm_connector base; > > The function drm_writeback_connector_init() will initialize the > drm_connector and drm_encoder and attach them as well. > Since the drm_connector/encoder are both struct in > drm_writeback_connector and intel_connector/encoder, we need > one of them to be a pointer so we can reference them or else we > will be pointing to 2 seprate instances. > > Usually the struct defined in drm framework pointing to any > struct will be pointer and allocating them and initialization > will be done with the users. > Like struct drm_connector and drm_encoder are part of drm > framework and the users of these such as i915 have included them > in their struct intel_connector and intel_encoder. Likewise > struct drm_writeback_connector is a special connector and hence > is not a user of drm_connector and hence this should be pointers. > > Adding drm_writeback_connector to drm_connector so that > writeback_connector can be fetched from drm_connector as the previous > container_of method won't work due to change in the feilds of > drm_connector and drm_encoder in drm_writeback_connector. > > Note:The corresponding ripple effect due to the above changes namely in > two drivers as I can see it komeda and vkms have been dealt with in the > upcoming patches of this series. > > Signed-off-by: Kandpal, Suraj <suraj.kandpal@intel.com> 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. > --- > drivers/gpu/drm/drm_writeback.c | 19 ++++++++++--------- > include/drm/drm_connector.h | 3 +++ > include/drm/drm_writeback.h | 6 +++--- > 3 files changed, 16 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > index dccf4504f1bb..47238db42363 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -87,7 +87,7 @@ static const char *drm_writeback_fence_get_driver_name(struct dma_fence *fence) > struct drm_writeback_connector *wb_connector = > fence_to_wb_connector(fence); > > - return wb_connector->base.dev->driver->name; > + return wb_connector->base->dev->driver->name; > } > > static const char * > @@ -177,7 +177,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > const u32 *formats, int n_formats) > { > struct drm_property_blob *blob; > - struct drm_connector *connector = &wb_connector->base; > + struct drm_connector *connector = wb_connector->base; > struct drm_mode_config *config = &dev->mode_config; > int ret = create_writeback_properties(dev); > > @@ -189,14 +189,15 @@ int drm_writeback_connector_init(struct drm_device *dev, > if (IS_ERR(blob)) > return PTR_ERR(blob); > > - drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); > - ret = drm_encoder_init(dev, &wb_connector->encoder, > + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); > + ret = drm_encoder_init(dev, wb_connector->encoder, > &drm_writeback_encoder_funcs, > DRM_MODE_ENCODER_VIRTUAL, NULL); > if (ret) > goto fail; > > connector->interlace_allowed = 0; > + connector->wb_connector = wb_connector; > > ret = drm_connector_init(dev, connector, con_funcs, > DRM_MODE_CONNECTOR_WRITEBACK); > @@ -204,7 +205,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > goto connector_fail; > > ret = drm_connector_attach_encoder(connector, > - &wb_connector->encoder); > + wb_connector->encoder); > if (ret) > goto attach_fail; > > @@ -233,7 +234,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > attach_fail: > drm_connector_cleanup(connector); > connector_fail: > - drm_encoder_cleanup(&wb_connector->encoder); > + drm_encoder_cleanup(wb_connector->encoder); > fail: > drm_property_blob_put(blob); > return ret; > @@ -263,7 +264,7 @@ int drm_writeback_prepare_job(struct drm_writeback_job *job) > { > struct drm_writeback_connector *connector = job->connector; > const struct drm_connector_helper_funcs *funcs = > - connector->base.helper_private; > + connector->base->helper_private; > int ret; > > if (funcs->prepare_writeback_job) { > @@ -315,7 +316,7 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > { > struct drm_writeback_connector *connector = job->connector; > const struct drm_connector_helper_funcs *funcs = > - connector->base.helper_private; > + connector->base->helper_private; > > if (job->prepared && funcs->cleanup_writeback_job) > funcs->cleanup_writeback_job(connector, job); > @@ -401,7 +402,7 @@ drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector) > { > struct dma_fence *fence; > > - if (WARN_ON(wb_connector->base.connector_type != > + if (WARN_ON(wb_connector->base->connector_type != > DRM_MODE_CONNECTOR_WRITEBACK)) > return NULL; > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index b501d0badaea..ec4657cfd7b9 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -44,6 +44,7 @@ struct drm_printer; > struct drm_privacy_screen; > struct edid; > struct i2c_adapter; > +struct drm_writeback_connector; > > enum drm_connector_force { > DRM_FORCE_UNSPECIFIED, > @@ -1533,6 +1534,8 @@ struct drm_connector { > */ > struct drm_encoder *encoder; > > + struct drm_writeback_connector *wb_connector; > + > #define MAX_ELD_BYTES 128 > /** @eld: EDID-like data, if present */ > uint8_t eld[MAX_ELD_BYTES]; > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > index 9697d2714d2a..078c9907219c 100644 > --- a/include/drm/drm_writeback.h > +++ b/include/drm/drm_writeback.h > @@ -22,7 +22,7 @@ struct drm_writeback_connector { > /** > * @base: base drm_connector object > */ > - struct drm_connector base; > + struct drm_connector *base; > > /** > * @encoder: Internal encoder used by the connector to fulfill > @@ -31,7 +31,7 @@ struct drm_writeback_connector { > * by passing the @enc_funcs parameter to drm_writeback_connector_init() > * function. > */ > - struct drm_encoder encoder; > + struct drm_encoder *encoder; > > /** > * @pixel_formats_blob_ptr: > @@ -143,7 +143,7 @@ struct drm_writeback_job { > static inline struct drm_writeback_connector * > drm_connector_to_writeback(struct drm_connector *connector) > { > - return container_of(connector, struct drm_writeback_connector, base); > + return connector->wb_connector; > } > > int drm_writeback_connector_init(struct drm_device *dev,
> > + 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
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@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 >
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@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
>>
Hey, > 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) > I will have a look at this and try to resolve it > > Looks like vc4 doesnt have its own encoder so we might have to move it to > have its own? Right seems like we will have to add a drm_connector and drm_encoder in the below structure > > 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); } > Changes will be required here in both connector_of functions to point to The new drm_connector and drm_encoder we add in vc4_txp struct. > > 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; > Yes this will need to be done Thanks Abhinav for pointing Ill implement this and send it in the next version of patches Regards, Suraj Kandpal
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504f1bb..47238db42363 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -87,7 +87,7 @@ static const char *drm_writeback_fence_get_driver_name(struct dma_fence *fence) struct drm_writeback_connector *wb_connector = fence_to_wb_connector(fence); - return wb_connector->base.dev->driver->name; + return wb_connector->base->dev->driver->name; } static const char * @@ -177,7 +177,7 @@ int drm_writeback_connector_init(struct drm_device *dev, const u32 *formats, int n_formats) { struct drm_property_blob *blob; - struct drm_connector *connector = &wb_connector->base; + struct drm_connector *connector = wb_connector->base; struct drm_mode_config *config = &dev->mode_config; int ret = create_writeback_properties(dev); @@ -189,14 +189,15 @@ int drm_writeback_connector_init(struct drm_device *dev, if (IS_ERR(blob)) return PTR_ERR(blob); - drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); - ret = drm_encoder_init(dev, &wb_connector->encoder, + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); + ret = drm_encoder_init(dev, wb_connector->encoder, &drm_writeback_encoder_funcs, DRM_MODE_ENCODER_VIRTUAL, NULL); if (ret) goto fail; connector->interlace_allowed = 0; + connector->wb_connector = wb_connector; ret = drm_connector_init(dev, connector, con_funcs, DRM_MODE_CONNECTOR_WRITEBACK); @@ -204,7 +205,7 @@ int drm_writeback_connector_init(struct drm_device *dev, goto connector_fail; ret = drm_connector_attach_encoder(connector, - &wb_connector->encoder); + wb_connector->encoder); if (ret) goto attach_fail; @@ -233,7 +234,7 @@ int drm_writeback_connector_init(struct drm_device *dev, attach_fail: drm_connector_cleanup(connector); connector_fail: - drm_encoder_cleanup(&wb_connector->encoder); + drm_encoder_cleanup(wb_connector->encoder); fail: drm_property_blob_put(blob); return ret; @@ -263,7 +264,7 @@ int drm_writeback_prepare_job(struct drm_writeback_job *job) { struct drm_writeback_connector *connector = job->connector; const struct drm_connector_helper_funcs *funcs = - connector->base.helper_private; + connector->base->helper_private; int ret; if (funcs->prepare_writeback_job) { @@ -315,7 +316,7 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) { struct drm_writeback_connector *connector = job->connector; const struct drm_connector_helper_funcs *funcs = - connector->base.helper_private; + connector->base->helper_private; if (job->prepared && funcs->cleanup_writeback_job) funcs->cleanup_writeback_job(connector, job); @@ -401,7 +402,7 @@ drm_writeback_get_out_fence(struct drm_writeback_connector *wb_connector) { struct dma_fence *fence; - if (WARN_ON(wb_connector->base.connector_type != + if (WARN_ON(wb_connector->base->connector_type != DRM_MODE_CONNECTOR_WRITEBACK)) return NULL; diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index b501d0badaea..ec4657cfd7b9 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -44,6 +44,7 @@ struct drm_printer; struct drm_privacy_screen; struct edid; struct i2c_adapter; +struct drm_writeback_connector; enum drm_connector_force { DRM_FORCE_UNSPECIFIED, @@ -1533,6 +1534,8 @@ struct drm_connector { */ struct drm_encoder *encoder; + struct drm_writeback_connector *wb_connector; + #define MAX_ELD_BYTES 128 /** @eld: EDID-like data, if present */ uint8_t eld[MAX_ELD_BYTES]; diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d2714d2a..078c9907219c 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -22,7 +22,7 @@ struct drm_writeback_connector { /** * @base: base drm_connector object */ - struct drm_connector base; + struct drm_connector *base; /** * @encoder: Internal encoder used by the connector to fulfill @@ -31,7 +31,7 @@ struct drm_writeback_connector { * by passing the @enc_funcs parameter to drm_writeback_connector_init() * function. */ - struct drm_encoder encoder; + struct drm_encoder *encoder; /** * @pixel_formats_blob_ptr: @@ -143,7 +143,7 @@ struct drm_writeback_job { static inline struct drm_writeback_connector * drm_connector_to_writeback(struct drm_connector *connector) { - return container_of(connector, struct drm_writeback_connector, base); + return connector->wb_connector; } int drm_writeback_connector_init(struct drm_device *dev,
Changing drm_connector and drm_encoder feilds to pointers in drm_writeback_connector as the elements of struct drm_writeback_connector are: struct drm_writeback_connector { struct drm_connector base; struct drm_encoder encoder; Similarly the elements of intel_encoder and intel_connector are: struct intel_encoder { struct drm_encoder base; struct intel_connector { struct drm_connector base; The function drm_writeback_connector_init() will initialize the drm_connector and drm_encoder and attach them as well. Since the drm_connector/encoder are both struct in drm_writeback_connector and intel_connector/encoder, we need one of them to be a pointer so we can reference them or else we will be pointing to 2 seprate instances. Usually the struct defined in drm framework pointing to any struct will be pointer and allocating them and initialization will be done with the users. Like struct drm_connector and drm_encoder are part of drm framework and the users of these such as i915 have included them in their struct intel_connector and intel_encoder. Likewise struct drm_writeback_connector is a special connector and hence is not a user of drm_connector and hence this should be pointers. Adding drm_writeback_connector to drm_connector so that writeback_connector can be fetched from drm_connector as the previous container_of method won't work due to change in the feilds of drm_connector and drm_encoder in drm_writeback_connector. Note:The corresponding ripple effect due to the above changes namely in two drivers as I can see it komeda and vkms have been dealt with in the upcoming patches of this series. Signed-off-by: Kandpal, Suraj <suraj.kandpal@intel.com> --- drivers/gpu/drm/drm_writeback.c | 19 ++++++++++--------- include/drm/drm_connector.h | 3 +++ include/drm/drm_writeback.h | 6 +++--- 3 files changed, 16 insertions(+), 12 deletions(-)