[PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector
Abhinav Kumar
quic_abhinavk at quicinc.com
Fri Apr 1 16:47:19 UTC 2022
Hi Laurent
Thanks for the review.
One question below.
On 4/1/2022 8:38 AM, Laurent Pinchart wrote:
> Hi Abhinav,
>
> Thank you for the patch.
>
> On Thu, Mar 31, 2022 at 05:12:13PM -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 v6:
>> - assign the encoder inside
>> drm_writeback_connector_init_with_encoder() for
>> better readability
>> - improve some documentation for internal encoder
>>
>> Co-developed-by: Kandpal Suraj <suraj.kandpal at intel.com>
>> Signed-off-by: Kandpal Suraj <suraj.kandpal at intel.com>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>> ---
>> drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
>> drivers/gpu/drm/vc4/vc4_txp.c | 14 ++++++++------
>> include/drm/drm_writeback.h | 21 +++++++++++++++++++--
>
> Please split this in two patches, one for the DRM core and one for the
> VC4 driver. This applies to most patches as a general rule, with the
> main exception being API refactoring that requires changing the
> implementation and all its users in a single patch.
I also wanted to do that but that would break compilation of this change
because of vc4 (explained below) so I had to club it in this patch.
If you can provide some suggestion on how to address vc4's dependency I
can break this up.
>
>> 3 files changed, 39 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>> index 797223c..7f72109 100644
>> --- a/drivers/gpu/drm/drm_writeback.c
>> +++ b/drivers/gpu/drm/drm_writeback.c
>> @@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
>> {
>> int ret = 0;
>>
>> - drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
>> + drm_encoder_helper_add(&wb_connector->internal_encoder, enc_helper_funcs);
>>
>> - wb_connector->encoder.possible_crtcs = possible_crtcs;
>> + wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
>>
>> - ret = drm_encoder_init(dev, &wb_connector->encoder,
>> + ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
>> &drm_writeback_encoder_funcs,
>> DRM_MODE_ENCODER_VIRTUAL, NULL);
>> if (ret)
>> return ret;
>>
>> - ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, &wb_connector->encoder,
>> - con_funcs, formats, n_formats);
>> + ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
>> + &wb_connector->internal_encoder, con_funcs, formats, n_formats);
>>
>> if (ret)
>> - drm_encoder_cleanup(&wb_connector->encoder);
>> + drm_encoder_cleanup(&wb_connector->internal_encoder);
>>
>> return ret;
>> }
>> @@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
>> struct drm_mode_config *config = &dev->mode_config;
>> int ret = create_writeback_properties(dev);
>>
>> + /*
>> + * Assign the encoder passed to this API to the wb_connector's encoder.
>> + * For drm_writeback_connector_init(), this shall be the internal_encoder
>> + */
>> + wb_connector->encoder = enc;
>> +
>> if (ret != 0)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
>> index 5e53f02..a9b4f83 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);
vc4 directly references connector.encoder here.
>> + return container_of(encoder, struct vc4_txp, drm_enc);
>> }
>>
>> static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
>> @@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>>
>> wb_conn = &txp->connector;
>>
>> - drm_encoder_helper_add(&wb_conn->encoder, &vc4_txp_encoder_helper_funcs);
>> + drm_encoder_helper_add(&txp->drm_enc, &vc4_txp_encoder_helper_funcs);
>>
>> - ret = drm_encoder_init(drm, &wb_conn->encoder,
>> + ret = drm_encoder_init(drm, &txp->drm_enc,
>> &vc4_txp_encoder_funcs,
>> DRM_MODE_ENCODER_VIRTUAL, NULL);
>>
>> @@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>> drm_connector_helper_add(&wb_conn->base,
>> &vc4_txp_connector_helper_funcs);
>>
>> - ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &wb_conn->encoder,
>> + ret = drm_writeback_connector_init_with_encoder(drm, wb_conn, &txp->drm_enc,
>> &vc4_txp_connector_funcs, drm_fmts, ARRAY_SIZE(drm_fmts));
>> if (ret) {
>> - drm_encoder_cleanup(&wb_conn->encoder);
>> + drm_encoder_cleanup(&txp->drm_enc);
>> return ret;
>> }
>>
>> @@ -523,7 +525,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
>> if (ret)
>> return ret;
>>
>> - encoder = &txp->connector.encoder;
Also here.
>> + 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 4795024..3f5c330 100644
>> --- a/include/drm/drm_writeback.h
>> +++ b/include/drm/drm_writeback.h
>> @@ -25,15 +25,32 @@ 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
>> + * drm_writeback_connector_init() is used.
>> + * @encoder will be assigned to this for those cases
>> + *
>> + * This will be unused when drm_writeback_connector_init_with_encoder()
>> + * is used.
>> + */
>> + struct drm_encoder internal_encoder;
>> + /**
>> * @pixel_formats_blob_ptr:
>> *
>> * DRM blob property data for the pixel formats list on writeback
>> --
>> 2.7.4
>>
>
More information about the dri-devel
mailing list