Le 17/09/24 - 18:02, José Expósito a écrit :
> Hey Louis,
>
> > 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. By using
> > drm managed variants, we can ensure that the writeback connector is
> > properly cleaned.
> >
> > This patch introduce drmm_writeback_connector_init, an helper to initialize
> > a writeback connector using drm managed helpers. This function allows the
> > caller to use its own encoder.
> >
> > Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
> > ---
> > Hi Maxime, Jani,
> >
> > I tried with this commit to implement the drm-managed version of writeback
> > connector initialization. I tested with the current vkms driver, and it
> > seems to works (at least no crash/warns).
> >
> > As suggested by Jani, I only created one function, which takes a
> > NULL-able pointer for encoder/encoder functions/possible_crtc. What do you
> > think about it?
> >
> > Regarding the documentation, I think I repeated too much, can I simply add
> > comments like "see documentation of @... for the details / requirements"?
> >
> > Good weekend,
> > Louis Chauvet
> > ---
> > drivers/gpu/drm/drm_writeback.c | 224 ++++++++++++++++++++++++++++++++++------
> > include/drm/drm_writeback.h | 7 ++
> > 2 files changed, 201 insertions(+), 30 deletions(-)
> >
> >
> > ---
> > base-commit: a6bb1f77a94335de67dba12e7f52651c115b82d2
> > change-id: 20240829-writeback-drmm-b9b85dcdaf7b
> >
> > Best regards,
> >
> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > index a031c335bdb9..a161deeea908 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -18,6 +18,7 @@
> > #include <drm/drm_modeset_helper_vtables.h>
> > #include <drm/drm_property.h>
> > #include <drm/drm_writeback.h>
> > +#include <drm/drm_managed.h>
>
> Includes are sort alphabetically, you might want to move this one up.
>
> >
> > /**
> > * DOC: overview
> > @@ -202,13 +203,12 @@ int drm_writeback_connector_init(struct drm_device *dev,
> > EXPORT_SYMBOL(drm_writeback_connector_init);
> >
> > /**
> > - * drm_writeback_connector_init_with_encoder - Initialize a writeback connector with
> > - * a custom encoder
> > + * __drm_writeback_connector_init - Common initialization code for writeback
> > + * connector
> > *
> > * @dev: DRM device
> > * @wb_connector: Writeback connector to initialize
> > * @enc: handle to the already initialized drm encoder
> > - * @con_funcs: Connector funcs vtable
> > * @formats: Array of supported pixel formats for the writeback engine
> > * @n_formats: Length of the formats array
> > *
> > @@ -224,41 +224,31 @@ EXPORT_SYMBOL(drm_writeback_connector_init);
> > * assigning the encoder helper functions, possible_crtcs and any other encoder
> > * specific operation.
> > *
> > - * Drivers should always use this function instead of drm_connector_init() to
> > - * set up writeback connectors if they want to manage themselves the lifetime of the
> > - * associated encoder.
> > - *
> > * Returns: 0 on success, or a negative error code
> > */
> > -int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> > - struct drm_writeback_connector *wb_connector, struct drm_encoder *enc,
> > - const struct drm_connector_funcs *con_funcs, const u32 *formats,
> > - int n_formats)
> > +static int __drm_writeback_connector_init(
> > + struct drm_device *dev, struct drm_writeback_connector *wb_connector,
> > + struct drm_encoder *enc, const u32 *formats, int n_formats)
>
> Not a big deal, but ./scripts/checkpatch.pl complains about this
> ideantation:
>
> CHECK: Lines should not end with a '('
> CHECK: Alignment should match open parenthesis
>
> You can fix it with:
>
> static int __drm_writeback_connector_init(struct drm_device *dev,
> struct drm_writeback_connector *wb_connector,
> struct drm_encoder *enc,
> const u32 *formats,
> int n_formats)
>
> > {
> > - struct drm_property_blob *blob;
> > struct drm_connector *connector = &wb_connector->base;
> > struct drm_mode_config *config = &dev->mode_config;
> > - int ret = create_writeback_properties(dev);
> > -
> > - if (ret != 0)
> > - return ret;
> > -
> > - blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
> > - formats);
> > - if (IS_ERR(blob))
> > - return PTR_ERR(blob);
> > -
> > + struct drm_property_blob *blob;
> > + int ret;
> >
> > connector->interlace_allowed = 0;
> >
> > - ret = drm_connector_init(dev, connector, con_funcs,
> > - DRM_MODE_CONNECTOR_WRITEBACK);
> > + ret = create_writeback_properties(dev);
> > if (ret)
> > - goto connector_fail;
> > + return ret;
> >
> > ret = drm_connector_attach_encoder(connector, enc);
> > if (ret)
> > - goto attach_fail;
> > + return ret;
> > +
> > + blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
> > + formats);
> > + if (IS_ERR(blob))
> > + return PTR_ERR(blob);
> >
> > INIT_LIST_HEAD(&wb_connector->job_queue);
> > spin_lock_init(&wb_connector->job_lock);
> > @@ -281,15 +271,189 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> > wb_connector->pixel_formats_blob_ptr = blob;
> >
> > return 0;
> > +}
> > +
> > +/**
> > + * drm_writeback_connector_init_with_encoder - Initialize a writeback connector with
> > + * a custom encoder
> > + *
> > + * @dev: DRM device
> > + * @wb_connector: Writeback connector to initialize
> > + * @enc: handle to the already initialized drm encoder
> > + * @con_funcs: Connector funcs vtable
> > + * @formats: Array of supported pixel formats for the writeback engine
> > + * @n_formats: Length of the formats array
> > + *
> > + * This function creates the writeback-connector-specific properties if they
> > + * have not been already created, initializes the connector as
> > + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
> > + * values.
> > + *
> > + * This function assumes that the drm_writeback_connector's encoder has already been
> > + * created and initialized before invoking this function.
> > + *
> > + * In addition, this function also assumes that callers of this API will manage
> > + * assigning the encoder helper functions, possible_crtcs and any other encoder
> > + * specific operation.
> > + *
> > + * Drivers should always use this function instead of drm_connector_init() to
> > + * set up writeback connectors if they want to manage themselves the lifetime of the
> > + * associated encoder.
> > + *
> > + * Returns: 0 on success, or a negative error code
> > + */
> > +int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> > + struct drm_writeback_connector *wb_connector, struct drm_encoder *enc,
> > + const struct drm_connector_funcs *con_funcs, const u32 *formats,
> > + int n_formats)
> > +{
> > + struct drm_connector *connector = &wb_connector->base;
> > + int ret;
> > +
> > + ret = drm_connector_init(dev, connector, con_funcs,
> > + DRM_MODE_CONNECTOR_WRITEBACK);
> > + if (ret)
> > + return ret;
> > +
> > + ret = __drm_writeback_connector_init(dev, wb_connector, enc, formats,
> > + n_formats);
> > + if (ret)
> > + drm_connector_cleanup(connector);
> >
> > -attach_fail:
> > - drm_connector_cleanup(connector);
> > -connector_fail:
> > - drm_property_blob_put(blob);
> > return ret;
> > }
> > EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
> >
> > +/**
> > + * drm_writeback_connector_cleanup - Cleanup the writeback connector
> > + * @dev: DRM device
> > + * @data: Opaque pointer to the writeback connector
> > + *
> > + * This will decrement the reference counter of blobs and it will clean the
> > + * remaining jobs in this writeback connector.
> > + */
> > +static void drm_writeback_connector_cleanup(struct drm_device *dev, void *data)
> > +{
> > + struct drm_writeback_connector *wb_connector = data;
> > + unsigned long flags;
> > + struct drm_writeback_job *pos, *n;
> > +
> > + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
> > +
> > + spin_lock_irqsave(&wb_connector->job_lock, flags);
> > + list_for_each_entry_safe(pos, n, &wb_connector->job_queue, list_entry) {
> > + drm_writeback_cleanup_job(pos);
> > + list_del(&pos->list_entry);
> > + }
> > + spin_unlock_irqrestore(&wb_connector->job_lock, flags);
> > +}
> > +
> > +/**
> > + * __drmm_writeback_connector_init - Initialize a writeback connector with
> > + * a custom encoder
> > + *
> > + * @dev: DRM device
> > + * @wb_connector: Writeback connector to initialize
> > + * @con_funcs: Connector funcs vtable
> > + * @enc: handle to the already initialized drm encoder
> > + * @formats: Array of supported pixel formats for the writeback engine
> > + * @n_formats: Length of the formats array
> > + *
> > + * This function initialize a writeback connector and register its cleanup.
> > + * It uses the common helper @__drm_writeback_connector_init to do the
> > + * general initialization.
> > + *
> > + * This function assumes that @enc has already been created and initialized
> > + * before invoking this function.
> > + *
> > + * In addition, this function also assumes that callers of this API will manage
> > + * assigning the encoder helper functions, possible_crtcs and any other encoder
> > + * specific operation.
> > + *
> > + * Returns: 0 on success, or a negative error code
> > + */
> > +static int __drmm_writeback_connector_init(
> > + struct drm_device *dev, struct drm_writeback_connector *wb_connector,
> > + const struct drm_connector_funcs *con_funcs, struct drm_encoder *enc,
> > + const u32 *formats, int n_formats)
>
> checkpatch warnings here as well.
>
> > +{
> > + struct drm_connector *connector = &wb_connector->base;
> > + int ret;
> > +
> > + ret = drmm_connector_init(dev, connector, con_funcs,
> > + DRM_MODE_CONNECTOR_WRITEBACK, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + ret = __drm_writeback_connector_init(dev, wb_connector, enc, formats,
> > + n_formats);
> > + if (ret) {
> > + drm_writeback_connector_cleanup(dev, &wb_connector);
>
> Is it safe to call drm_writeback_connector_cleanup() without initializing
> the job_queue and job_lock?
Right, I don't need to cleanup anything here.
> > + return ret;
> > + }
> > +
> > + ret = drmm_add_action_or_reset(dev, &drm_writeback_connector_cleanup,
> > + wb_connector);
> > + if (ret)
>
> Missing call to drm_writeback_connector_cleanup()?
I don't need to call it, the version _or_reset already does it [1,2].
[1]:https://dri.freedesktop.org/docs/drm/gpu/drm-internals.html#c.drmm_add_action_or_reset
[2]:https://elixir.bootlin.com/linux/v6.11/source/drivers/gpu/drm/drm_managed.c#L165
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * drmm_writeback_connector_init - Initialize a writeback connector with
> > + * a custom encoder
> > + *
> > + * @dev: DRM device
> > + * @wb_connector: Writeback connector to initialize
> > + * @con_funcs: Connector funcs vtable
> > + * @enc: handle to the already initialized drm encoder, optional
> > + * @enc_funcs: Encoder funcs vtable, optional
>
> I think that this is only used if @enc is NULL, it'd be nice to clarify
> it like you did with @possible_crtcs.
Yes, definitly. On my first implementation it was always used and I forgot
to update doc.
@enc_funcs: Encoder funcs vtable, optional, only used when @enc is
NULL and a new encoder is created
> > + * @formats: Array of supported pixel formats for the writeback engine
> > + * @n_formats: Length of the formats array
> > + * @possible_crtcs: if @enc is NULL, this will set the possible_crtc for the
> > + * newly created encoder
> > + *
> > + * This function initialize a writeback connector and register its cleanup.
> > + *
> > + * This function creates the writeback-connector-specific properties if they
> > + * have not been already created, initializes the connector as
> > + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
> > + * values.
> > + *
> > + * If @enc is NULL, a drm-managed encoder is created and used.
> > + * If @enc_funcs is not NULL, this vtable is attached to @enc or this new
> > + * encoder.
>
> Isn't it only attached when @enc is NULL?
Yes!
> > + *
> > + * Returns: 0 on success, or a negative error code
> > + */
> > +int drmm_writeback_connector_init(
> > + struct drm_device *dev, struct drm_writeback_connector *wb_connector,
> > + const struct drm_connector_funcs *con_funcs,
> > + struct drm_encoder *enc,
> > + const struct drm_encoder_helper_funcs *enc_funcs, const u32 *formats,
> > + int n_formats, u32 possible_crtcs)
>
> Same checkpatch issues.
>
> > +{
> > + int ret;
> > +
> > + if (!enc) {
> > + ret = drmm_encoder_init(dev, &wb_connector->encoder,
> > + NULL, DRM_MODE_ENCODER_VIRTUAL, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + enc = &wb_connector->encoder;
>
> This modifies an input function parameter, not sure if it's intended
> but undocumented.
It does not change the input function parameter, it changes the pointer
value, not the pointee value. If I want to change the input functino
parameter, I would have to use
*enc = smthing;
> > + enc->possible_crtcs |= possible_crtcs;
> > + if (enc_funcs)
> > + drm_encoder_helper_add(enc, enc_funcs);
> > + }
> > +
> > + return __drmm_writeback_connector_init(dev, wb_connector, con_funcs,
> > + &wb_connector->encoder, formats,
> > + n_formats);
> > +}
> > +EXPORT_SYMBOL(drmm_writeback_connector_init);
> > +
> > 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..88abfd3d4564 100644
> > --- a/include/drm/drm_writeback.h
> > +++ b/include/drm/drm_writeback.h
> > @@ -161,6 +161,13 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> > const struct drm_connector_funcs *con_funcs, const u32 *formats,
> > int n_formats);
> >
> > +int drmm_writeback_connector_init(
> > + struct drm_device *dev, struct drm_writeback_connector *wb_connector,
> > + const struct drm_connector_funcs *con_funcs,
> > + struct drm_encoder *enc,
> > + const struct drm_encoder_helper_funcs *enc_funcs, const u32 *formats,
> > + int n_formats, u32 possible_crtcs);
> > +
>
> This indentation make checkpatch happy:
>
> int drmm_writeback_connector_init(struct drm_device *dev,
> struct drm_writeback_connector *wb_connector,
> const struct drm_connector_funcs *con_funcs,
> struct drm_encoder *enc,
> const struct drm_encoder_helper_funcs *enc_funcs,
> const u32 *formats,
> int n_formats,
> u32 possible_crtcs);
>
> > int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> > struct drm_framebuffer *fb);
> >
> >
