Re: [Freedreno] [PATCH v6 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API
Hi Liviu
Thank you for your reviews. They were very useful to get this in shape.
Some comments below.
Abhinav
On 4/6/2022 7:55 AM, Liviu Dudau wrote:
Hi Abhinav,
On Thu, Mar 31, 2022 at 05:12:11PM -0700, Abhinav Kumar wrote:
For vendors drivers which pass an already allocated and
initialized encoder especially for cases where the encoder
hardware is shared OR the writeback encoder shares the resources
with the rest of the display pipeline introduce a new API,
drm_writeback_connector_init_with_encoder() which expects
an initialized encoder as a parameter and only sets up the
writeback connector.
changes in v6:
- remove drm_writeback_connector_setup() and instead
directly call drm_writeback_connector_init_with_encoder()
- fix a drm_writeback_connector typo and function doc which
incorrectly shows that the function accepts enc_helper_funcs
- pass encoder as a parameter explicitly to the new API
for better readability
Side comment: if you plan to have the log of changes in the commit message then
I
would keep a list of all the changes. Otherwise, putting the log after the --
line
below would still convey the information but will also ensure that when merged
it
will not show up in the commit message.
For DRM, I believe the convention was to have it above the --- ?
But yes, I will certainly include the full change log across the
revisions. Still waiting for the reviews on the next 2 patches to reach
a conclusion and when I post it again I will add it.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_writeback.c | 72 +
include/drm/drm_writeback.h | 6
2 files changed, 64 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dc2ef12..797223c 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -177,6 +177,62 @@ int drm_writeback_connector_init(struct drm_device *dev,
const struct drm_encoder_helper_funcs
*enc_helper_funcs,
const u32 *formats, int n_formats, uint32_t
possible_crtcs)
{
+ int ret = 0;
+
+ drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+
+ wb_connector->encoder.possible_crtcs = possible_crtcs;
+
+ ret = drm_encoder_init(dev, _connector->encoder,
+ _writeback_encoder_funcs,
+ DRM_MODE_ENCODER_VIRTUAL, NULL);
+ if (ret)
+ return ret;
+
+ ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
_connector->encoder,
+ con_funcs, formats, n_formats);
+
+ if (ret)
+ drm_encoder_cleanup(_connector->encoder);
+
+ return ret;
+}
+EXPORT_SYMBOL(drm_writeback_connector_init);
+
+/**
+ * drm_writeback_connector_init_with_encoder - Initialize a writeback
connector and its properties
+ * using the encoder which already assigned and initialized
+ *
+ * @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 which is otherwise handled by
drm_writeback_connector_init().
I would stop after "... specific operation".
Alright, will do. Thanks.
Otherwise, looks good to me.
Reviewed-by: Liviu Dudau
Best regards,
Liviu
+ *
+ * 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_property_blob *blob;
struct drm_connector *connector = _connector->base;
struct drm_mode_config *config = >mode_config;
@@ -190,15 +246,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
if (IS_ERR(blob))
return PTR_ERR(blob);
-
Re: [Freedreno] [PATCH v6 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API
Hi Abhinav,
On Thu, Mar 31, 2022 at 05:12:11PM -0700, Abhinav Kumar wrote:
> For vendors drivers which pass an already allocated and
> initialized encoder especially for cases where the encoder
> hardware is shared OR the writeback encoder shares the resources
> with the rest of the display pipeline introduce a new API,
> drm_writeback_connector_init_with_encoder() which expects
> an initialized encoder as a parameter and only sets up the
> writeback connector.
>
> changes in v6:
> - remove drm_writeback_connector_setup() and instead
> directly call drm_writeback_connector_init_with_encoder()
> - fix a drm_writeback_connector typo and function doc which
> incorrectly shows that the function accepts enc_helper_funcs
> - pass encoder as a parameter explicitly to the new API
> for better readability
Side comment: if you plan to have the log of changes in the commit message then
I
would keep a list of all the changes. Otherwise, putting the log after the --
line
below would still convey the information but will also ensure that when merged
it
will not show up in the commit message.
>
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/drm_writeback.c | 72
> +
> include/drm/drm_writeback.h | 6
> 2 files changed, 64 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dc2ef12..797223c 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -177,6 +177,62 @@ int drm_writeback_connector_init(struct drm_device *dev,
>const struct drm_encoder_helper_funcs
> *enc_helper_funcs,
>const u32 *formats, int n_formats, uint32_t
> possible_crtcs)
> {
> + int ret = 0;
> +
> + drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> +
> + wb_connector->encoder.possible_crtcs = possible_crtcs;
> +
> + ret = drm_encoder_init(dev, _connector->encoder,
> +_writeback_encoder_funcs,
> +DRM_MODE_ENCODER_VIRTUAL, NULL);
> + if (ret)
> + return ret;
> +
> + ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> _connector->encoder,
> + con_funcs, formats, n_formats);
> +
> + if (ret)
> + drm_encoder_cleanup(_connector->encoder);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_writeback_connector_init);
> +
> +/**
> + * drm_writeback_connector_init_with_encoder - Initialize a writeback
> connector and its properties
> + * using the encoder which already assigned and initialized
> + *
> + * @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 which is otherwise handled by
> drm_writeback_connector_init().
I would stop after "... specific operation".
Otherwise, looks good to me.
Reviewed-by: Liviu Dudau
Best regards,
Liviu
> + *
> + * 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_property_blob *blob;
> struct drm_connector *connector = _connector->base;
> struct drm_mode_config *config = >mode_config;
> @@ -190,15 +246,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
> if (IS_ERR(blob))
> return PTR_ERR(blob);
>
> - drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> -
> - wb_connector->encoder.possible_crtcs = possible_crtcs;
> -
> - ret = drm_encoder_init(dev, _connector->encoder,
> -_writeback_encoder_funcs,
> -DRM_MODE_ENCODER_VIRTUAL, NULL);
[Freedreno] [PATCH v6 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API
For vendors drivers which pass an already allocated and
initialized encoder especially for cases where the encoder
hardware is shared OR the writeback encoder shares the resources
with the rest of the display pipeline introduce a new API,
drm_writeback_connector_init_with_encoder() which expects
an initialized encoder as a parameter and only sets up the
writeback connector.
changes in v6:
- remove drm_writeback_connector_setup() and instead
directly call drm_writeback_connector_init_with_encoder()
- fix a drm_writeback_connector typo and function doc which
incorrectly shows that the function accepts enc_helper_funcs
- pass encoder as a parameter explicitly to the new API
for better readability
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_writeback.c | 72 +
include/drm/drm_writeback.h | 6
2 files changed, 64 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dc2ef12..797223c 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -177,6 +177,62 @@ int drm_writeback_connector_init(struct drm_device *dev,
const struct drm_encoder_helper_funcs
*enc_helper_funcs,
const u32 *formats, int n_formats, uint32_t
possible_crtcs)
{
+ int ret = 0;
+
+ drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+
+ wb_connector->encoder.possible_crtcs = possible_crtcs;
+
+ ret = drm_encoder_init(dev, _connector->encoder,
+ _writeback_encoder_funcs,
+ DRM_MODE_ENCODER_VIRTUAL, NULL);
+ if (ret)
+ return ret;
+
+ ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
_connector->encoder,
+ con_funcs, formats, n_formats);
+
+ if (ret)
+ drm_encoder_cleanup(_connector->encoder);
+
+ return ret;
+}
+EXPORT_SYMBOL(drm_writeback_connector_init);
+
+/**
+ * drm_writeback_connector_init_with_encoder - Initialize a writeback
connector and its properties
+ * using the encoder which already assigned and initialized
+ *
+ * @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 which is otherwise handled by
drm_writeback_connector_init().
+ *
+ * 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_property_blob *blob;
struct drm_connector *connector = _connector->base;
struct drm_mode_config *config = >mode_config;
@@ -190,15 +246,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
if (IS_ERR(blob))
return PTR_ERR(blob);
- drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
-
- wb_connector->encoder.possible_crtcs = possible_crtcs;
-
- ret = drm_encoder_init(dev, _connector->encoder,
- _writeback_encoder_funcs,
- DRM_MODE_ENCODER_VIRTUAL, NULL);
- if (ret)
- goto fail;
connector->interlace_allowed = 0;
@@ -207,8 +254,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
if (ret)
goto connector_fail;
- ret = drm_connector_attach_encoder(connector,
- _connector->encoder);
+ ret = drm_connector_attach_encoder(connector, enc);
if (ret)
goto attach_fail;
@@ -237,12 +283,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
attach_fail:
drm_connector_cleanup(connector);
connector_fail:
-
