Re: [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
> > > > 
> > > > 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.


> 
> 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.

> 
> 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
> > > > > 
> > > > 
> > 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux