[PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API
Abhinav Kumar
quic_abhinavk at quicinc.com
Tue Mar 29 17:00:26 UTC 2022
Hi Liviu
Gentle reminder ... Can you please help to clarify the last set of
questions so that I can work on the next version?
Thanks
Abhinav
On 3/25/2022 9:31 AM, Abhinav Kumar wrote:
> 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