| Message ID | 1646963400-25606-2-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 Fri, 11 Mar 2022 at 04:50, 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. > > Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com> > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > drivers/gpu/drm/drm_writeback.c | 8 ++++---- > include/drm/drm_writeback.h | 13 +++++++++++-- > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > index dccf4504..4dad687 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev, > if (IS_ERR(blob)) > return PTR_ERR(blob); > > - drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); > - ret = drm_encoder_init(dev, &wb_connector->encoder, > + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); > + ret = drm_encoder_init(dev, wb_connector->encoder, > &drm_writeback_encoder_funcs, > DRM_MODE_ENCODER_VIRTUAL, NULL); If the encoder is provided by a separate driver, it might use a different set of encoder funcs. I'd suggest checking whether the wb_connector->encoder is NULL here. If it is, allocate one using drmm_kzalloc and init it. If it is not NULL, assume that it has been initialized already, so skip the drm_encoder_init() and just call the drm_encoder_helper_add() > if (ret) > @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > goto connector_fail; > > ret = drm_connector_attach_encoder(connector, > - &wb_connector->encoder); > + wb_connector->encoder); > if (ret) > goto attach_fail; > > @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > attach_fail: > drm_connector_cleanup(connector); > connector_fail: > - drm_encoder_cleanup(&wb_connector->encoder); > + drm_encoder_cleanup(wb_connector->encoder); > fail: > drm_property_blob_put(blob); > return ret; > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > index 9697d27..0ba266e 100644 > --- a/include/drm/drm_writeback.h > +++ b/include/drm/drm_writeback.h > @@ -25,13 +25,22 @@ 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; > + struct drm_encoder *encoder; > > /** > * @pixel_formats_blob_ptr: > -- > 2.7.4 >
On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote: > On Fri, 11 Mar 2022 at 04:50, 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. > > > > Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com> > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > --- > > drivers/gpu/drm/drm_writeback.c | 8 ++++---- > > include/drm/drm_writeback.h | 13 +++++++++++-- > > 2 files changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > index dccf4504..4dad687 100644 > > --- a/drivers/gpu/drm/drm_writeback.c > > +++ b/drivers/gpu/drm/drm_writeback.c > > @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev, > > if (IS_ERR(blob)) > > return PTR_ERR(blob); > > > > - drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); > > - ret = drm_encoder_init(dev, &wb_connector->encoder, > > + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); > > + ret = drm_encoder_init(dev, wb_connector->encoder, > > &drm_writeback_encoder_funcs, > > DRM_MODE_ENCODER_VIRTUAL, NULL); > > If the encoder is provided by a separate driver, it might use a > different set of encoder funcs. More than that, if the encoder is provided externally but doesn't have custom operations, I don't really see the point of having an external encoder in the first place. Has this series been tested with a driver that needs to provide an encoder, to make sure it fits the purpose ? > I'd suggest checking whether the wb_connector->encoder is NULL here. > If it is, allocate one using drmm_kzalloc and init it. > If it is not NULL, assume that it has been initialized already, so > skip the drm_encoder_init() and just call the drm_encoder_helper_add() > > > if (ret) > > @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > > goto connector_fail; > > > > ret = drm_connector_attach_encoder(connector, > > - &wb_connector->encoder); > > + wb_connector->encoder); > > if (ret) > > goto attach_fail; > > > > @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > > attach_fail: > > drm_connector_cleanup(connector); > > connector_fail: > > - drm_encoder_cleanup(&wb_connector->encoder); > > + drm_encoder_cleanup(wb_connector->encoder); > > fail: > > drm_property_blob_put(blob); > > return ret; > > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > > index 9697d27..0ba266e 100644 > > --- a/include/drm/drm_writeback.h > > +++ b/include/drm/drm_writeback.h > > @@ -25,13 +25,22 @@ 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; > > + struct drm_encoder *encoder; > > > > /** > > * @pixel_formats_blob_ptr:
Hi Dmitry and Laurent On 3/11/2022 12:05 AM, Laurent Pinchart wrote: > On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote: >> On Fri, 11 Mar 2022 at 04:50, 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. >>> >>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com> >>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >>> --- >>> drivers/gpu/drm/drm_writeback.c | 8 ++++---- >>> include/drm/drm_writeback.h | 13 +++++++++++-- >>> 2 files changed, 15 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c >>> index dccf4504..4dad687 100644 >>> --- a/drivers/gpu/drm/drm_writeback.c >>> +++ b/drivers/gpu/drm/drm_writeback.c >>> @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev, >>> if (IS_ERR(blob)) >>> return PTR_ERR(blob); >>> >>> - drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); >>> - ret = drm_encoder_init(dev, &wb_connector->encoder, >>> + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); >>> + ret = drm_encoder_init(dev, wb_connector->encoder, >>> &drm_writeback_encoder_funcs, >>> DRM_MODE_ENCODER_VIRTUAL, NULL); >> >> If the encoder is provided by a separate driver, it might use a >> different set of encoder funcs. > > More than that, if the encoder is provided externally but doesn't have > custom operations, I don't really see the point of having an external > encoder in the first place. > > Has this series been tested with a driver that needs to provide an > encoder, to make sure it fits the purpose ? > Yes, I have tested this with the MSM driver which provides an encoder and yes it absolutely fits the purpose. >> I'd suggest checking whether the wb_connector->encoder is NULL here. >> If it is, allocate one using drmm_kzalloc and init it. >> If it is not NULL, assume that it has been initialized already, so >> skip the drm_encoder_init() and just call the drm_encoder_helper_add() You are both right. We can skip the drm_encoder_init for drivers which have already provided an encoder. The only issue I was facing with that is some of the drivers for example the below one, access the "wb_conn->encoder.possible_crtcs" before the call to drm_writeback_connector_init(). 198 int rcar_du_writeback_init(struct rcar_du_device *rcdu, 199 struct rcar_du_crtc *rcrtc) 200 { 201 struct drm_writeback_connector *wb_conn = &rcrtc->writeback; 202 203 wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); 204 drm_connector_helper_add(&wb_conn->base, 205 &rcar_du_wb_conn_helper_funcs); 206 207 return drm_writeback_connector_init(&rcdu->ddev, wb_conn, 208 &rcar_du_wb_conn_funcs, 209 &rcar_du_wb_enc_helper_funcs, 210 writeback_formats, 211 ARRAY_SIZE(writeback_formats)); If we allocate the encoder within drm_writeback_connector_init(), do you suggest I modify the drivers to move the usage of possible_crtcs after the drm_writeback_connector_init() call to avoid NULL ptr crash? >> >>> if (ret) >>> @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev, >>> goto connector_fail; >>> >>> ret = drm_connector_attach_encoder(connector, >>> - &wb_connector->encoder); >>> + wb_connector->encoder); >>> if (ret) >>> goto attach_fail; >>> >>> @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev, >>> attach_fail: >>> drm_connector_cleanup(connector); >>> connector_fail: >>> - drm_encoder_cleanup(&wb_connector->encoder); >>> + drm_encoder_cleanup(wb_connector->encoder); >>> fail: >>> drm_property_blob_put(blob); >>> return ret; >>> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h >>> index 9697d27..0ba266e 100644 >>> --- a/include/drm/drm_writeback.h >>> +++ b/include/drm/drm_writeback.h >>> @@ -25,13 +25,22 @@ 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; >>> + struct drm_encoder *encoder; >>> >>> /** >>> * @pixel_formats_blob_ptr: >
On Fri, 11 Mar 2022 at 20:09, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > Hi Dmitry and Laurent > > On 3/11/2022 12:05 AM, Laurent Pinchart wrote: > > On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote: > >> On Fri, 11 Mar 2022 at 04:50, 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. > >>> > >>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com> > >>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > >>> --- > >>> drivers/gpu/drm/drm_writeback.c | 8 ++++---- > >>> include/drm/drm_writeback.h | 13 +++++++++++-- > >>> 2 files changed, 15 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > >>> index dccf4504..4dad687 100644 > >>> --- a/drivers/gpu/drm/drm_writeback.c > >>> +++ b/drivers/gpu/drm/drm_writeback.c > >>> @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev, > >>> if (IS_ERR(blob)) > >>> return PTR_ERR(blob); > >>> > >>> - drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); > >>> - ret = drm_encoder_init(dev, &wb_connector->encoder, > >>> + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); > >>> + ret = drm_encoder_init(dev, wb_connector->encoder, > >>> &drm_writeback_encoder_funcs, > >>> DRM_MODE_ENCODER_VIRTUAL, NULL); > >> > >> If the encoder is provided by a separate driver, it might use a > >> different set of encoder funcs. > > > > More than that, if the encoder is provided externally but doesn't have > > custom operations, I don't really see the point of having an external > > encoder in the first place. > > > > Has this series been tested with a driver that needs to provide an > > encoder, to make sure it fits the purpose ? > > > > Yes, I have tested this with the MSM driver which provides an encoder > and yes it absolutely fits the purpose. > > > >> I'd suggest checking whether the wb_connector->encoder is NULL here. > >> If it is, allocate one using drmm_kzalloc and init it. > >> If it is not NULL, assume that it has been initialized already, so > >> skip the drm_encoder_init() and just call the drm_encoder_helper_add() > > You are both right. We can skip the drm_encoder_init for drivers which > have already provided an encoder. > > The only issue I was facing with that is some of the drivers for example > the below one, access the "wb_conn->encoder.possible_crtcs" before the > call to drm_writeback_connector_init(). Yes. please do. > > 198 int rcar_du_writeback_init(struct rcar_du_device *rcdu, > 199 struct rcar_du_crtc *rcrtc) > 200 { > 201 struct drm_writeback_connector *wb_conn = &rcrtc->writeback; > 202 > 203 wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); > 204 drm_connector_helper_add(&wb_conn->base, > 205 &rcar_du_wb_conn_helper_funcs); > 206 > 207 return drm_writeback_connector_init(&rcdu->ddev, wb_conn, > 208 &rcar_du_wb_conn_funcs, > 209 &rcar_du_wb_enc_helper_funcs, > 210 writeback_formats, > 211 ARRAY_SIZE(writeback_formats)); > > If we allocate the encoder within drm_writeback_connector_init(), do you > suggest I modify the drivers to move the usage of possible_crtcs after > the drm_writeback_connector_init() call to avoid NULL ptr crash? > > > >> > >>> if (ret) > >>> @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > >>> goto connector_fail; > >>> > >>> ret = drm_connector_attach_encoder(connector, > >>> - &wb_connector->encoder); > >>> + wb_connector->encoder); > >>> if (ret) > >>> goto attach_fail; > >>> > >>> @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > >>> attach_fail: > >>> drm_connector_cleanup(connector); > >>> connector_fail: > >>> - drm_encoder_cleanup(&wb_connector->encoder); > >>> + drm_encoder_cleanup(wb_connector->encoder); > >>> fail: > >>> drm_property_blob_put(blob); > >>> return ret; > >>> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > >>> index 9697d27..0ba266e 100644 > >>> --- a/include/drm/drm_writeback.h > >>> +++ b/include/drm/drm_writeback.h > >>> @@ -25,13 +25,22 @@ 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; > >>> + struct drm_encoder *encoder; > >>> > >>> /** > >>> * @pixel_formats_blob_ptr: > >
On Fri, Mar 11, 2022 at 10:05:53AM +0200, Laurent Pinchart wrote: > On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote: > > On Fri, 11 Mar 2022 at 04:50, 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. > > > > > > Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com> > > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > > --- > > > drivers/gpu/drm/drm_writeback.c | 8 ++++---- > > > include/drm/drm_writeback.h | 13 +++++++++++-- > > > 2 files changed, 15 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > > index dccf4504..4dad687 100644 > > > --- a/drivers/gpu/drm/drm_writeback.c > > > +++ b/drivers/gpu/drm/drm_writeback.c > > > @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev, > > > if (IS_ERR(blob)) > > > return PTR_ERR(blob); > > > > > > - drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); > > > - ret = drm_encoder_init(dev, &wb_connector->encoder, > > > + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); > > > + ret = drm_encoder_init(dev, wb_connector->encoder, > > > &drm_writeback_encoder_funcs, > > > DRM_MODE_ENCODER_VIRTUAL, NULL); > > > > If the encoder is provided by a separate driver, it might use a > > different set of encoder funcs. > > More than that, if the encoder is provided externally but doesn't have > custom operations, I don't really see the point of having an external > encoder in the first place. > > Has this series been tested with a driver that needs to provide an > encoder, to make sure it fits the purpose ? Also, can we not force all drivers to do this setup that don't need it? We have a ton of kms drivers, forcing unnecessary busiwork on drivers is really not good. -Daniel > > > I'd suggest checking whether the wb_connector->encoder is NULL here. > > If it is, allocate one using drmm_kzalloc and init it. > > If it is not NULL, assume that it has been initialized already, so > > skip the drm_encoder_init() and just call the drm_encoder_helper_add() > > > > > if (ret) > > > @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > > > goto connector_fail; > > > > > > ret = drm_connector_attach_encoder(connector, > > > - &wb_connector->encoder); > > > + wb_connector->encoder); > > > if (ret) > > > goto attach_fail; > > > > > > @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev, > > > attach_fail: > > > drm_connector_cleanup(connector); > > > connector_fail: > > > - drm_encoder_cleanup(&wb_connector->encoder); > > > + drm_encoder_cleanup(wb_connector->encoder); > > > fail: > > > drm_property_blob_put(blob); > > > return ret; > > > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h > > > index 9697d27..0ba266e 100644 > > > --- a/include/drm/drm_writeback.h > > > +++ b/include/drm/drm_writeback.h > > > @@ -25,13 +25,22 @@ 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; > > > + struct drm_encoder *encoder; > > > > > > /** > > > * @pixel_formats_blob_ptr: > > -- > Regards, > > Laurent Pinchart
Hi Daniel On 3/17/2022 3:01 AM, Daniel Vetter wrote: > On Fri, Mar 11, 2022 at 10:05:53AM +0200, Laurent Pinchart wrote: >> On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote: >>> On Fri, 11 Mar 2022 at 04:50, 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. >>>> >>>> Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com> >>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >>>> --- >>>> drivers/gpu/drm/drm_writeback.c | 8 ++++---- >>>> include/drm/drm_writeback.h | 13 +++++++++++-- >>>> 2 files changed, 15 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c >>>> index dccf4504..4dad687 100644 >>>> --- a/drivers/gpu/drm/drm_writeback.c >>>> +++ b/drivers/gpu/drm/drm_writeback.c >>>> @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev, >>>> if (IS_ERR(blob)) >>>> return PTR_ERR(blob); >>>> >>>> - drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); >>>> - ret = drm_encoder_init(dev, &wb_connector->encoder, >>>> + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); >>>> + ret = drm_encoder_init(dev, wb_connector->encoder, >>>> &drm_writeback_encoder_funcs, >>>> DRM_MODE_ENCODER_VIRTUAL, NULL); >>> >>> If the encoder is provided by a separate driver, it might use a >>> different set of encoder funcs. >> >> More than that, if the encoder is provided externally but doesn't have >> custom operations, I don't really see the point of having an external >> encoder in the first place. >> >> Has this series been tested with a driver that needs to provide an >> encoder, to make sure it fits the purpose ? > > Also, can we not force all drivers to do this setup that don't need it? We > have a ton of kms drivers, forcing unnecessary busiwork on drivers is > really not good. > -Daniel No, there will not be any requirement forced for existing drivers. A new API will be exposed for new clients which setup the encoder. > >> >>> I'd suggest checking whether the wb_connector->encoder is NULL here. >>> If it is, allocate one using drmm_kzalloc and init it. >>> If it is not NULL, assume that it has been initialized already, so >>> skip the drm_encoder_init() and just call the drm_encoder_helper_add() >>> >>>> if (ret) >>>> @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev, >>>> goto connector_fail; >>>> >>>> ret = drm_connector_attach_encoder(connector, >>>> - &wb_connector->encoder); >>>> + wb_connector->encoder); >>>> if (ret) >>>> goto attach_fail; >>>> >>>> @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev, >>>> attach_fail: >>>> drm_connector_cleanup(connector); >>>> connector_fail: >>>> - drm_encoder_cleanup(&wb_connector->encoder); >>>> + drm_encoder_cleanup(wb_connector->encoder); >>>> fail: >>>> drm_property_blob_put(blob); >>>> return ret; >>>> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h >>>> index 9697d27..0ba266e 100644 >>>> --- a/include/drm/drm_writeback.h >>>> +++ b/include/drm/drm_writeback.h >>>> @@ -25,13 +25,22 @@ 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; >>>> + struct drm_encoder *encoder; >>>> >>>> /** >>>> * @pixel_formats_blob_ptr: >> >> -- >> Regards, >> >> Laurent Pinchart >
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index dccf4504..4dad687 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -189,8 +189,8 @@ int drm_writeback_connector_init(struct drm_device *dev, if (IS_ERR(blob)) return PTR_ERR(blob); - drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs); - ret = drm_encoder_init(dev, &wb_connector->encoder, + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs); + ret = drm_encoder_init(dev, wb_connector->encoder, &drm_writeback_encoder_funcs, DRM_MODE_ENCODER_VIRTUAL, NULL); if (ret) @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev, goto connector_fail; ret = drm_connector_attach_encoder(connector, - &wb_connector->encoder); + wb_connector->encoder); if (ret) goto attach_fail; @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev, attach_fail: drm_connector_cleanup(connector); connector_fail: - drm_encoder_cleanup(&wb_connector->encoder); + drm_encoder_cleanup(wb_connector->encoder); fail: drm_property_blob_put(blob); return ret; diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 9697d27..0ba266e 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -25,13 +25,22 @@ 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; + struct drm_encoder *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. Co-developed-by: Kandpal Suraj <suraj.kandpal@intel.com> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/gpu/drm/drm_writeback.c | 8 ++++---- include/drm/drm_writeback.h | 13 +++++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-)