[Freedreno] [PATCH v4 2/4] drm: allow real encoder to be passed for drm_writeback_connector

[Liviu Dudau]

On Thu, Mar 17, 2022 at 06:45:34PM -0700, Abhinav Kumar wrote:
> For some vendor driver implementations, display hardware can
> be shared between the encoder used for writeback and the physical
> display.
> 
> In addition resources such as clocks and interrupts can
> also be shared between writeback and the real encoder.
> 
> To accommodate such vendor drivers and hardware, allow
> real encoder to be passed for drm_writeback_connector.
> 
> changes in v4:
> 	- split the possible_crtcs change and the parts which should
> 	  belong to the addition of new API to the next change
> 
> Co-developed-by: Kandpal Suraj <suraj.kandpal at intel.com>
> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
> ---
>  drivers/gpu/drm/drm_writeback.c | 12 +++++++-----
>  drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++++++++++----
>  include/drm/drm_writeback.h     | 18 ++++++++++++++++--
>  3 files changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dc2ef12..a4c17d6 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -190,11 +190,13 @@ 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);
> +	drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
>  
> -	wb_connector->encoder.possible_crtcs = possible_crtcs;
> +	wb_connector->encoder = &wb_connector->internal_encoder;

You need to check here that the wb_connector doesn't have already an attached encoder
before you overwrite the pointer with the internal encoder.

>  
> -	ret = drm_encoder_init(dev, &wb_connector->encoder,
> +	wb_connector->encoder->possible_crtcs = possible_crtcs;
> +
> +	ret = drm_encoder_init(dev, wb_connector->encoder,
>  			       &drm_writeback_encoder_funcs,
>  			       DRM_MODE_ENCODER_VIRTUAL, NULL);

Here you have initialised the encoder pointed at by wb_connector->encoder, which is
always wb_connector->internal_encoder with your code.

>  	if (ret)
> @@ -208,7 +210,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  		goto connector_fail;
>  
>  	ret = drm_connector_attach_encoder(connector,
> -						&wb_connector->encoder);
> +						wb_connector->encoder);
>  	if (ret)
>  		goto attach_fail;
>  
> @@ -237,7 +239,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  attach_fail:
>  	drm_connector_cleanup(connector);
>  connector_fail:
> -	drm_encoder_cleanup(&wb_connector->encoder);
> +	drm_encoder_cleanup(wb_connector->encoder);
>  fail:
>  	drm_property_blob_put(blob);
>  	return ret;
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index 3447eb6..341a9be5 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -151,6 +151,8 @@ struct vc4_txp {
>  
>  	struct platform_device *pdev;
>  
> +	struct drm_encoder drm_enc;
> +
>  	struct drm_writeback_connector connector;
>  
>  	void __iomem *regs;
> @@ -159,7 +161,7 @@ struct vc4_txp {
>  
>  static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
>  {
> -	return container_of(encoder, struct vc4_txp, connector.encoder);
> +	return container_of(encoder, struct vc4_txp, drm_enc);
>  }
>  
>  static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
> @@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>  	struct vc4_txp *txp;
>  	struct drm_crtc *crtc;
>  	struct drm_encoder *encoder;
> +	struct drm_writeback_connector *wb_conn;
>  	int ret, irq;
>  
>  	irq = platform_get_irq(pdev, 0);
> @@ -492,9 +495,12 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>  	txp->regset.regs = txp_regs;
>  	txp->regset.nregs = ARRAY_SIZE(txp_regs);
>  
> -	drm_connector_helper_add(&txp->connector.base,
> +	wb_conn = &txp->connector;
> +	wb_conn->encoder = &txp->drm_enc;
> +
> +	drm_connector_helper_add(&wb_conn->base,
>  				 &vc4_txp_connector_helper_funcs);
> -	ret = drm_writeback_connector_init(drm, &txp->connector,
> +	ret = drm_writeback_connector_init(drm, wb_conn,
>  					   &vc4_txp_connector_funcs,
>  					   &vc4_txp_encoder_helper_funcs,
>  					   drm_fmts, ARRAY_SIZE(drm_fmts),

This call will never initialise the txp->drm_enc, as per my comments above. However
if this was the intent, it's fine, but then you need to add a drm_encoder_init() call
here for txp->drm_enc. Otherwise, you need to stop overwriting the pointer in
drm_writeback_connector_init().

> @@ -507,7 +513,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		return ret;
>  
> -	encoder = &txp->connector.encoder;
> +	encoder = txp->connector.encoder;
>  	encoder->possible_crtcs = drm_crtc_mask(crtc);
>  
>  	ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index db6214f..c525b60 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -25,15 +25,29 @@ struct drm_writeback_connector {
>  	struct drm_connector base;
>  
>  	/**
> -	 * @encoder: Internal encoder used by the connector to fulfill
> +	 * @encoder: handle to drm_encoder used by the connector to fulfill
>  	 * the DRM framework requirements. The users of the
>  	 * @drm_writeback_connector control the behaviour of the @encoder
>  	 * by passing the @enc_funcs parameter to drm_writeback_connector_init()
>  	 * function.
> +	 *
> +	 * For some vendor drivers, the hardware resources are shared between
> +	 * writeback encoder and rest of the display pipeline.
> +	 * To accommodate such cases, encoder is a handle to the real encoder
> +	 * hardware.
> +	 *
> +	 * For current existing writeback users, this shall continue to be the
> +	 * embedded encoder for the writeback connector.
>  	 */
> -	struct drm_encoder encoder;
> +	struct drm_encoder *encoder;
>  
>  	/**
> +	 * @internal_encoder: internal encoder used by writeback when
> +	 * a real encoder is not provided by the vendor drm drivers.
> +	 * @encoder will be assigned to this for those cases.
> +	 */
> +	struct drm_encoder internal_encoder;
> +	/**
>  	 * @pixel_formats_blob_ptr:
>  	 *
>  	 * DRM blob property data for the pixel formats list on writeback
> -- 
> 2.7.4
> 

You need to come up with a bit more sophisticated algorithm for allowing drivers to
pass on a connector with a pre-allocated encoder. If the encoder is pre-populated
then you need to decide if drm_writeback_connector_init() will initialise the encoder
too or if it's going to assume that it has been already initialised (in which case
the error path where we call drm_encoder_cleanup() also needs to know if we're using
the internal encoder or not and act accordingly).

Otherwise the code looks pretty good, thanks for the improvements.

Best regards,
Liviu



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