[PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API
Abhinav Kumar
quic_abhinavk at quicinc.com
Fri Mar 25 16:31:35 UTC 2022
Hi Liviu
On 3/25/2022 3:19 AM, Liviu Dudau wrote:
> On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:
>> Hi Liviu
>
> Hello,
>
>>
>> Thanks for the response.
>>
>> On 3/24/2022 3:12 AM, Liviu Dudau wrote:
>>> On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
>>>> Hi Liviu
>>>
>>> Hello,
>>>
>>>>
>>>> Thanks for the review.
>>>>
>>>> On 3/23/2022 9:46 AM, Liviu Dudau wrote:
>>>>> On Mon, Mar 21, 2022 at 04:56:43PM -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 v5:
>>>>>> - reorder this change to come before in the series
>>>>>> to avoid incorrect functionality in subsequent changes
>>>>>> - continue using struct drm_encoder instead of
>>>>>> struct drm_encoder * and switch it in next change
>>>>>>
>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>>>>
>>>>> Hi Abhinav,
>>>>>
>>>>>> ---
>>>>>> drivers/gpu/drm/drm_writeback.c | 143 ++++++++++++++++++++++++++++------------
>>>>>> include/drm/drm_writeback.h | 5 ++
>>>>>> 2 files changed, 106 insertions(+), 42 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>>>>>> index dc2ef12..abe78b9 100644
>>>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>>>> @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
>>>>>> .destroy = drm_encoder_cleanup,
>>>>>> };
>>>>>> -/**
>>>>>> - * drm_writeback_connector_init - Initialize a writeback connector and its properties
>>>>>> - * @dev: DRM device
>>>>>> - * @wb_connector: Writeback connector to initialize
>>>>>> - * @con_funcs: Connector funcs vtable
>>>>>> - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
>>>>>> - * @formats: Array of supported pixel formats for the writeback engine
>>>>>> - * @n_formats: Length of the formats array
>>>>>> - * @possible_crtcs: possible crtcs for the internal writeback encoder
>>>>>> - *
>>>>>> - * 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. It will also create an internal encoder associated with the
>>>>>> - * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
>>>>>> - * the encoder helper.
>>>>>> - *
>>>>>> - * Drivers should always use this function instead of drm_connector_init() to
>>>>>> - * set up writeback connectors.
>>>>>> - *
>>>>>> - * Returns: 0 on success, or a negative error code
>>>>>> - */
>>>>>> -int drm_writeback_connector_init(struct drm_device *dev,
>>>>>> - struct drm_writeback_connector *wb_connector,
>>>>>> - const struct drm_connector_funcs *con_funcs,
>>>>>> - const struct drm_encoder_helper_funcs *enc_helper_funcs,
>>>>>> - const u32 *formats, int n_formats, uint32_t possible_crtcs)
>>>>>> +static int drm_writeback_connector_setup(struct drm_device *dev,
>>>>>> + struct drm_writeback_connector *wb_connector,
>>>>>> + 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;
>>>>>> + struct drm_connector *connector = &wb_connector->base;
>>>>>> +
>>>>>
>>>>> Point of this reordering the statements is...?
>>>> This diff can be avoided. There was no reason to reorder this. I will remove
>>>> this re-order.
>>>>>
>>>>>> int ret = create_writeback_properties(dev);
>>>>>> if (ret != 0)
>>>>>> @@ -187,18 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
>>>>>> blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
>>>>>> formats);
>>>>>> - 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;
>>>>>> + if (IS_ERR(blob)) {
>>>>>> + ret = PTR_ERR(blob);
>>>>>> + return ret;
>>>>>> + }
>>>>>
>>>>> I don't see why you have changed the earlier code to store the result of PTR_ERR into
>>>>> ret other than to help your debugging. I suggest that you keep the existing code that
>>>>> returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.
>>>> Sure, i can fix this for a smaller diff stat.
>>>>>
>>>>>> connector->interlace_allowed = 0;
>>>>>> @@ -237,13 +207,102 @@ 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;
>>>>>> }
>>>>>> +
>>>>>> +/**
>>>>>> + * drm_writeback_connector_init - Initialize a writeback connector and its properties
>>>>>> + * @dev: DRM device
>>>>>> + * @wb_connector: Writeback connector to initialize
>>>>>> + * @con_funcs: Connector funcs vtable
>>>>>> + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
>>>>>> + * @formats: Array of supported pixel formats for the writeback engine
>>>>>> + * @n_formats: Length of the formats array
>>>>>> + * @possible_crtcs: possible crtcs for the internal writeback encoder
>>>>>> + *
>>>>>> + * 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. It will also create an internal encoder associated with the
>>>>>> + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
>>>>>> + * the encoder helper.
>>>>>> + *
>>>>>> + * Drivers should always use this function instead of drm_connector_init() to
>>>>>> + * set up writeback connectors.
>>>>>> + *
>>>>>> + * Returns: 0 on success, or a negative error code
>>>>>> + */
>>>>>> +int drm_writeback_connector_init(struct drm_device *dev,
>>>>>> + struct drm_writeback_connector *wb_connector,
>>>>>> + const struct drm_connector_funcs *con_funcs,
>>>>>> + 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_setup(dev, wb_connector, 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
>>>>>> + * @con_funcs: Connector funcs vtable
>>>>>> + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal encoder
>>>>>> + * @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_writebac_connector's encoder has already been
>>>>>
>>>>> spelling: writeback
>>>> Ack. will fix this.
>>>>>
>>>>>> + * 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.
>>>>>
>>>>> We're not trying to replace drm_writeback_connector_init() function here, only to
>>>>> provide an alternative function to call for special cases.
>>>>
>>>> Yes, we are trying to provide an alternative function call for special case
>>>> where the encoder is a shared encoder and/or where the resources of the
>>>> writeback encoder are shared with other hardware blocks.
>>>>
>>>> I can add that comment to this function's doc.
>>>>
>>>>>
>>>>>> + *
>>>>>> + * 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,
>>>>>> + const struct drm_connector_funcs *con_funcs, const u32 *formats,
>>>>>> + int n_formats)
>>>>>
>>>>> Where is the encoder parameter? Isn't that the reason why the function is called
>>>>> `_with_encoder`?
>>>> The encoder parameter is skipped here because this function assumes that
>>>> wb_connector->encoder has already been initialized, setup with functions and
>>>> its possible_crts have already been set prior to calling this function like
>>>> the vc4 example shows. So this function doesnt need an explicit encoder
>>>> parameter. Let me know if thats a concern.
>>>>>
>>>>> I think there might have been too many version of these patchsets and things are
>>>>> starting to be confusing. Please revisit the series without rushing and come up with
>>>>> a plan of action. My understanding of watching this series has been that you're
>>>>> trying to come up with a function that does *connector* initialisation but skips the
>>>>> *encoder* initialisation because it might have been already done by the driver. The
>>>>> idea will be then to have a function `drm_writeback_connector_init_with_encoder()`
>>>>> that does *all* the work that `drm_writeback_connector_init()` does at the moment minus
>>>>> the encoder initialisation part. Then `drm_writeback_connector_init()` only
>>>>> initialises the internal encoder and calls `drm_writeback_connector_init_with_encoder()`.
>>>>> There is no need to have the `drm_writeback_connector_setup()` function at all.
>>>>>
>>>>> Best regards,
>>>>> Liviu
>>>>>
>>>>
>>>> I agree there have been a 4 revisions prior to this because of the way this
>>>> affects the existing writeback drivers. So initial revision was a bit
>>>> intrusive into other drivers (which was just mostly a take over from the
>>>> previous patchset posted by the Co-developer ) and since rev3 we have
>>>> decided to have a separate API drm_writeback_connector_init_with_encoder()
>>>> so that existing clients are unaffected and it works seamlessly under the
>>>> hood.
>>>>
>>>> Only clients which already embed an encoder (vc4) and the new ones which
>>>> have special encoder requirements like the MSM driver for which I am
>>>> preparing these changes for will use the new API.
>>>>
>>>> Apologies for the revisions, but thanks to some great feedback from you and
>>>> laurent its shaping up nicely and reaching its conclusion I feel.
>>>
>>> I think it is only natural that there will be some iterations. If I remember
>>> correctly, the initial writeback series has something like 13 revisions :)
>>>
>>>>
>>>> So i think this revision is pretty close to being clean thanks to the
>>>> feedback you gave on rev4. Between rev4 and this one I didnt drastically
>>>> change the design other than re-ordering the changes to avoid the
>>>> intermediate patches having an incorrect state for the vc4 encoder. So all
>>>> your comments related to the encoder_cleanup() and vc4's encoder not getting
>>>> initialized would have gotten addressed but overall concept remained same.
>>>>
>>>> You are right, we are trying to come up with a function which does connector
>>>> initialization but skips the encoder part because that has already been done
>>>> in the client side of this API ( hence _with_encoder() name ). The
>>>> "_with_encoder" indicates that the caller already has an encoder for the
>>>> writeback connector which is being passed so there is no need to pass the
>>>> encoder again.
>>>>
>>>> I thought this addresses all the concerns posted in the previous series.
>>>>
>>>> So are you suggesting something like below?
>>>>
>>>> 1) rename drm_writeback_connector_setup() to
>>>> drm_writeback_connector_init_with_encoder()
>>>> (essentially thats what will end up happening )
>>>
>>> Correct. Without the pointless reordering of code it should be about 3 lines of code
>>> that get removed (the calls to drm_encoder_helper_add() and drm_encoder_init()).
>>>
>>
>> But isnt thats how it already looks.
>>
>> int drm_writeback_connector_init(struct drm_device *dev,
>> struct drm_writeback_connector *wb_connector,
>> const struct drm_connector_funcs *con_funcs,
>> const struct drm_encoder_helper_funcs *enc_helper_funcs,
>> const u32 *formats, int n_formats, uint32_t possible_crtcs)
>> {
>> int ret = 0;
>>
>> wb_connector->encoder = &wb_connector->internal_encoder;
>>
>> 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_setup(dev, wb_connector, con_funcs,
>> formats,
>> n_formats);
>>
>> So the only change you are requesting is that, instead of having a new
>> drm_writeback_connector_setup(), just call
>> drm_writeback_connector_init_with_encoder().
>>
>> It will essentially look like
>>
>> int drm_writeback_connector_init(struct drm_device *dev,
>> struct drm_writeback_connector *wb_connector,
>> const struct drm_connector_funcs *con_funcs,
>> const struct drm_encoder_helper_funcs *enc_helper_funcs,
>> const u32 *formats, int n_formats, uint32_t possible_crtcs)
>> {
>> int ret = 0;
>>
>> wb_connector->encoder = &wb_connector->internal_encoder;
>
> (1)
>
>>
>> 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,
>> con_funcs, formats,
>> n_formats);
>
> Yes, this is exactly what I had in mind.
Thank you for confirming.
>
>
>>
>> drm_writeback_connector_init_with_encoder() will still not have an encoder
>> parameter because of what I wrote previously.
>
> But in your patch drm_writeback_connector_init_with_encoder() still has an
> encoder_funcs pointer which is useless, as the expectations are (AFAII) that the
> whole encoder init dance has already happened. And while you have a point that you
> can set the encoder pointer in the connector before calling
> drm_writeback_connector_init_with_encoder() I think it would be easier to read if you
> pass the encoder explicitly in the parameter list and skip the assignment in (1) and
> do it inside drm_writeback_connector_init_with_encoder(). Again, your code is not
> wrong, I just think we should be explicit so that code is easier to read.
Sorry, but I am missing something here.
Where is the encoder_funcs pointer in
drm_writeback_connector_init_with_encoder()? It only has
drm_connector_funcs pointer. Because, like I mentioned in the earlier
comment, the callers of this API would have already setup the encoder
with the required functions/possible_crtcs etc.
int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
struct drm_writeback_connector *wb_connector,
const struct drm_connector_funcs *con_funcs, const u32 *formats,
int n_formats)
{
int ret = 0;
ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs,
formats,
n_formats);
return ret;
}
So are you suggesting I pass the drm_encoder as a parameter to
drm_writeback_connector_init_with_encoder as well?
That has two potential issues:
1) If we move just the assignment of wb_connector->encoder to inside
drm_writeback_connector_init_with_encoder, we cannot do these before
calling drm_writeback_connector_init_with_encoder() as
wb_connector->encoder will be NULL for the cases which use internal_encoder
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);
2) We can only do these changes once wb_connector->encoder has been
changed to a pointer which happens in patch 4 of this series
https://patchwork.freedesktop.org/patch/479084/?series=101260&rev=5
I was told to split the functionality of adding the new API in a
separate patch by laurent.
To keep changes independent and functionally correct I thought this is
the best split.
So the cleanup you are suggesting can be done in patch 4 of this series
but still I am not sure how to get around concern (1) because the
encoder has to be fully initialized before attaching to a wb_connector.
>
>>
>> Hope this is what you had in mind as well.
>>
>>>>
>>>> 2) Inside drm_writeback_connector_init() check:
>>>>
>>>> drm_writeback_connector_init(.....)
>>>> {
>>>> if (!wb_conn->encoder)
>>>> initialize the encoder
>>>
>>> No, the assumption of drm_writeback_connector_init() is that we will provide the
>>> encoder, so no need to check that one is already provided. What you could do is:
>>>
>>> WARN_ON(wb_conn->encoder);
>>>
>>
>> Got it, i will add a warning inside drm_writeback_connector_init() to
>> emphasize that its only meant for cases where an encoder is not provided.
>>
>>> before we overwrite the encoder. That way we will get a nice warning in the kernel
>>> log if someone tries to call drm_writeback_connector_init() with a preset encoder.
>>>
>>>>
>>>> call drm_writeback_**_init_with_encoder()
>>>> }
>>>>
>>>> This will also work but have the foll concerns/questions:
>>>>
>>>> 1) Will drm_writeback_connector_init_with_encoder() be exported if we change
>>>> as per your suggestion or will all clients continue to use
>>>> drm_writeback_connector_init() only? We wanted to have a separate function
>>>> for new clients.
>>>
>>> Yes, we will need to export drm_writeback_connector_init_with_encoder() as well.
>>>
>> Alright, so vc4 and new vendors which provide their own encoder will use
>> this one. So no changes to the rest of the series.
>>
>>>>
>>>> 2) How will we detect that encoder needs to be initialized without it being
>>>> a pointer which happens in the later change. So ordering becomes an issue.
>>>
>>> I think the init problem is simple. You either call drm_writeback_connector_init()
>>> and the framework provides you with an encoder, or you call
>>> drm_writeback_connector_init_with_encoder() and the framework will use yours. The
>>> problems will show up on the cleanup and exit codes, where we need to be able to skip
>>> the cleanup if the encoder pointer is not the internal one. I think a simple
>>>
>>> if (connector->encoder == &connector->internal_encoder)
>>> do_encoder_cleanup_work_here()
>>>
>>> should work.
>>
>> Sorry, I am missing something here.
>>
>> Even in this current patch, the drm_encoder_cleanup() is done only inside
>> drm_writeback_connector_init() where internal_encoder is used.
>>
>> For drm_writeback_connector_init_with_encoder(), we dont do the cleanup and
>> expect the caller to do it like vc4 does in the next patch.
>>
>> So why do we need such a condition?
>
> You're right. I was thinking that at cleanup time we also need to do work with the
> right encoder, but that should accomplished by passing the right .destroy hook in the
> drm_encoder_funcs pointer via drm_encoder_init. So if the special drivers to the
> initialisation correctly it should all work fine, please disregard my comments.
>
> Best regards,
> Liviu
>
>
>>
>>>
>>>>
>>>> Thats why I thought this is the best way to address the comments and keep
>>>> the functionality intact with each change.
>>>>
>>>> Let me know if I have misunderstood some comment or idea.
>>>
>>> I hope that with these clarifications we are both on the same page.
>>>
>>> Best regards,
>>> Liviu
>>>
>>>
>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>> +{
>>>>>> + int ret = 0;
>>>>>> +
>>>>>> + ret = drm_writeback_connector_setup(dev, wb_connector, con_funcs, formats,
>>>>>> + n_formats);
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +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..0093bab 100644
>>>>>> --- a/include/drm/drm_writeback.h
>>>>>> +++ b/include/drm/drm_writeback.h
>>>>>> @@ -152,6 +152,11 @@ 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,
>>>>>> + 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