On Mon, Apr 25, 2022 at 10:48:07AM -0700, Abhinav Kumar wrote:
On 4/25/2022 10:32 AM, Laurent Pinchart wrote:
On Mon, Apr 25, 2022 at 01:50:43PM +0300, Dmitry Baryshkov wrote:
On Sun, 24 Apr 2022 at 22:59, Laurent Pinchart wrote:
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().
If we allow it to be unitialized, it might end with hard-to-trace bugs, additional conditions, etc. In my opnion we should:
- either make drm_writeback_connector.encoder a valid pointer
- or drop the field completely.
And up to some point I'd vote for the second option. The code using internal_encoder can continue using it directly. The code using drm_writeback_connector_init_with_encoder() will manage encoder on their own. We will loose a single entry point for wb's encoder, but do we really need it? (Frankly speaking I didn't check.)
As far as I understand, we went for the second option as Abhinav dropped this patch for the next version.
I dropped the patch as there was no agreement yet and I didnt want to block the series as its not really needed now but thats not option 2 because wb_conn->encoder field remains there, just that its unused for users using drm_writeback_connector_init_with_encoder().
I guess what Dmitry is suggesting is just drop that wb_connector->encoder field completely.
I don't think we can do that, as that field is used for the internal encoder :-) It could be renamed to internal_encoder if desired, but not dropped. I don't mind much either way, as long as we make it clear that drivers must never reference it directly.
Hope this clarifies it.
>> Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com >> Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@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: