[PATCH v6 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API
Abhinav Kumar
quic_abhinavk at quicinc.com
Wed Apr 6 17:14:04 UTC 2022
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 <quic_abhinavk at quicinc.com>
>> ---
>> 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(&wb_connector->encoder, enc_helper_funcs);
>> +
>> + wb_connector->encoder.possible_crtcs = possible_crtcs;
>> +
>> + ret = drm_encoder_init(dev, &wb_connector->encoder,
>> + &drm_writeback_encoder_funcs,
>> + DRM_MODE_ENCODER_VIRTUAL, NULL);
>> + if (ret)
>> + return ret;
>> +
>> + ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
>> + con_funcs, formats, n_formats);
>> +
>> + if (ret)
>> + drm_encoder_cleanup(&wb_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 <liviu.dudau at arm.com>
>
> 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 = &wb_connector->base;
>> struct drm_mode_config *config = &dev->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(&wb_connector->encoder, enc_helper_funcs);
>> -
>> - wb_connector->encoder.possible_crtcs = possible_crtcs;
>> -
>> - ret = drm_encoder_init(dev, &wb_connector->encoder,
>> - &drm_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,
>> - &wb_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:
>> - drm_encoder_cleanup(&wb_connector->encoder);
>> -fail:
>> drm_property_blob_put(blob);
>> return ret;
>> }
>> -EXPORT_SYMBOL(drm_writeback_connector_init);
>> +EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
>>
>> 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 db6214f..4795024 100644
>> --- a/include/drm/drm_writeback.h
>> +++ b/include/drm/drm_writeback.h
>> @@ -152,6 +152,12 @@ 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 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);
>> +
>> int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>> struct drm_framebuffer *fb);
>>
>> --
>> 2.7.4
>>
>
More information about the dri-devel
mailing list