| Message ID | 1650668815-7048-4-git-send-email-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add writeback block support for DPU | expand |
Hi Abhinav, Thank you for the patch. 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. > 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:
Hi Laurent On 4/24/2022 7:50 AM, Laurent Pinchart wrote: > Hi Abhinav, > > Thank you for the patch. > > 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? >> 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: >
On 4/24/2022 11:12 AM, Abhinav Kumar wrote: > Hi Laurent > > On 4/24/2022 7:50 AM, Laurent Pinchart wrote: >> Hi Abhinav, >> >> Thank you for the patch. >> >> 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. 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: >>
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(). > 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:
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@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: >
Hi Laurent On 4/24/2022 2:12 PM, Abhinav Kumar wrote: > 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. I was able to abandon this change and still make writeback work with just trivial changes to MSM driver. So we are okay to drop this change as you requested and let the vendor driver manage the encoder completely. If you do think that this change is worth re-visiting sometime, let me know I can absorb it as a separate change. Thanks Abhinav > >> >>> 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: >>
On Sun, 24 Apr 2022 at 22:59, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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(). 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.) > > > 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: > > -- > Regards, > > Laurent Pinchart
On 4/25/2022 3:50 AM, Dmitry Baryshkov wrote: > On Sun, 24 Apr 2022 at 22:59, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> 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(). > > 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.) > Agree with your comment Dmitry and thats why I was suggesting to let drm_writeback_connector.encoder remain a valid pointer to avoid discrepancies. If we go with option (1), this change as-it-is is the best way to go. If we go with option (2), to drop the encoder field and just rename encoder ---> internal_encoder, it will still work for existing users. But I would prefer that goes as a separate change then because MSM doesn't really need this ( and at this point even other drivers don't ) I am still ready to post the change separately and do basic validation with our boards. >> >>> 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: >> >> -- >> Regards, >> >> Laurent Pinchart > > >
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. > > > 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:
Hi Laurent 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. >>>> 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: >
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:
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: