| Message ID | 1649465635-20542-4-git-send-email-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow drm_writeback_connector to accept pointer to drm_encoder | expand |
On Sat, 9 Apr 2022 at 03:54, Abhinav Kumar <quic_abhinavk@quicinc.com> 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 > > 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: > -- > 2.7.4 >
Hi Abhinav, Sorry for the delay in reviewing this, Easter break happened in between. On Fri, Apr 08, 2022 at 05:53:54PM -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 > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > 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); This doesn't feel right. First, vc4_txp should not be aware of the existance of internal_encoder, that is only for the use of drm_writeback_connector when someone doesn't pass on an encoder. Second, how do we know someone didn't actually use the 'encoder' pointer from drm_writeback_connector? I think we should be consistent and only use encoder member. Best regards, Liviu > } > > 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: > -- > 2.7.4 >
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:
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 Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- 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(-)