| Message ID | 20240814-google-remove-crtc-index-from-parameter-v1-11-6e179abf9fd4@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: Introduce detailed configuration | expand |
Hi, On Wed, Aug 14, 2024 at 04:36:33PM GMT, Louis Chauvet wrote: > Currently drm_writeback_connector are created by > drm_writeback_connector_init or drm_writeback_connector_init_with_encoder. > Both of the function uses drm_connector_init and drm_encoder_init, but > there is no way to properly clean those structure from outside. > > This patch introduce the new function drm_writeback_connector_cleanup to > allow a proper cleanup. > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > --- > drivers/gpu/drm/drm_writeback.c | 10 ++++++++++ > include/drm/drm_writeback.h | 11 +++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > index a031c335bdb9..505a4eb40f93 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); > > wb_connector->encoder.possible_crtcs = possible_crtcs; > + wb_connector->managed_encoder = true; > > ret = drm_encoder_init(dev, &wb_connector->encoder, > &drm_writeback_encoder_funcs, > @@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder); > > +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector) > +{ > + drm_connector_cleanup(&wb_connector->base); > + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr); > + if (wb_connector->managed_encoder) > + drm_encoder_cleanup(&wb_connector->encoder); > +} > +EXPORT_SYMBOL(drm_writeback_connector_cleanup); > + > int drm_writeback_set_fb(struct drm_connector_state *conn_state, > struct drm_framebuffer *fb) > { > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > index 17e576c80169..e651c0c0c84c 100644 > --- a/include/drm/drm_writeback.h > +++ b/include/drm/drm_writeback.h > @@ -35,6 +35,15 @@ struct drm_writeback_connector { > */ > struct drm_encoder encoder; > > + /** > + * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init() > + * > + * If the user used drm_writeback_connector_init_with_encoder() to create the connector, > + * @encoder is not valid and not managed by drm_writeback_connector. This fields allows > + * the drm_writeback_cleanup() function to properly destroy the encoder if needed. > + */ > + bool managed_encoder; > + I think we should rather create drmm_writeback_connector variants, and make both deprecated in favor of these new functions. Maxime
Le 27/08/24 - 16:33, Maxime Ripard a écrit : > Hi, > > On Wed, Aug 14, 2024 at 04:36:33PM GMT, Louis Chauvet wrote: > > Currently drm_writeback_connector are created by > > drm_writeback_connector_init or drm_writeback_connector_init_with_encoder. > > Both of the function uses drm_connector_init and drm_encoder_init, but > > there is no way to properly clean those structure from outside. > > > > This patch introduce the new function drm_writeback_connector_cleanup to > > allow a proper cleanup. > > > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > > --- > > drivers/gpu/drm/drm_writeback.c | 10 ++++++++++ > > include/drm/drm_writeback.h | 11 +++++++++++ > > 2 files changed, 21 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > index a031c335bdb9..505a4eb40f93 100644 > > --- a/drivers/gpu/drm/drm_writeback.c > > +++ b/drivers/gpu/drm/drm_writeback.c > > @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > > drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); > > > > wb_connector->encoder.possible_crtcs = possible_crtcs; > > + wb_connector->managed_encoder = true; > > > > ret = drm_encoder_init(dev, &wb_connector->encoder, > > &drm_writeback_encoder_funcs, > > @@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev, > > } > > EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder); > > > > +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector) > > +{ > > + drm_connector_cleanup(&wb_connector->base); > > + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr); > > + if (wb_connector->managed_encoder) > > + drm_encoder_cleanup(&wb_connector->encoder); > > +} > > +EXPORT_SYMBOL(drm_writeback_connector_cleanup); > > + > > int drm_writeback_set_fb(struct drm_connector_state *conn_state, > > struct drm_framebuffer *fb) > > { > > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > > index 17e576c80169..e651c0c0c84c 100644 > > --- a/include/drm/drm_writeback.h > > +++ b/include/drm/drm_writeback.h > > @@ -35,6 +35,15 @@ struct drm_writeback_connector { > > */ > > struct drm_encoder encoder; > > > > + /** > > + * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init() > > + * > > + * If the user used drm_writeback_connector_init_with_encoder() to create the connector, > > + * @encoder is not valid and not managed by drm_writeback_connector. This fields allows > > + * the drm_writeback_cleanup() function to properly destroy the encoder if needed. > > + */ > > + bool managed_encoder; > > + > > I think we should rather create drmm_writeback_connector variants, > and make both deprecated in favor of these new functions. Hi, I can try to do it. If I understand correctly, you want to create two functions like this? int drmm_writeback_connector_init([...]) { /* drmm and alloc as we want to let drm core to manage this encoder, no need to store it in drm_writeback_connector */ enc = drmm_plain_encoder_alloc(...); return drmm_writeback_connector_init_with_encoder([...], enc); } int drmm_writeback_connector_init_with_encoder([...], enc) { con = drmm_connector_init([...]); drm_connector_attach_encoder(enc, con); /* Needed for pixel_formats_blob_ptr, base is already managed by drmm_connector_init. Maybe cleaning job_queue is also needed? */ drmm_add_action_or_reset([...], &drm_writeback_connector_cleanup) } Louis Chauvet > Maxime
On Tue, 27 Aug 2024, Louis Chauvet <louis.chauvet@bootlin.com> wrote: > Le 27/08/24 - 16:33, Maxime Ripard a écrit : >> Hi, >> >> On Wed, Aug 14, 2024 at 04:36:33PM GMT, Louis Chauvet wrote: >> > Currently drm_writeback_connector are created by >> > drm_writeback_connector_init or drm_writeback_connector_init_with_encoder. >> > Both of the function uses drm_connector_init and drm_encoder_init, but >> > there is no way to properly clean those structure from outside. >> > >> > This patch introduce the new function drm_writeback_connector_cleanup to >> > allow a proper cleanup. >> > >> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> >> > --- >> > drivers/gpu/drm/drm_writeback.c | 10 ++++++++++ >> > include/drm/drm_writeback.h | 11 +++++++++++ >> > 2 files changed, 21 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c >> > index a031c335bdb9..505a4eb40f93 100644 >> > --- a/drivers/gpu/drm/drm_writeback.c >> > +++ b/drivers/gpu/drm/drm_writeback.c >> > @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev, >> > drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); >> > >> > wb_connector->encoder.possible_crtcs = possible_crtcs; >> > + wb_connector->managed_encoder = true; >> > >> > ret = drm_encoder_init(dev, &wb_connector->encoder, >> > &drm_writeback_encoder_funcs, >> > @@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev, >> > } >> > EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder); >> > >> > +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector) >> > +{ >> > + drm_connector_cleanup(&wb_connector->base); >> > + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr); >> > + if (wb_connector->managed_encoder) >> > + drm_encoder_cleanup(&wb_connector->encoder); >> > +} >> > +EXPORT_SYMBOL(drm_writeback_connector_cleanup); >> > + >> > int drm_writeback_set_fb(struct drm_connector_state *conn_state, >> > struct drm_framebuffer *fb) >> > { >> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h >> > index 17e576c80169..e651c0c0c84c 100644 >> > --- a/include/drm/drm_writeback.h >> > +++ b/include/drm/drm_writeback.h >> > @@ -35,6 +35,15 @@ struct drm_writeback_connector { >> > */ >> > struct drm_encoder encoder; >> > >> > + /** >> > + * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init() >> > + * >> > + * If the user used drm_writeback_connector_init_with_encoder() to create the connector, >> > + * @encoder is not valid and not managed by drm_writeback_connector. This fields allows >> > + * the drm_writeback_cleanup() function to properly destroy the encoder if needed. >> > + */ >> > + bool managed_encoder; >> > + >> >> I think we should rather create drmm_writeback_connector variants, >> and make both deprecated in favor of these new functions. > > Hi, > > I can try to do it. If I understand correctly, you want to create two > functions like this? > > int drmm_writeback_connector_init([...]) { > /* drmm and alloc as we want to let drm core to manage this > encoder, no need to store it in drm_writeback_connector > */ > enc = drmm_plain_encoder_alloc(...); > > return drmm_writeback_connector_init_with_encoder([...], enc); > } > > int drmm_writeback_connector_init_with_encoder([...], enc) { > con = drmm_connector_init([...]); > > drm_connector_attach_encoder(enc, con); > > /* Needed for pixel_formats_blob_ptr, base is already > managed by drmm_connector_init. Maybe cleaning > job_queue is also needed? */ > drmm_add_action_or_reset([...], &drm_writeback_connector_cleanup) > } Why add two variants, when you can have one and pass NULL for encoder? We have the _init_with_encoder variant only because nobody bothered to clean up existing call sites. Side note, I'd still like to be able to pass driver's own allocated connector instead of having writeback midlayer force it on you. BR, Jani. > > Louis Chauvet > >> Maxime > >
Le 28/08/24 - 11:35, Jani Nikula a écrit : > On Tue, 27 Aug 2024, Louis Chauvet <louis.chauvet@bootlin.com> wrote: > > Le 27/08/24 - 16:33, Maxime Ripard a écrit : > >> Hi, > >> > >> On Wed, Aug 14, 2024 at 04:36:33PM GMT, Louis Chauvet wrote: > >> > Currently drm_writeback_connector are created by > >> > drm_writeback_connector_init or drm_writeback_connector_init_with_encoder. > >> > Both of the function uses drm_connector_init and drm_encoder_init, but > >> > there is no way to properly clean those structure from outside. > >> > > >> > This patch introduce the new function drm_writeback_connector_cleanup to > >> > allow a proper cleanup. > >> > > >> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > >> > --- > >> > drivers/gpu/drm/drm_writeback.c | 10 ++++++++++ > >> > include/drm/drm_writeback.h | 11 +++++++++++ > >> > 2 files changed, 21 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > >> > index a031c335bdb9..505a4eb40f93 100644 > >> > --- a/drivers/gpu/drm/drm_writeback.c > >> > +++ b/drivers/gpu/drm/drm_writeback.c > >> > @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > >> > drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); > >> > > >> > wb_connector->encoder.possible_crtcs = possible_crtcs; > >> > + wb_connector->managed_encoder = true; > >> > > >> > ret = drm_encoder_init(dev, &wb_connector->encoder, > >> > &drm_writeback_encoder_funcs, > >> > @@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev, > >> > } > >> > EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder); > >> > > >> > +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector) > >> > +{ > >> > + drm_connector_cleanup(&wb_connector->base); > >> > + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr); > >> > + if (wb_connector->managed_encoder) > >> > + drm_encoder_cleanup(&wb_connector->encoder); > >> > +} > >> > +EXPORT_SYMBOL(drm_writeback_connector_cleanup); > >> > + > >> > int drm_writeback_set_fb(struct drm_connector_state *conn_state, > >> > struct drm_framebuffer *fb) > >> > { > >> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > >> > index 17e576c80169..e651c0c0c84c 100644 > >> > --- a/include/drm/drm_writeback.h > >> > +++ b/include/drm/drm_writeback.h > >> > @@ -35,6 +35,15 @@ struct drm_writeback_connector { > >> > */ > >> > struct drm_encoder encoder; > >> > > >> > + /** > >> > + * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init() > >> > + * > >> > + * If the user used drm_writeback_connector_init_with_encoder() to create the connector, > >> > + * @encoder is not valid and not managed by drm_writeback_connector. This fields allows > >> > + * the drm_writeback_cleanup() function to properly destroy the encoder if needed. > >> > + */ > >> > + bool managed_encoder; > >> > + > >> > >> I think we should rather create drmm_writeback_connector variants, > >> and make both deprecated in favor of these new functions. > > > > Hi, > > > > I can try to do it. If I understand correctly, you want to create two > > functions like this? > > > > int drmm_writeback_connector_init([...]) { > > /* drmm and alloc as we want to let drm core to manage this > > encoder, no need to store it in drm_writeback_connector > > */ > > enc = drmm_plain_encoder_alloc(...); > > > > return drmm_writeback_connector_init_with_encoder([...], enc); > > } > > > > int drmm_writeback_connector_init_with_encoder([...], enc) { > > con = drmm_connector_init([...]); > > > > drm_connector_attach_encoder(enc, con); > > > > /* Needed for pixel_formats_blob_ptr, base is already > > managed by drmm_connector_init. Maybe cleaning > > job_queue is also needed? */ > > drmm_add_action_or_reset([...], &drm_writeback_connector_cleanup) > > } > > Why add two variants, when you can have one and pass NULL for encoder? > We have the _init_with_encoder variant only because nobody bothered to > clean up existing call sites. I just followed the existing code, but yes, I can make only one function and create the encoder if the pointer is NULL. > Side note, I'd still like to be able to pass driver's own allocated > connector instead of having writeback midlayer force it on you. I just checked, it seems non-trivial to make this change and I don't feel confident to change this much the drm core. Louis Chauvet > BR, > Jani. > > > > > > Louis Chauvet > > > >> Maxime > > > > > > -- > Jani Nikula, Intel
On Wed, Aug 28, 2024 at 11:07:37AM GMT, Louis Chauvet wrote: > Le 28/08/24 - 11:35, Jani Nikula a écrit : > > On Tue, 27 Aug 2024, Louis Chauvet <louis.chauvet@bootlin.com> wrote: > > > Le 27/08/24 - 16:33, Maxime Ripard a écrit : > > >> Hi, > > >> > > >> On Wed, Aug 14, 2024 at 04:36:33PM GMT, Louis Chauvet wrote: > > >> > Currently drm_writeback_connector are created by > > >> > drm_writeback_connector_init or drm_writeback_connector_init_with_encoder. > > >> > Both of the function uses drm_connector_init and drm_encoder_init, but > > >> > there is no way to properly clean those structure from outside. > > >> > > > >> > This patch introduce the new function drm_writeback_connector_cleanup to > > >> > allow a proper cleanup. > > >> > > > >> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> > > >> > --- > > >> > drivers/gpu/drm/drm_writeback.c | 10 ++++++++++ > > >> > include/drm/drm_writeback.h | 11 +++++++++++ > > >> > 2 files changed, 21 insertions(+) > > >> > > > >> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > >> > index a031c335bdb9..505a4eb40f93 100644 > > >> > --- a/drivers/gpu/drm/drm_writeback.c > > >> > +++ b/drivers/gpu/drm/drm_writeback.c > > >> > @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > > >> > drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); > > >> > > > >> > wb_connector->encoder.possible_crtcs = possible_crtcs; > > >> > + wb_connector->managed_encoder = true; > > >> > > > >> > ret = drm_encoder_init(dev, &wb_connector->encoder, > > >> > &drm_writeback_encoder_funcs, > > >> > @@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev, > > >> > } > > >> > EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder); > > >> > > > >> > +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector) > > >> > +{ > > >> > + drm_connector_cleanup(&wb_connector->base); > > >> > + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr); > > >> > + if (wb_connector->managed_encoder) > > >> > + drm_encoder_cleanup(&wb_connector->encoder); > > >> > +} > > >> > +EXPORT_SYMBOL(drm_writeback_connector_cleanup); > > >> > + > > >> > int drm_writeback_set_fb(struct drm_connector_state *conn_state, > > >> > struct drm_framebuffer *fb) > > >> > { > > >> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > > >> > index 17e576c80169..e651c0c0c84c 100644 > > >> > --- a/include/drm/drm_writeback.h > > >> > +++ b/include/drm/drm_writeback.h > > >> > @@ -35,6 +35,15 @@ struct drm_writeback_connector { > > >> > */ > > >> > struct drm_encoder encoder; > > >> > > > >> > + /** > > >> > + * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init() > > >> > + * > > >> > + * If the user used drm_writeback_connector_init_with_encoder() to create the connector, > > >> > + * @encoder is not valid and not managed by drm_writeback_connector. This fields allows > > >> > + * the drm_writeback_cleanup() function to properly destroy the encoder if needed. > > >> > + */ > > >> > + bool managed_encoder; > > >> > + > > >> > > >> I think we should rather create drmm_writeback_connector variants, > > >> and make both deprecated in favor of these new functions. > > > > > > Hi, > > > > > > I can try to do it. If I understand correctly, you want to create two > > > functions like this? > > > > > > int drmm_writeback_connector_init([...]) { > > > /* drmm and alloc as we want to let drm core to manage this > > > encoder, no need to store it in drm_writeback_connector > > > */ > > > enc = drmm_plain_encoder_alloc(...); > > > > > > return drmm_writeback_connector_init_with_encoder([...], enc); > > > } > > > > > > int drmm_writeback_connector_init_with_encoder([...], enc) { > > > con = drmm_connector_init([...]); > > > > > > drm_connector_attach_encoder(enc, con); > > > > > > /* Needed for pixel_formats_blob_ptr, base is already > > > managed by drmm_connector_init. Maybe cleaning > > > job_queue is also needed? */ > > > drmm_add_action_or_reset([...], &drm_writeback_connector_cleanup) > > > } > > > > Why add two variants, when you can have one and pass NULL for encoder? > > We have the _init_with_encoder variant only because nobody bothered to > > clean up existing call sites. > > I just followed the existing code, but yes, I can make only one function > and create the encoder if the pointer is NULL. > > > Side note, I'd still like to be able to pass driver's own allocated > > connector instead of having writeback midlayer force it on you. > > I just checked, it seems non-trivial to make this change and I don't feel > confident to change this much the drm core. It's a great opportunity to add unit tests then :) Maxime
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index a031c335bdb9..505a4eb40f93 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev, drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); wb_connector->encoder.possible_crtcs = possible_crtcs; + wb_connector->managed_encoder = true; ret = drm_encoder_init(dev, &wb_connector->encoder, &drm_writeback_encoder_funcs, @@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev, } EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder); +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector) +{ + drm_connector_cleanup(&wb_connector->base); + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr); + if (wb_connector->managed_encoder) + drm_encoder_cleanup(&wb_connector->encoder); +} +EXPORT_SYMBOL(drm_writeback_connector_cleanup); + int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb) { diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 17e576c80169..e651c0c0c84c 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -35,6 +35,15 @@ struct drm_writeback_connector { */ struct drm_encoder encoder; + /** + * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init() + * + * If the user used drm_writeback_connector_init_with_encoder() to create the connector, + * @encoder is not valid and not managed by drm_writeback_connector. This fields allows + * the drm_writeback_cleanup() function to properly destroy the encoder if needed. + */ + bool managed_encoder; + /** * @pixel_formats_blob_ptr: * @@ -161,6 +170,8 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev, const struct drm_connector_funcs *con_funcs, const u32 *formats, int n_formats); +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector); + int drm_writeback_set_fb(struct drm_connector_state *conn_state, struct drm_framebuffer *fb);
Currently drm_writeback_connector are created by drm_writeback_connector_init or drm_writeback_connector_init_with_encoder. Both of the function uses drm_connector_init and drm_encoder_init, but there is no way to properly clean those structure from outside. This patch introduce the new function drm_writeback_connector_cleanup to allow a proper cleanup. Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com> --- drivers/gpu/drm/drm_writeback.c | 10 ++++++++++ include/drm/drm_writeback.h | 11 +++++++++++ 2 files changed, 21 insertions(+)