Hi Abhinav
On Fri, Mar 11, 2022 at 09:47:17AM -0800, Abhinav Kumar wrote:
> On 3/10/2022 11:28 PM, Laurent Pinchart wrote:
> > On Thu, Mar 10, 2022 at 05:49:59PM -0800, Abhinav Kumar wrote:
> >> Make changes to rcar_du driver to start using drm_encoder pointer
> >> for drm_writeback_connector.
> >>
> >> Co-developed-by: Kandpal Suraj <suraj.kandpal@xxxxxxxxx>
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >> index c79d125..03930ad 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> >> @@ -200,7 +200,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> >> {
> >> struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
> >>
> >> - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> >> + wb_conn->encoder = kzalloc(sizeof(struct drm_encoder), GFP_KERNEL);
> >
> > Where is this freed ?
>
> You are right, this isnt. Looking more into this, it seems like moving
> the allocation of encoder to drm_writeback.c for cases which dont pass a
> real encoder is much better so that I will not have to add alloc() /
> free() code for all the vendor driver changes which is what I originally
> thought in my RFC but changed my mind because of below.
Yes, I think that would be better indeed. You could even skip the
dynamic allocation, you could have
struct drm_encoder *encoder;
struct drm_encoder internal_encoder;
in drm_writeback_connector, and set
wb->encoder = &wb->internal_encoder;
when the user doesn't pass an encoder.
> >> + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>
> Do you think we can just move usage of wb_conn->encoder->possible_crtcs
> just right after drm_writeback_connector_init() so that it wont crash?
How about adding a possible_crtcs argument to
drm_writeback_connector_init() (to cover existing use cases), and adding
a new drm_writeback_connector_init_with_encoder() that would get an
encoder pointer (and expect possible_crtcs, as well as all the other
appropriate encoder fields, having been initialized) ?
> 198 int rcar_du_writeback_init(struct rcar_du_device *rcdu,
> 199 struct rcar_du_crtc *rcrtc)
> 200 {
> 201 struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
> 202
> 203 wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
> 204 drm_connector_helper_add(&wb_conn->base,
> 205 &rcar_du_wb_conn_helper_funcs);
> 206
> 207 return drm_writeback_connector_init(&rcdu->ddev, wb_conn,
> 208 &rcar_du_wb_conn_funcs,
> 209 &rcar_du_wb_enc_helper_funcs,
> 210 writeback_formats,
> 211 ARRAY_SIZE(writeback_formats));
> 212 }
>
> >> drm_connector_helper_add(&wb_conn->base,
> >> &rcar_du_wb_conn_helper_funcs);
> >>
--
Regards,
Laurent Pinchart
