| Message ID | 1646963400-25606-6-git-send-email-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow drm_writeback_connector to accept pointer to drm_encoder | expand |
Hi Abhinav Thank you for the patch. 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@intel.com> > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > 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 ? > + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); > drm_connector_helper_add(&wb_conn->base, > &rcar_du_wb_conn_helper_funcs); >
Hi Laurent On 3/10/2022 11:28 PM, Laurent Pinchart wrote: > Hi Abhinav > > Thank you for the patch. > > 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@intel.com> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> --- >> 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. > >> + 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? 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); >> >
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@intel.com> > >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > >> --- > >> 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); > >>
Hi Laurent Thank you for your inputs. I liked all the suggestions, hence I have incorporated those and pushed a v2. Thanks Abhinav On 3/13/2022 7:50 AM, Laurent Pinchart wrote: > 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@intel.com> >>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >>>> --- >>>> 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); >>>> >
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); + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); drm_connector_helper_add(&wb_conn->base, &rcar_du_wb_conn_helper_funcs);
Make changes to rcar_du driver to start using drm_encoder pointer for drm_writeback_connector. Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)