[Freedreno] [PATCH v4 03/20] drm: allow real encoder to be passed for drm_writeback_connector
Abhinav Kumar
quic_abhinavk at quicinc.com
Sun Apr 24 21:12:19 UTC 2022
Hi Laurent
On 4/24/2022 12:59 PM, Laurent Pinchart wrote:
> Hi Abhinav,
>
> On Sun, Apr 24, 2022 at 11:23:20AM -0700, Abhinav Kumar wrote:
>> On 4/24/2022 11:12 AM, Abhinav Kumar wrote:
>>> On 4/24/2022 7:50 AM, Laurent Pinchart wrote:
>>>> On Fri, Apr 22, 2022 at 04:06:38PM -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.
>>>>>
>>>>> For existing clients, drm_writeback_connector_init() will use
>>>>> an internal_encoder under the hood and hence no changes will
>>>>> be needed.
>>>>>
>>>>> changes in v7:
>>>>> - move this change before the vc4 change in the series
>>>>> to minimize the changes to vendor drivers in drm core
>>>>> changes
>>>>
>>>> Why is this needed ? The drm_writeback_connector functions don't need
>>>> the drm_encoder after drm_writeback_connector_init() (or
>>>> drm_writeback_connector_init_with_encoder()) returns.
>>>>
>>>
>>> Sorry I didnt follow this comment. This change log is incorrect, so
>>> after changing the previous change in the series and modifying this, no
>>> further changes are needed to vc4, so I decided to drop the next change.
>>> So this change log is incorrect. I can remove this.
>>>
>>> Is that what you meant?
>>
>> So till the previous change, the only user of
>> drm_writeback_connector_init_with_encoder() was
>> drm_writeback_connector_init() which was still passing its own
>> internal_encoder.
>>
>> Only if the wb_connector->encoder is changed to a pointer, other vendor
>> drivers can pass their own encoder to
>> drm_writeback_connector_init_with_encoder().
>>
>> Hence you are right that drm_writeback_connector functions do not need
>> drm_encoder after init() returns, but till this change is done, other
>> vendor drivers cannot directly call
>> drm_writeback_connector_init_with_encoder() because the encoder will not
>> be valid till then.
>
> Users of drm_writeback_connector_init_with_encoder() handle the encoder
> themselves, they can simply ignore drm_writeback_connector.encoder. The
> documentation of the encoder field needs to be updated though (I'd do so
> in the previous patch), to clearly mention that the field is valid only
> when using drm_writeback_connector_init(), not when calling
> drm_writeback_connector_init_with_encoder().
So you are suggesting to drop this change and just update the doc in
patch 2?
If so, I dont think it would be correct to assume that even though
drm_writeback_connector_init_with_encoder() is used
drm_writeback_connector.encoder should not be used.
The reason is that, lets consider, prepare_wb_job() and
cleanup_writeback_job().
These pass only the connector to the vendor hooks.
1126 *
1127 * This callback is used by the atomic modeset helpers.
1128 */
1129 int (*prepare_writeback_job)(struct drm_writeback_connector
*connector,
1130 struct drm_writeback_job *job);
Now, if you assume connector->encoder is not valid, this adds more
confusion because now we are putting an extra condition that
connector->encoder is not valid anymore.
Thats why I think we should leave it as-it-is to not allow room for such
discrepancies.
>
>> Hope this clarifies it.
>>
>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>> ---
>>>>> drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
>>>>> drivers/gpu/drm/vc4/vc4_txp.c | 4 ++--
>>>>> include/drm/drm_writeback.h | 22 ++++++++++++++++++++--
>>>>> 3 files changed, 34 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_writeback.c
>>>>> b/drivers/gpu/drm/drm_writeback.c
>>>>> index 92658ad..0538674 100644
>>>>> --- a/drivers/gpu/drm/drm_writeback.c
>>>>> +++ b/drivers/gpu/drm/drm_writeback.c
>>>>> @@ -180,21 +180,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;
>>>>> }
>>>>> @@ -239,6 +239,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 3447eb6..7e063a9 100644
>>>>> --- a/drivers/gpu/drm/vc4/vc4_txp.c
>>>>> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
>>>>> @@ -159,7 +159,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, connector.internal_encoder);
>>>>> }
>>>>> static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
>>>>> @@ -507,7 +507,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 bb306fa..3fbae9d 100644
>>>>> --- a/include/drm/drm_writeback.h
>>>>> +++ b/include/drm/drm_writeback.h
>>>>> @@ -25,13 +25,31 @@ 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
>>>>> + * 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 encoder;
>>>>> + struct drm_encoder internal_encoder;
>>>>> /**
>>>>> * @pixel_formats_blob_ptr:
>
More information about the dri-devel
mailing list