[PATCH v4 2/4] drm: allow real encoder to be passed for drm_writeback_connector
Abhinav Kumar
quic_abhinavk at quicinc.com
Mon Mar 21 18:15:28 UTC 2022
Hi Livid
Thanks for your review.
All your comments are valid. I think I should re-order the patches like
you have suggested. That should address all comments.
Thanks
Abhinav
On 3/21/2022 10:24 AM, Liviu Dudau wrote:
> 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
>
>
>
More information about the dri-devel
mailing list