[Freedreno] [PATCH v3 1/3] drm: allow real encoder to be passed for drm_writeback_connector
Abhinav Kumar
quic_abhinavk at quicinc.com
Mon Mar 21 18:17:01 UTC 2022
Hi Liviu
On 3/21/2022 11:07 AM, Liviu Dudau wrote:
> On Thu, Mar 17, 2022 at 10:26:38AM -0700, Abhinav Kumar wrote:
>> Hi Laurent
>>
>> Thanks for the review.
>>
>> On 3/17/2022 1:51 AM, Laurent Pinchart wrote:
>>> Hi Abhinav,
>>>
>>> Thank you for the patch.
>>>
>>> On Wed, Mar 16, 2022 at 11:48:16AM -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 using a new
>>>> drm_writeback_connector_init_with_encoder() API.
>>>
>>> The commit message doesn't match the commit.
>> Sorry, while splitting the change , I missed this part of the commit text.
>> Will fix it up.
>>>
>>>> In addition, to preserve the same call flows for the existing
>>>> users of drm_writeback_connector_init(), also allow passing
>>>> possible_crtcs as a parameter so that encoder can be initialized
>>>> with it.
>>>>
>>>> changes in v3:
>>>> - allow passing possible_crtcs for existing users of
>>>> drm_writeback_connector_init()
>>>> - squash the vendor changes into the same commit so
>>>> that each patch in the series can compile individually
>>>>
>>>> Co-developed-by: Kandpal Suraj <suraj.kandpal at intel.com>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>>> ---
>>>> .../drm/arm/display/komeda/komeda_wb_connector.c | 3 +-
>>>> drivers/gpu/drm/arm/malidp_mw.c | 5 +-
>>>> drivers/gpu/drm/drm_writeback.c | 103 +++++++++++++--------
>>>> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 5 +-
>>>> drivers/gpu/drm/vc4/vc4_txp.c | 19 ++--
>>>> drivers/gpu/drm/vkms/vkms_writeback.c | 3 +-
>>>> include/drm/drm_writeback.h | 22 ++++-
>>>> 7 files changed, 103 insertions(+), 57 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
>>>> index e465cc4..40774e6 100644
>>>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
>>>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
>>>> @@ -155,7 +155,6 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
>>>> kwb_conn->wb_layer = kcrtc->master->wb_layer;
>>>> wb_conn = &kwb_conn->base;
>>>> - wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(&kcrtc->base));
>>>> formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
>>>> kwb_conn->wb_layer->layer_type,
>>>> @@ -164,7 +163,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
>>>> err = drm_writeback_connector_init(&kms->base, wb_conn,
>>>> &komeda_wb_connector_funcs,
>>>> &komeda_wb_encoder_helper_funcs,
>>>> - formats, n_formats);
>>>> + formats, n_formats, BIT(drm_crtc_index(&kcrtc->base)));
>>>> komeda_put_fourcc_list(formats);
>>>> if (err) {
>>>> kfree(kwb_conn);
>>>> diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
>>>> index f5847a7..b882066 100644
>>>> --- a/drivers/gpu/drm/arm/malidp_mw.c
>>>> +++ b/drivers/gpu/drm/arm/malidp_mw.c
>>>> @@ -208,11 +208,12 @@ int malidp_mw_connector_init(struct drm_device *drm)
>>>> struct malidp_drm *malidp = drm->dev_private;
>>>> u32 *formats;
>>>> int ret, n_formats;
>>>> + uint32_t possible_crtcs;
>>>> if (!malidp->dev->hw->enable_memwrite)
>>>> return 0;
>>>> - malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
>>>> + possible_crtcs = 1 << drm_crtc_index(&malidp->crtc);
>>>> drm_connector_helper_add(&malidp->mw_connector.base,
>>>> &malidp_mw_connector_helper_funcs);
>>>> @@ -223,7 +224,7 @@ int malidp_mw_connector_init(struct drm_device *drm)
>>>> ret = drm_writeback_connector_init(drm, &malidp->mw_connector,
>>>> &malidp_mw_connector_funcs,
>>>> &malidp_mw_encoder_helper_funcs,
>>>> - formats, n_formats);
>>>> + formats, n_formats, possible_crtcs);
>>>
>>> Do you need the local variable ?
>>
>> Yes, we can dtop this. I just used this instead of "1 <<
>> drm_crtc_index(&malidp->crtc)" to simplify it.
>> No strong preference.
>>
>>>
>>>> kfree(formats);
>>>> if (ret)
>>>> return ret;
>>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>>>> index dccf4504..17c1471 100644
>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>> @@ -149,36 +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
>>>> - *
>>>> - * 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)
>>>> +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;
>>>> +
>>>> int ret = create_writeback_properties(dev);
>>>> if (ret != 0)
>>>> @@ -186,15 +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);
>>>> - 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;
>>>> + }
>>>> connector->interlace_allowed = 0;
>>>> @@ -204,7 +178,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;
>>>> @@ -233,11 +207,62 @@ 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
>>>> + * using an internal encoder
>>>> + *
>>>> + * @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;
>>>> +
>>>> + 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);
>>>> +
>>>> + if (ret)
>>>> + drm_encoder_cleanup(wb_connector->encoder);
>>>> +
>>>> + return ret;
>>>> +}
>>>
>>> All this doesn't belong to this patch. Just add the possible_crtcs
>>> argument to drm_writeback_connector_init() in 1/3, nothing else. One
>>> functional change, one patch.
>>
>> Ok, so let me re-factor the series like this:
>>
>> 1) possible_crtcs argument along with other vendor changes
>> 2) changing encoder to a pointer with changes to vc4
>> 3) adding a new API drm_writeback_connector_init_with_encoder() with the
>> cleanup in the drm_writeback
>> 4) changing vc4 to use the new API
>
> I think the order should be 1, 3, 4, 2 (without the vc4 changes).
>
> Best regards,
> Liviu
>
You are right. I should re-order this way to address the very valid
comments posted on patch 2.
Thanks
Abhinav
>>
>> Let me know what you think.
>>
>>>
>>>> EXPORT_SYMBOL(drm_writeback_connector_init);
>>>> int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> index c79d125..1a610c0 100644
>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
>>>> @@ -199,8 +199,9 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>>>> struct rcar_du_crtc *rcrtc)
>>>> {
>>>> struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
>>>> + uint32_t possible_crtcs;
>>>> - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>>>> + possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc);
>>>> drm_connector_helper_add(&wb_conn->base,
>>>> &rcar_du_wb_conn_helper_funcs);
>>>> @@ -208,7 +209,7 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>>>> &rcar_du_wb_conn_funcs,
>>>> &rcar_du_wb_enc_helper_funcs,
>>>> writeback_formats,
>>>> - ARRAY_SIZE(writeback_formats));
>>>> + ARRAY_SIZE(writeback_formats), possible_crtcs);
>>>
>>> Same here, do you need the intermediate variable ?
>>
>>>
>>> And please add a line break after ARRAY_SIZE().
>> Ok will do
>>>
>>>> }
>>>> void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
>>>> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
>>>> index 9809ca3..32c4fb0 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,12 +495,14 @@ 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,
>>>> - &vc4_txp_connector_funcs,
>>>> - &vc4_txp_encoder_helper_funcs,
>>>> - drm_fmts, ARRAY_SIZE(drm_fmts));
>>>> + ret = drm_writeback_connector_init(drm, wb_conn,
>>>> + &vc4_txp_connector_funcs, &vc4_txp_encoder_helper_funcs,
>>>> + drm_fmts, ARRAY_SIZE(drm_fmts), 0);
>>>
>>> Most of this also doesn't belong to this patch.
>>>
>>>> if (ret)
>>>> return ret;
>>>> @@ -506,7 +511,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/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
>>>> index 8694227..381e7b7 100644
>>>> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
>>>> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
>>>> @@ -140,12 +140,11 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
>>>> {
>>>> struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
>>>> - vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
>>>> drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
>>>> return drm_writeback_connector_init(&vkmsdev->drm, wb,
>>>> &vkms_wb_connector_funcs,
>>>> &vkms_wb_encoder_helper_funcs,
>>>> vkms_wb_formats,
>>>> - ARRAY_SIZE(vkms_wb_formats));
>>>> + ARRAY_SIZE(vkms_wb_formats), 1);
>>>> }
>>>> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
>>>> index 9697d27..5e002bc 100644
>>>> --- a/include/drm/drm_writeback.h
>>>> +++ b/include/drm/drm_writeback.h
>>>> @@ -25,13 +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;
>>>> +
>>>> + /**
>>>> + * @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 encoder;
>>>> + struct drm_encoder internal_encoder;
>>>
>>> None of this belong here.
>> Alright, will split to the next patch.
>>>
>>>> /**
>>>> * @pixel_formats_blob_ptr:
>>>> @@ -150,7 +166,7 @@ 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);
>>>> + const u32 *formats, int n_formats, uint32_t possible_crtcs);
>>>> int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>>>> struct drm_framebuffer *fb);
>>>
>
More information about the Freedreno
mailing list