| Message ID | 20220202085429.22261-6-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm writeback connector changes | expand |
Hi Kandpal, Thank you for the patch. On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > Changing rcar_du driver to accomadate the change of > drm_writeback_connector.base and drm_writeback_connector.encoder > to a pointer the reason for which is explained in the > Patch(drm: add writeback pointers to drm_connector). > > Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > index 66e8839db708..68f387a04502 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > @@ -72,6 +72,8 @@ struct rcar_du_crtc { > const char *const *sources; > unsigned int sources_count; > > + struct drm_connector connector; > + struct drm_encoder encoder; Those fields are, at best, poorly named. Furthermore, there's no need in this driver or in other drivers using drm_writeback_connector to create an encoder or connector manually. Let's not polute all drivers because i915 doesn't have its abstractions right. Nack. > struct drm_writeback_connector writeback; > }; > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > index c79d1259e49b..5b1e83380c47 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, > { > struct drm_writeback_connector *wb_conn = &rcrtc->writeback; > > - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); > - drm_connector_helper_add(&wb_conn->base, > + wb_conn->base = &rcrtc->connector; > + wb_conn->encoder = &rcrtc->encoder; > + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); > + drm_connector_helper_add(wb_conn->base, > &rcar_du_wb_conn_helper_funcs); > > return drm_writeback_connector_init(&rcdu->ddev, wb_conn, > @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, > struct drm_framebuffer *fb; > unsigned int i; > > - state = rcrtc->writeback.base.state; > + state = rcrtc->writeback.base->state; > if (!state || !state->writeback_job) > return; >
On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Kandpal, > > Thank you for the patch. > > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: >> Changing rcar_du driver to accomadate the change of >> drm_writeback_connector.base and drm_writeback_connector.encoder >> to a pointer the reason for which is explained in the >> Patch(drm: add writeback pointers to drm_connector). >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> >> --- >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- >> 2 files changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> index 66e8839db708..68f387a04502 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { >> const char *const *sources; >> unsigned int sources_count; >> >> + struct drm_connector connector; >> + struct drm_encoder encoder; > > Those fields are, at best, poorly named. Furthermore, there's no need in > this driver or in other drivers using drm_writeback_connector to create > an encoder or connector manually. Let's not polute all drivers because > i915 doesn't have its abstractions right. i915 uses the quite common model for struct inheritance: struct intel_connector { struct drm_connector base; /* ... */ } Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, radeon, tilcdc, and vboxvideo. We could argue about the relative merits of that abstraction, but I think the bottom line is that it's popular and the drivers using it are not going to be persuaded to move away from it. It's no coincidence that the drivers who've implemented writeback so far (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, because the drm_writeback_connector midlayer does, forcing the issue. So I think drm_writeback_connector should *not* use the inheritance abstraction because it's a midlayer that should leave that option to the drivers. I think drm_writeback_connector needs to be changed to accommodate that, and, unfortunately, it means current writeback users need to be changed as well. I am not sure, however, if the series at hand is the right approach. Perhaps writeback can be modified to allocate the stuff for you if you prefer it that way, as long as the drm_connector is not embedded in struct drm_writeback_connector. BR, Jani. > > Nack. > >> struct drm_writeback_connector writeback; >> }; >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >> index c79d1259e49b..5b1e83380c47 100644 >> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >> @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, >> { >> struct drm_writeback_connector *wb_conn = &rcrtc->writeback; >> >> - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); >> - drm_connector_helper_add(&wb_conn->base, >> + wb_conn->base = &rcrtc->connector; >> + wb_conn->encoder = &rcrtc->encoder; >> + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); >> + drm_connector_helper_add(wb_conn->base, >> &rcar_du_wb_conn_helper_funcs); >> >> return drm_writeback_connector_init(&rcdu->ddev, wb_conn, >> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, >> struct drm_framebuffer *fb; >> unsigned int i; >> >> - state = rcrtc->writeback.base.state; >> + state = rcrtc->writeback.base->state; >> if (!state || !state->writeback_job) >> return; >>
Hi Jani, On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: > On Wed, 02 Feb 2022, Laurent Pinchart wrote: > > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > >> Changing rcar_du driver to accomadate the change of > >> drm_writeback_connector.base and drm_writeback_connector.encoder > >> to a pointer the reason for which is explained in the > >> Patch(drm: add writeback pointers to drm_connector). > >> > >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > >> --- > >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > >> 2 files changed, 7 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> index 66e8839db708..68f387a04502 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > >> const char *const *sources; > >> unsigned int sources_count; > >> > >> + struct drm_connector connector; > >> + struct drm_encoder encoder; > > > > Those fields are, at best, poorly named. Furthermore, there's no need in > > this driver or in other drivers using drm_writeback_connector to create > > an encoder or connector manually. Let's not polute all drivers because > > i915 doesn't have its abstractions right. > > i915 uses the quite common model for struct inheritance: > > struct intel_connector { > struct drm_connector base; > /* ... */ > } > > Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > radeon, tilcdc, and vboxvideo. > > We could argue about the relative merits of that abstraction, but I > think the bottom line is that it's popular and the drivers using it are > not going to be persuaded to move away from it. Nobody said inheritance is bad. > It's no coincidence that the drivers who've implemented writeback so far > (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > because the drm_writeback_connector midlayer does, forcing the issue. Are you sure it's not a coincidence ? :-) The encoder and especially connector created by drm_writeback_connector are there only because KMS requires a drm_encoder and a drm_connector to be exposed to userspace (and I could argue that using a connector for writeback is a hack, but that won't change). The connector is "virtual", I still fail to see why i915 or any other driver would need to wrap it into something else. The whole point of the drm_writeback_connector abstraction is that drivers do not have to manage the writeback drm_connector manually, they shouldn't touch it at all. > So I think drm_writeback_connector should *not* use the inheritance > abstraction because it's a midlayer that should leave that option to the > drivers. I think drm_writeback_connector needs to be changed to > accommodate that, and, unfortunately, it means current writeback users > need to be changed as well. > > I am not sure, however, if the series at hand is the right > approach. Perhaps writeback can be modified to allocate the stuff for > you if you prefer it that way, as long as the drm_connector is not > embedded in struct drm_writeback_connector. > > > Nack. > > > >> struct drm_writeback_connector writeback; > >> }; > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > >> index c79d1259e49b..5b1e83380c47 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > >> @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, > >> { > >> struct drm_writeback_connector *wb_conn = &rcrtc->writeback; > >> > >> - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); > >> - drm_connector_helper_add(&wb_conn->base, > >> + wb_conn->base = &rcrtc->connector; > >> + wb_conn->encoder = &rcrtc->encoder; > >> + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); > >> + drm_connector_helper_add(wb_conn->base, > >> &rcar_du_wb_conn_helper_funcs); > >> > >> return drm_writeback_connector_init(&rcdu->ddev, wb_conn, > >> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, > >> struct drm_framebuffer *fb; > >> unsigned int i; > >> > >> - state = rcrtc->writeback.base.state; > >> + state = rcrtc->writeback.base->state; > >> if (!state || !state->writeback_job) > >> return; > >>
On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: > On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Kandpal, > > > > Thank you for the patch. > > > > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > >> Changing rcar_du driver to accomadate the change of > >> drm_writeback_connector.base and drm_writeback_connector.encoder > >> to a pointer the reason for which is explained in the > >> Patch(drm: add writeback pointers to drm_connector). > >> > >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > >> --- > >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > >> 2 files changed, 7 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> index 66e8839db708..68f387a04502 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > >> const char *const *sources; > >> unsigned int sources_count; > >> > >> + struct drm_connector connector; > >> + struct drm_encoder encoder; > > > > Those fields are, at best, poorly named. Furthermore, there's no need in > > this driver or in other drivers using drm_writeback_connector to create > > an encoder or connector manually. Let's not polute all drivers because > > i915 doesn't have its abstractions right. > > i915 uses the quite common model for struct inheritance: > > struct intel_connector { > struct drm_connector base; > /* ... */ > } > > Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > radeon, tilcdc, and vboxvideo. > > We could argue about the relative merits of that abstraction, but I > think the bottom line is that it's popular and the drivers using it are > not going to be persuaded to move away from it. > > It's no coincidence that the drivers who've implemented writeback so far > (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > because the drm_writeback_connector midlayer does, forcing the issue. > > So I think drm_writeback_connector should *not* use the inheritance > abstraction because it's a midlayer that should leave that option to the > drivers. I think drm_writeback_connector needs to be changed to > accommodate that, and, unfortunately, it means current writeback users > need to be changed as well. > > I am not sure, however, if the series at hand is the right > approach. Perhaps writeback can be modified to allocate the stuff for > you if you prefer it that way, as long as the drm_connector is not > embedded in struct drm_writeback_connector. Maybe it's possible to split out the actual writeback functionality into a separate lighter struct that then gets embedded into the current drm_writeback_connector (which would therefore remain as a midlayer for the drivers that want one). And other drivers can embed the core writeback struct directly into their own things. Something like that should perhaps minimize the driver changes for the current users and we just need to adjust a bunch of things in drm_writeback.c/etc. to not depend on the midlayer stuff.
On Wed, 2 Feb 2022 at 16:15, Jani Nikula <jani.nikula@intel.com> wrote: > > On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Kandpal, > > > > Thank you for the patch. > > > > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > >> Changing rcar_du driver to accomadate the change of > >> drm_writeback_connector.base and drm_writeback_connector.encoder > >> to a pointer the reason for which is explained in the > >> Patch(drm: add writeback pointers to drm_connector). > >> > >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > >> --- > >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > >> 2 files changed, 7 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> index 66e8839db708..68f387a04502 100644 > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > >> const char *const *sources; > >> unsigned int sources_count; > >> > >> + struct drm_connector connector; > >> + struct drm_encoder encoder; > > > > Those fields are, at best, poorly named. Furthermore, there's no need in > > this driver or in other drivers using drm_writeback_connector to create > > an encoder or connector manually. Let's not polute all drivers because > > i915 doesn't have its abstractions right. > > i915 uses the quite common model for struct inheritance: > > struct intel_connector { > struct drm_connector base; > /* ... */ > } > > Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > radeon, tilcdc, and vboxvideo. For the reference. msm does not wrap drm_connector into any _common_ structure, which is used internally. > We could argue about the relative merits of that abstraction, but I > think the bottom line is that it's popular and the drivers using it are > not going to be persuaded to move away from it. As I wrote earlier, I am not sure if these drivers would try using their drm_connector subclass for writeback. ast: ast_connector = drm_connector + respective i2c adapter for EDID, not needed for WB fsl-dcu: fsl_dcu_drm_connector = drm_connector + drm_encoder pointer + drm_panel. Not needed for WB hisilicon, mgag200: same as ast tilcdc: same as fsl-dcu vboxdrv: the only driver that may possibly benefit from using vbox_connector in the writeback support, as the connector is bare drm_connector + crtc pointer + hints (width, height, disconnected). I have left amd, nouveau and radeon out of this list, too complex to analyze in several minutes. I'd second the proposal of supporting optional drm_encoder for drm_writeback_connector (as the crtc/encoder split can be vague), but I do not see the benefit for the drivers to use their own drm_connector subclass for drm_writeback. It well might be that we all misunderstand your design. Do you have a complete intel drm_writeback implementation based on this patchset? It would be easier to judge if the approach is correct seeing your target. > > It's no coincidence that the drivers who've implemented writeback so far > (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > because the drm_writeback_connector midlayer does, forcing the issue. > > So I think drm_writeback_connector should *not* use the inheritance > abstraction because it's a midlayer that should leave that option to the > drivers. I think drm_writeback_connector needs to be changed to > accommodate that, and, unfortunately, it means current writeback users > need to be changed as well. > > I am not sure, however, if the series at hand is the right > approach. Perhaps writeback can be modified to allocate the stuff for > you if you prefer it that way, as long as the drm_connector is not > embedded in struct drm_writeback_connector.
On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Jani, > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: >> On Wed, 02 Feb 2022, Laurent Pinchart wrote: >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: >> >> Changing rcar_du driver to accomadate the change of >> >> drm_writeback_connector.base and drm_writeback_connector.encoder >> >> to a pointer the reason for which is explained in the >> >> Patch(drm: add writeback pointers to drm_connector). >> >> >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> >> >> --- >> >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ >> >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- >> >> 2 files changed, 7 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> >> index 66e8839db708..68f387a04502 100644 >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { >> >> const char *const *sources; >> >> unsigned int sources_count; >> >> >> >> + struct drm_connector connector; >> >> + struct drm_encoder encoder; >> > >> > Those fields are, at best, poorly named. Furthermore, there's no need in >> > this driver or in other drivers using drm_writeback_connector to create >> > an encoder or connector manually. Let's not polute all drivers because >> > i915 doesn't have its abstractions right. >> >> i915 uses the quite common model for struct inheritance: >> >> struct intel_connector { >> struct drm_connector base; >> /* ... */ >> } >> >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, >> radeon, tilcdc, and vboxvideo. >> >> We could argue about the relative merits of that abstraction, but I >> think the bottom line is that it's popular and the drivers using it are >> not going to be persuaded to move away from it. > > Nobody said inheritance is bad. > >> It's no coincidence that the drivers who've implemented writeback so far >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, >> because the drm_writeback_connector midlayer does, forcing the issue. > > Are you sure it's not a coincidence ? :-) > > The encoder and especially connector created by drm_writeback_connector > are there only because KMS requires a drm_encoder and a drm_connector to > be exposed to userspace (and I could argue that using a connector for > writeback is a hack, but that won't change). The connector is "virtual", > I still fail to see why i915 or any other driver would need to wrap it > into something else. The whole point of the drm_writeback_connector > abstraction is that drivers do not have to manage the writeback > drm_connector manually, they shouldn't touch it at all. The thing is, drm_writeback_connector_init() calling drm_connector_init() on the drm_connector embedded in drm_writeback_connector leads to that connector being added to the drm_device's list of connectors. Ditto for the encoder. All the driver code that handles drm_connectors would need to take into account they might not be embedded in intel_connector. Throughout the driver. Ditto for the encoders. The point is, you can't initialize a connector or an encoder for a drm_device in isolation of the rest of the driver, even if it were supposed to be hidden away. BR, Jani. > >> So I think drm_writeback_connector should *not* use the inheritance >> abstraction because it's a midlayer that should leave that option to the >> drivers. I think drm_writeback_connector needs to be changed to >> accommodate that, and, unfortunately, it means current writeback users >> need to be changed as well. >> >> I am not sure, however, if the series at hand is the right >> approach. Perhaps writeback can be modified to allocate the stuff for >> you if you prefer it that way, as long as the drm_connector is not >> embedded in struct drm_writeback_connector. >> >> > Nack. >> > >> >> struct drm_writeback_connector writeback; >> >> }; >> >> >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >> >> index c79d1259e49b..5b1e83380c47 100644 >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >> >> @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, >> >> { >> >> struct drm_writeback_connector *wb_conn = &rcrtc->writeback; >> >> >> >> - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); >> >> - drm_connector_helper_add(&wb_conn->base, >> >> + wb_conn->base = &rcrtc->connector; >> >> + wb_conn->encoder = &rcrtc->encoder; >> >> + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); >> >> + drm_connector_helper_add(wb_conn->base, >> >> &rcar_du_wb_conn_helper_funcs); >> >> >> >> return drm_writeback_connector_init(&rcdu->ddev, wb_conn, >> >> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, >> >> struct drm_framebuffer *fb; >> >> unsigned int i; >> >> >> >> - state = rcrtc->writeback.base.state; >> >> + state = rcrtc->writeback.base->state; >> >> if (!state || !state->writeback_job) >> >> return; >> >>
On Wed, 02 Feb 2022, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > On Wed, 2 Feb 2022 at 16:15, Jani Nikula <jani.nikula@intel.com> wrote: >> >> On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> > Hi Kandpal, >> > >> > Thank you for the patch. >> > >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: >> >> Changing rcar_du driver to accomadate the change of >> >> drm_writeback_connector.base and drm_writeback_connector.encoder >> >> to a pointer the reason for which is explained in the >> >> Patch(drm: add writeback pointers to drm_connector). >> >> >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> >> >> --- >> >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ >> >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- >> >> 2 files changed, 7 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> >> index 66e8839db708..68f387a04502 100644 >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { >> >> const char *const *sources; >> >> unsigned int sources_count; >> >> >> >> + struct drm_connector connector; >> >> + struct drm_encoder encoder; >> > >> > Those fields are, at best, poorly named. Furthermore, there's no need in >> > this driver or in other drivers using drm_writeback_connector to create >> > an encoder or connector manually. Let's not polute all drivers because >> > i915 doesn't have its abstractions right. >> >> i915 uses the quite common model for struct inheritance: >> >> struct intel_connector { >> struct drm_connector base; >> /* ... */ >> } >> >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, >> radeon, tilcdc, and vboxvideo. > > For the reference. msm does not wrap drm_connector into any _common_ > structure, which is used internally. > >> We could argue about the relative merits of that abstraction, but I >> think the bottom line is that it's popular and the drivers using it are >> not going to be persuaded to move away from it. > > As I wrote earlier, I am not sure if these drivers would try using > their drm_connector subclass for writeback. > ast: ast_connector = drm_connector + respective i2c adapter for EDID, > not needed for WB > fsl-dcu: fsl_dcu_drm_connector = drm_connector + drm_encoder pointer + > drm_panel. Not needed for WB > hisilicon, mgag200: same as ast > tilcdc: same as fsl-dcu > vboxdrv: the only driver that may possibly benefit from using > vbox_connector in the writeback support, as the connector is bare > drm_connector + crtc pointer + hints (width, height, disconnected). > > I have left amd, nouveau and radeon out of this list, too complex to > analyze in several minutes. > > I'd second the proposal of supporting optional drm_encoder for > drm_writeback_connector (as the crtc/encoder split can be vague), but > I do not see the benefit for the drivers to use their own > drm_connector subclass for drm_writeback. If a driver uses inheritance throughout the driver, and a *different* subclass gets introduced into the mix, you need to add a ton of checks all over the place when you cast the superclass pointer to the subclass. The connector/encoder funcs you do have to pass to drm_writeback_connector_init() can't use any of the shared driver infrastructure that assume a certain inheritance. See also my reply to Laurent [1]. > It well might be that we all misunderstand your design. Do you have a > complete intel drm_writeback implementation based on this patchset? It > would be easier to judge if the approach is correct seeing your > target. That would be up to Suraj Kandpal. BR, Jani. [1] https://lore.kernel.org/r/87v8xxs2hz.fsf@intel.com > >> >> It's no coincidence that the drivers who've implemented writeback so far >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, >> because the drm_writeback_connector midlayer does, forcing the issue. >> >> So I think drm_writeback_connector should *not* use the inheritance >> abstraction because it's a midlayer that should leave that option to the >> drivers. I think drm_writeback_connector needs to be changed to >> accommodate that, and, unfortunately, it means current writeback users >> need to be changed as well. >> >> I am not sure, however, if the series at hand is the right >> approach. Perhaps writeback can be modified to allocate the stuff for >> you if you prefer it that way, as long as the drm_connector is not >> embedded in struct drm_writeback_connector.
On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jani, > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: > > On Wed, 02 Feb 2022, Laurent Pinchart wrote: > > > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > > >> Changing rcar_du driver to accomadate the change of > > >> drm_writeback_connector.base and drm_writeback_connector.encoder > > >> to a pointer the reason for which is explained in the > > >> Patch(drm: add writeback pointers to drm_connector). > > >> > > >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > > >> --- > > >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > > >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > > >> 2 files changed, 7 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >> index 66e8839db708..68f387a04502 100644 > > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > > >> const char *const *sources; > > >> unsigned int sources_count; > > >> > > >> + struct drm_connector connector; > > >> + struct drm_encoder encoder; > > > > > > Those fields are, at best, poorly named. Furthermore, there's no need in > > > this driver or in other drivers using drm_writeback_connector to create > > > an encoder or connector manually. Let's not polute all drivers because > > > i915 doesn't have its abstractions right. > > > > i915 uses the quite common model for struct inheritance: > > > > struct intel_connector { > > struct drm_connector base; > > /* ... */ > > } > > > > Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > > radeon, tilcdc, and vboxvideo. > > > > We could argue about the relative merits of that abstraction, but I > > think the bottom line is that it's popular and the drivers using it are > > not going to be persuaded to move away from it. > > Nobody said inheritance is bad. > > > It's no coincidence that the drivers who've implemented writeback so far > > (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > > because the drm_writeback_connector midlayer does, forcing the issue. > > Are you sure it's not a coincidence ? :-) > > The encoder and especially connector created by drm_writeback_connector > are there only because KMS requires a drm_encoder and a drm_connector to > be exposed to userspace (and I could argue that using a connector for > writeback is a hack, but that won't change). The connector is "virtual", > I still fail to see why i915 or any other driver would need to wrap it > into something else. The whole point of the drm_writeback_connector > abstraction is that drivers do not have to manage the writeback > drm_connector manually, they shouldn't touch it at all. Laurent, I wanted to shift a bit from the question of drm_connector to the question of drm_encoder being embedded in the drm_writeback_connector. In case of the msm driver the drm_encoder is not a lightweight entity, but a full-featured driver part. Significant part of it can be shared with the writeback implementation, if we allow using a pointer to the external drm_encoder with the drm_writeback_connector. Does the following patch set stand a chance to receive your ack? - Switch drm_writeback_connector to point to drm_encoder rather than embedding it? - Create drm_encoder for the drm_writeback_connector when one is not specified, so the current drivers can be left unchanged. > > > So I think drm_writeback_connector should *not* use the inheritance > > abstraction because it's a midlayer that should leave that option to the > > drivers. I think drm_writeback_connector needs to be changed to > > accommodate that, and, unfortunately, it means current writeback users > > need to be changed as well. > > > > I am not sure, however, if the series at hand is the right > > approach. Perhaps writeback can be modified to allocate the stuff for > > you if you prefer it that way, as long as the drm_connector is not > > embedded in struct drm_writeback_connector. > > > > > Nack. > > > > > >> struct drm_writeback_connector writeback; > > >> }; > > >> > > >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > > >> index c79d1259e49b..5b1e83380c47 100644 > > >> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > > >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > > >> @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, > > >> { > > >> struct drm_writeback_connector *wb_conn = &rcrtc->writeback; > > >> > > >> - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); > > >> - drm_connector_helper_add(&wb_conn->base, > > >> + wb_conn->base = &rcrtc->connector; > > >> + wb_conn->encoder = &rcrtc->encoder; > > >> + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); > > >> + drm_connector_helper_add(wb_conn->base, > > >> &rcar_du_wb_conn_helper_funcs); > > >> > > >> return drm_writeback_connector_init(&rcdu->ddev, wb_conn, > > >> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, > > >> struct drm_framebuffer *fb; > > >> unsigned int i; > > >> > > >> - state = rcrtc->writeback.base.state; > > >> + state = rcrtc->writeback.base->state; > > >> if (!state || !state->writeback_job) > > >> return; > > >> > > -- > Regards, > > Laurent Pinchart
Hi Laurent On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote: > On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> >> Hi Jani, >> >> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: >>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: >>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: >>>>> Changing rcar_du driver to accomadate the change of >>>>> drm_writeback_connector.base and drm_writeback_connector.encoder >>>>> to a pointer the reason for which is explained in the >>>>> Patch(drm: add writeback pointers to drm_connector). >>>>> >>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> >>>>> --- >>>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ >>>>> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- >>>>> 2 files changed, 7 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>> index 66e8839db708..68f387a04502 100644 >>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc { >>>>> const char *const *sources; >>>>> unsigned int sources_count; >>>>> >>>>> + struct drm_connector connector; >>>>> + struct drm_encoder encoder; >>>> >>>> Those fields are, at best, poorly named. Furthermore, there's no need in >>>> this driver or in other drivers using drm_writeback_connector to create >>>> an encoder or connector manually. Let's not polute all drivers because >>>> i915 doesn't have its abstractions right. >>> >>> i915 uses the quite common model for struct inheritance: >>> >>> struct intel_connector { >>> struct drm_connector base; >>> /* ... */ >>> } >>> >>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, >>> radeon, tilcdc, and vboxvideo. >>> >>> We could argue about the relative merits of that abstraction, but I >>> think the bottom line is that it's popular and the drivers using it are >>> not going to be persuaded to move away from it. >> >> Nobody said inheritance is bad. >> >>> It's no coincidence that the drivers who've implemented writeback so far >>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, >>> because the drm_writeback_connector midlayer does, forcing the issue. >> >> Are you sure it's not a coincidence ? :-) >> >> The encoder and especially connector created by drm_writeback_connector >> are there only because KMS requires a drm_encoder and a drm_connector to >> be exposed to userspace (and I could argue that using a connector for >> writeback is a hack, but that won't change). The connector is "virtual", >> I still fail to see why i915 or any other driver would need to wrap it >> into something else. The whole point of the drm_writeback_connector >> abstraction is that drivers do not have to manage the writeback >> drm_connector manually, they shouldn't touch it at all. > > Laurent, I wanted to shift a bit from the question of drm_connector to > the question of drm_encoder being embedded in the > drm_writeback_connector. > In case of the msm driver the drm_encoder is not a lightweight entity, > but a full-featured driver part. Significant part of it can be shared > with the writeback implementation, if we allow using a pointer to the > external drm_encoder with the drm_writeback_connector. > Does the following patch set stand a chance to receive your ack? > - Switch drm_writeback_connector to point to drm_encoder rather than > embedding it? > - Create drm_encoder for the drm_writeback_connector when one is not > specified, so the current drivers can be left unchanged. > I second Dmitry's request here. For the reasons he has mentioned along with the possibility of the writeback encoder being shared across display pipelines, strengthens our request of the drm encoder being a pointer inside the drm_writeback_connector instead of embedding it. Like I had shown in my RFC, in case the other drivers dont specify one, we can allocate one: https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/ We think this should be a reasonable accomodation to the existing drm_writeback driver. Thanks Abhinav >> >>> So I think drm_writeback_connector should *not* use the inheritance >>> abstraction because it's a midlayer that should leave that option to the >>> drivers. I think drm_writeback_connector needs to be changed to >>> accommodate that, and, unfortunately, it means current writeback users >>> need to be changed as well. >>> >>> I am not sure, however, if the series at hand is the right >>> approach. Perhaps writeback can be modified to allocate the stuff for >>> you if you prefer it that way, as long as the drm_connector is not >>> embedded in struct drm_writeback_connector. >>> >>>> Nack. >>>> >>>>> struct drm_writeback_connector writeback; >>>>> }; >>>>> >>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >>>>> index c79d1259e49b..5b1e83380c47 100644 >>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >>>>> @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, >>>>> { >>>>> struct drm_writeback_connector *wb_conn = &rcrtc->writeback; >>>>> >>>>> - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); >>>>> - drm_connector_helper_add(&wb_conn->base, >>>>> + wb_conn->base = &rcrtc->connector; >>>>> + wb_conn->encoder = &rcrtc->encoder; >>>>> + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); >>>>> + drm_connector_helper_add(wb_conn->base, >>>>> &rcar_du_wb_conn_helper_funcs); >>>>> >>>>> return drm_writeback_connector_init(&rcdu->ddev, wb_conn, >>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, >>>>> struct drm_framebuffer *fb; >>>>> unsigned int i; >>>>> >>>>> - state = rcrtc->writeback.base.state; >>>>> + state = rcrtc->writeback.base->state; >>>>> if (!state || !state->writeback_job) >>>>> return; >>>>> >> >> -- >> Regards, >> >> Laurent Pinchart > > >
Hi Laurent Gentle reminder on this. Thanks Abhinav On 2/6/2022 11:20 PM, Abhinav Kumar wrote: > Hi Laurent > > On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote: >> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart >> <laurent.pinchart@ideasonboard.com> wrote: >>> >>> Hi Jani, >>> >>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: >>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: >>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: >>>>>> Changing rcar_du driver to accomadate the change of >>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder >>>>>> to a pointer the reason for which is explained in the >>>>>> Patch(drm: add writeback pointers to drm_connector). >>>>>> >>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ >>>>>> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- >>>>>> 2 files changed, 7 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>> index 66e8839db708..68f387a04502 100644 >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc { >>>>>> const char *const *sources; >>>>>> unsigned int sources_count; >>>>>> >>>>>> + struct drm_connector connector; >>>>>> + struct drm_encoder encoder; >>>>> >>>>> Those fields are, at best, poorly named. Furthermore, there's no >>>>> need in >>>>> this driver or in other drivers using drm_writeback_connector to >>>>> create >>>>> an encoder or connector manually. Let's not polute all drivers because >>>>> i915 doesn't have its abstractions right. >>>> >>>> i915 uses the quite common model for struct inheritance: >>>> >>>> struct intel_connector { >>>> struct drm_connector base; >>>> /* ... */ >>>> } >>>> >>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, >>>> radeon, tilcdc, and vboxvideo. >>>> >>>> We could argue about the relative merits of that abstraction, but I >>>> think the bottom line is that it's popular and the drivers using it are >>>> not going to be persuaded to move away from it. >>> >>> Nobody said inheritance is bad. >>> >>>> It's no coincidence that the drivers who've implemented writeback so >>>> far >>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, >>>> because the drm_writeback_connector midlayer does, forcing the issue. >>> >>> Are you sure it's not a coincidence ? :-) >>> >>> The encoder and especially connector created by drm_writeback_connector >>> are there only because KMS requires a drm_encoder and a drm_connector to >>> be exposed to userspace (and I could argue that using a connector for >>> writeback is a hack, but that won't change). The connector is "virtual", >>> I still fail to see why i915 or any other driver would need to wrap it >>> into something else. The whole point of the drm_writeback_connector >>> abstraction is that drivers do not have to manage the writeback >>> drm_connector manually, they shouldn't touch it at all. >> >> Laurent, I wanted to shift a bit from the question of drm_connector to >> the question of drm_encoder being embedded in the >> drm_writeback_connector. >> In case of the msm driver the drm_encoder is not a lightweight entity, >> but a full-featured driver part. Significant part of it can be shared >> with the writeback implementation, if we allow using a pointer to the >> external drm_encoder with the drm_writeback_connector. >> Does the following patch set stand a chance to receive your ack? >> - Switch drm_writeback_connector to point to drm_encoder rather than >> embedding it? >> - Create drm_encoder for the drm_writeback_connector when one is not >> specified, so the current drivers can be left unchanged. >> > > I second Dmitry's request here. For the reasons he has mentioned along > with the possibility of the writeback encoder being shared across > display pipelines, strengthens our request of the drm encoder being a > pointer inside the drm_writeback_connector instead of embedding it. > > Like I had shown in my RFC, in case the other drivers dont specify one, > we can allocate one: > > https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/ > > > We think this should be a reasonable accomodation to the existing > drm_writeback driver. > > Thanks > > Abhinav > >>> >>>> So I think drm_writeback_connector should *not* use the inheritance >>>> abstraction because it's a midlayer that should leave that option to >>>> the >>>> drivers. I think drm_writeback_connector needs to be changed to >>>> accommodate that, and, unfortunately, it means current writeback users >>>> need to be changed as well. >>>> >>>> I am not sure, however, if the series at hand is the right >>>> approach. Perhaps writeback can be modified to allocate the stuff for >>>> you if you prefer it that way, as long as the drm_connector is not >>>> embedded in struct drm_writeback_connector. >>>> >>>>> Nack. >>>>> >>>>>> struct drm_writeback_connector writeback; >>>>>> }; >>>>>> >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >>>>>> index c79d1259e49b..5b1e83380c47 100644 >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >>>>>> @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct >>>>>> rcar_du_device *rcdu, >>>>>> { >>>>>> struct drm_writeback_connector *wb_conn = &rcrtc->writeback; >>>>>> >>>>>> - wb_conn->encoder.possible_crtcs = 1 << >>>>>> drm_crtc_index(&rcrtc->crtc); >>>>>> - drm_connector_helper_add(&wb_conn->base, >>>>>> + wb_conn->base = &rcrtc->connector; >>>>>> + wb_conn->encoder = &rcrtc->encoder; >>>>>> + wb_conn->encoder->possible_crtcs = 1 << >>>>>> drm_crtc_index(&rcrtc->crtc); >>>>>> + drm_connector_helper_add(wb_conn->base, >>>>>> &rcar_du_wb_conn_helper_funcs); >>>>>> >>>>>> return drm_writeback_connector_init(&rcdu->ddev, wb_conn, >>>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct >>>>>> rcar_du_crtc *rcrtc, >>>>>> struct drm_framebuffer *fb; >>>>>> unsigned int i; >>>>>> >>>>>> - state = rcrtc->writeback.base.state; >>>>>> + state = rcrtc->writeback.base->state; >>>>>> if (!state || !state->writeback_job) >>>>>> return; >>>>>> >>> >>> -- >>> Regards, >>> >>> Laurent Pinchart >> >> >>
Hi Abhinav, On Wed, Feb 09, 2022 at 05:40:29PM -0800, Abhinav Kumar wrote: > Hi Laurent > > Gentle reminder on this. I won't have time before next week I'm afraid. > On 2/6/2022 11:20 PM, Abhinav Kumar wrote: > > Hi Laurent > > > > On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote: > >> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart > >> <laurent.pinchart@ideasonboard.com> wrote: > >>> > >>> Hi Jani, > >>> > >>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: > >>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: > >>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > >>>>>> Changing rcar_du driver to accomadate the change of > >>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder > >>>>>> to a pointer the reason for which is explained in the > >>>>>> Patch(drm: add writeback pointers to drm_connector). > >>>>>> > >>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > >>>>>> --- > >>>>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > >>>>>> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > >>>>>> 2 files changed, 7 insertions(+), 3 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >>>>>> index 66e8839db708..68f387a04502 100644 > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > >>>>>> const char *const *sources; > >>>>>> unsigned int sources_count; > >>>>>> > >>>>>> + struct drm_connector connector; > >>>>>> + struct drm_encoder encoder; > >>>>> > >>>>> Those fields are, at best, poorly named. Furthermore, there's no > >>>>> need in > >>>>> this driver or in other drivers using drm_writeback_connector to > >>>>> create > >>>>> an encoder or connector manually. Let's not polute all drivers because > >>>>> i915 doesn't have its abstractions right. > >>>> > >>>> i915 uses the quite common model for struct inheritance: > >>>> > >>>> struct intel_connector { > >>>> struct drm_connector base; > >>>> /* ... */ > >>>> } > >>>> > >>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > >>>> radeon, tilcdc, and vboxvideo. > >>>> > >>>> We could argue about the relative merits of that abstraction, but I > >>>> think the bottom line is that it's popular and the drivers using it are > >>>> not going to be persuaded to move away from it. > >>> > >>> Nobody said inheritance is bad. > >>> > >>>> It's no coincidence that the drivers who've implemented writeback so > >>>> far > >>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > >>>> because the drm_writeback_connector midlayer does, forcing the issue. > >>> > >>> Are you sure it's not a coincidence ? :-) > >>> > >>> The encoder and especially connector created by drm_writeback_connector > >>> are there only because KMS requires a drm_encoder and a drm_connector to > >>> be exposed to userspace (and I could argue that using a connector for > >>> writeback is a hack, but that won't change). The connector is "virtual", > >>> I still fail to see why i915 or any other driver would need to wrap it > >>> into something else. The whole point of the drm_writeback_connector > >>> abstraction is that drivers do not have to manage the writeback > >>> drm_connector manually, they shouldn't touch it at all. > >> > >> Laurent, I wanted to shift a bit from the question of drm_connector to > >> the question of drm_encoder being embedded in the > >> drm_writeback_connector. > >> In case of the msm driver the drm_encoder is not a lightweight entity, > >> but a full-featured driver part. Significant part of it can be shared > >> with the writeback implementation, if we allow using a pointer to the > >> external drm_encoder with the drm_writeback_connector. > >> Does the following patch set stand a chance to receive your ack? > >> - Switch drm_writeback_connector to point to drm_encoder rather than > >> embedding it? > >> - Create drm_encoder for the drm_writeback_connector when one is not > >> specified, so the current drivers can be left unchanged. > >> > > > > I second Dmitry's request here. For the reasons he has mentioned along > > with the possibility of the writeback encoder being shared across > > display pipelines, strengthens our request of the drm encoder being a > > pointer inside the drm_writeback_connector instead of embedding it. > > > > Like I had shown in my RFC, in case the other drivers dont specify one, > > we can allocate one: > > > > https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/ > > > > > > We think this should be a reasonable accomodation to the existing > > drm_writeback driver. > > > > Thanks > > > > Abhinav > > > >>> > >>>> So I think drm_writeback_connector should *not* use the inheritance > >>>> abstraction because it's a midlayer that should leave that option to > >>>> the > >>>> drivers. I think drm_writeback_connector needs to be changed to > >>>> accommodate that, and, unfortunately, it means current writeback users > >>>> need to be changed as well. > >>>> > >>>> I am not sure, however, if the series at hand is the right > >>>> approach. Perhaps writeback can be modified to allocate the stuff for > >>>> you if you prefer it that way, as long as the drm_connector is not > >>>> embedded in struct drm_writeback_connector. > >>>> > >>>>> Nack. > >>>>> > >>>>>> struct drm_writeback_connector writeback; > >>>>>> }; > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > >>>>>> index c79d1259e49b..5b1e83380c47 100644 > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > >>>>>> @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct > >>>>>> rcar_du_device *rcdu, > >>>>>> { > >>>>>> struct drm_writeback_connector *wb_conn = &rcrtc->writeback; > >>>>>> > >>>>>> - wb_conn->encoder.possible_crtcs = 1 << > >>>>>> drm_crtc_index(&rcrtc->crtc); > >>>>>> - drm_connector_helper_add(&wb_conn->base, > >>>>>> + wb_conn->base = &rcrtc->connector; > >>>>>> + wb_conn->encoder = &rcrtc->encoder; > >>>>>> + wb_conn->encoder->possible_crtcs = 1 << > >>>>>> drm_crtc_index(&rcrtc->crtc); > >>>>>> + drm_connector_helper_add(wb_conn->base, > >>>>>> &rcar_du_wb_conn_helper_funcs); > >>>>>> > >>>>>> return drm_writeback_connector_init(&rcdu->ddev, wb_conn, > >>>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct > >>>>>> rcar_du_crtc *rcrtc, > >>>>>> struct drm_framebuffer *fb; > >>>>>> unsigned int i; > >>>>>> > >>>>>> - state = rcrtc->writeback.base.state; > >>>>>> + state = rcrtc->writeback.base->state; > >>>>>> if (!state || !state->writeback_job) > >>>>>> return; > >>>>>>
On Thu, 10 Feb 2022 at 07:59, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Abhinav, > > On Wed, Feb 09, 2022 at 05:40:29PM -0800, Abhinav Kumar wrote: > > Hi Laurent > > > > Gentle reminder on this. > > I won't have time before next week I'm afraid. Laurent, another gentle ping. > > > On 2/6/2022 11:20 PM, Abhinav Kumar wrote: > > > Hi Laurent > > > > > > On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote: > > >> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart > > >> <laurent.pinchart@ideasonboard.com> wrote: > > >>> > > >>> Hi Jani, > > >>> > > >>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: > > >>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: > > >>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > > >>>>>> Changing rcar_du driver to accomadate the change of > > >>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder > > >>>>>> to a pointer the reason for which is explained in the > > >>>>>> Patch(drm: add writeback pointers to drm_connector). > > >>>>>> > > >>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > > >>>>>> --- > > >>>>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > > >>>>>> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > > >>>>>> 2 files changed, 7 insertions(+), 3 deletions(-) > > >>>>>> > > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >>>>>> index 66e8839db708..68f387a04502 100644 > > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > > >>>>>> const char *const *sources; > > >>>>>> unsigned int sources_count; > > >>>>>> > > >>>>>> + struct drm_connector connector; > > >>>>>> + struct drm_encoder encoder; > > >>>>> > > >>>>> Those fields are, at best, poorly named. Furthermore, there's no > > >>>>> need in > > >>>>> this driver or in other drivers using drm_writeback_connector to > > >>>>> create > > >>>>> an encoder or connector manually. Let's not polute all drivers because > > >>>>> i915 doesn't have its abstractions right. > > >>>> > > >>>> i915 uses the quite common model for struct inheritance: > > >>>> > > >>>> struct intel_connector { > > >>>> struct drm_connector base; > > >>>> /* ... */ > > >>>> } > > >>>> > > >>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > > >>>> radeon, tilcdc, and vboxvideo. > > >>>> > > >>>> We could argue about the relative merits of that abstraction, but I > > >>>> think the bottom line is that it's popular and the drivers using it are > > >>>> not going to be persuaded to move away from it. > > >>> > > >>> Nobody said inheritance is bad. > > >>> > > >>>> It's no coincidence that the drivers who've implemented writeback so > > >>>> far > > >>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > > >>>> because the drm_writeback_connector midlayer does, forcing the issue. > > >>> > > >>> Are you sure it's not a coincidence ? :-) > > >>> > > >>> The encoder and especially connector created by drm_writeback_connector > > >>> are there only because KMS requires a drm_encoder and a drm_connector to > > >>> be exposed to userspace (and I could argue that using a connector for > > >>> writeback is a hack, but that won't change). The connector is "virtual", > > >>> I still fail to see why i915 or any other driver would need to wrap it > > >>> into something else. The whole point of the drm_writeback_connector > > >>> abstraction is that drivers do not have to manage the writeback > > >>> drm_connector manually, they shouldn't touch it at all. > > >> > > >> Laurent, I wanted to shift a bit from the question of drm_connector to > > >> the question of drm_encoder being embedded in the > > >> drm_writeback_connector. > > >> In case of the msm driver the drm_encoder is not a lightweight entity, > > >> but a full-featured driver part. Significant part of it can be shared > > >> with the writeback implementation, if we allow using a pointer to the > > >> external drm_encoder with the drm_writeback_connector. > > >> Does the following patch set stand a chance to receive your ack? > > >> - Switch drm_writeback_connector to point to drm_encoder rather than > > >> embedding it? > > >> - Create drm_encoder for the drm_writeback_connector when one is not > > >> specified, so the current drivers can be left unchanged. > > >> > > > > > > I second Dmitry's request here. For the reasons he has mentioned along > > > with the possibility of the writeback encoder being shared across > > > display pipelines, strengthens our request of the drm encoder being a > > > pointer inside the drm_writeback_connector instead of embedding it. > > > > > > Like I had shown in my RFC, in case the other drivers dont specify one, > > > we can allocate one: > > > > > > https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/ > > > > > > > > > We think this should be a reasonable accomodation to the existing > > > drm_writeback driver. > > > > > > Thanks > > > > > > Abhinav > > > > > >>> > > >>>> So I think drm_writeback_connector should *not* use the inheritance > > >>>> abstraction because it's a midlayer that should leave that option to > > >>>> the > > >>>> drivers. I think drm_writeback_connector needs to be changed to > > >>>> accommodate that, and, unfortunately, it means current writeback users > > >>>> need to be changed as well. > > >>>> > > >>>> I am not sure, however, if the series at hand is the right > > >>>> approach. Perhaps writeback can be modified to allocate the stuff for > > >>>> you if you prefer it that way, as long as the drm_connector is not > > >>>> embedded in struct drm_writeback_connector. > > >>>> > > >>>>> Nack. > > >>>>> > > >>>>>> struct drm_writeback_connector writeback; > > >>>>>> }; > > >>>>>> > > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > > >>>>>> index c79d1259e49b..5b1e83380c47 100644 > > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > > >>>>>> @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct > > >>>>>> rcar_du_device *rcdu, > > >>>>>> { > > >>>>>> struct drm_writeback_connector *wb_conn = &rcrtc->writeback; > > >>>>>> > > >>>>>> - wb_conn->encoder.possible_crtcs = 1 << > > >>>>>> drm_crtc_index(&rcrtc->crtc); > > >>>>>> - drm_connector_helper_add(&wb_conn->base, > > >>>>>> + wb_conn->base = &rcrtc->connector; > > >>>>>> + wb_conn->encoder = &rcrtc->encoder; > > >>>>>> + wb_conn->encoder->possible_crtcs = 1 << > > >>>>>> drm_crtc_index(&rcrtc->crtc); > > >>>>>> + drm_connector_helper_add(wb_conn->base, > > >>>>>> &rcar_du_wb_conn_helper_funcs); > > >>>>>> > > >>>>>> return drm_writeback_connector_init(&rcdu->ddev, wb_conn, > > >>>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct > > >>>>>> rcar_du_crtc *rcrtc, > > >>>>>> struct drm_framebuffer *fb; > > >>>>>> unsigned int i; > > >>>>>> > > >>>>>> - state = rcrtc->writeback.base.state; > > >>>>>> + state = rcrtc->writeback.base->state; > > >>>>>> if (!state || !state->writeback_job) > > >>>>>> return; > > >>>>>> > > -- > Regards, > > Laurent Pinchart
Hi Dmitry, On Tue, Feb 22, 2022 at 06:32:50AM +0300, Dmitry Baryshkov wrote: > On Thu, 10 Feb 2022 at 07:59, Laurent Pinchart wrote: > > On Wed, Feb 09, 2022 at 05:40:29PM -0800, Abhinav Kumar wrote: > > > Hi Laurent > > > > > > Gentle reminder on this. > > > > I won't have time before next week I'm afraid. > > Laurent, another gentle ping. I'm really late on this so I probably deserve a bit of a rougher ping, but thanks for being gentle :-) > > > On 2/6/2022 11:20 PM, Abhinav Kumar wrote: > > > > On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote: > > > >> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart wrote: > > > >>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: > > > >>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: > > > >>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > > > >>>>>> Changing rcar_du driver to accomadate the change of > > > >>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder > > > >>>>>> to a pointer the reason for which is explained in the > > > >>>>>> Patch(drm: add writeback pointers to drm_connector). > > > >>>>>> > > > >>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > > > >>>>>> --- > > > >>>>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > > > >>>>>> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > > > >>>>>> 2 files changed, 7 insertions(+), 3 deletions(-) > > > >>>>>> > > > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > > >>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > > >>>>>> index 66e8839db708..68f387a04502 100644 > > > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > > >>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > > > >>>>>> const char *const *sources; > > > >>>>>> unsigned int sources_count; > > > >>>>>> > > > >>>>>> + struct drm_connector connector; > > > >>>>>> + struct drm_encoder encoder; > > > >>>>> > > > >>>>> Those fields are, at best, poorly named. Furthermore, there's no need in > > > >>>>> this driver or in other drivers using drm_writeback_connector to create > > > >>>>> an encoder or connector manually. Let's not polute all drivers because > > > >>>>> i915 doesn't have its abstractions right. > > > >>>> > > > >>>> i915 uses the quite common model for struct inheritance: > > > >>>> > > > >>>> struct intel_connector { > > > >>>> struct drm_connector base; > > > >>>> /* ... */ > > > >>>> } > > > >>>> > > > >>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > > > >>>> radeon, tilcdc, and vboxvideo. > > > >>>> > > > >>>> We could argue about the relative merits of that abstraction, but I > > > >>>> think the bottom line is that it's popular and the drivers using it are > > > >>>> not going to be persuaded to move away from it. > > > >>> > > > >>> Nobody said inheritance is bad. > > > >>> > > > >>>> It's no coincidence that the drivers who've implemented writeback so far > > > >>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > > > >>>> because the drm_writeback_connector midlayer does, forcing the issue. > > > >>> > > > >>> Are you sure it's not a coincidence ? :-) > > > >>> > > > >>> The encoder and especially connector created by drm_writeback_connector > > > >>> are there only because KMS requires a drm_encoder and a drm_connector to > > > >>> be exposed to userspace (and I could argue that using a connector for > > > >>> writeback is a hack, but that won't change). The connector is "virtual", > > > >>> I still fail to see why i915 or any other driver would need to wrap it > > > >>> into something else. The whole point of the drm_writeback_connector > > > >>> abstraction is that drivers do not have to manage the writeback > > > >>> drm_connector manually, they shouldn't touch it at all. > > > >> > > > >> Laurent, I wanted to shift a bit from the question of drm_connector to > > > >> the question of drm_encoder being embedded in the drm_writeback_connector. > > > >> In case of the msm driver the drm_encoder is not a lightweight entity, > > > >> but a full-featured driver part. Significant part of it can be shared > > > >> with the writeback implementation, if we allow using a pointer to the > > > >> external drm_encoder with the drm_writeback_connector. > > > >> Does the following patch set stand a chance to receive your ack? > > > >> - Switch drm_writeback_connector to point to drm_encoder rather than > > > >> embedding it? > > > >> - Create drm_encoder for the drm_writeback_connector when one is not > > > >> specified, so the current drivers can be left unchanged. The situation is a bit different for the encoder indeed. The encoder concept is loosely defined nowadays, with more and more of the "real" encoders being implemented as a drm_bridge. That's what I usually recommend when reviewing new drivers. drm_encoder is slowly becoming an empty shell (see for instance [1]), although that transition is not enforced globally and will thus take a long time to complete (if ever). This being said, lots of drivers have "featureful" encoder implementations, and that won't go away any time soon. In those cases, I could be OK with drivers optionally passing an encoder fo the writeback helper if the hardware really shares resources between writeback and a real encoder. I would however be careful there, as in many cases I would expect the need to pass a custom encoder to originate from an old software design decision rather than from the hardware architecture. In those cases it would be best, I think, to move towards cleaning up the software architecture, but that can be done step by step and I won't consider that a requirement to implement writeback support. In the MSM case in particular, can you explain what resources are shared between writeback and hardware encoder(s) ? [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > > > > I second Dmitry's request here. For the reasons he has mentioned along > > > > with the possibility of the writeback encoder being shared across > > > > display pipelines, strengthens our request of the drm encoder being a > > > > pointer inside the drm_writeback_connector instead of embedding it. > > > > > > > > Like I had shown in my RFC, in case the other drivers dont specify one, > > > > we can allocate one: > > > > > > > > https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/ > > > > > > > > We think this should be a reasonable accomodation to the existing > > > > drm_writeback driver. > > > > > > > >>>> So I think drm_writeback_connector should *not* use the inheritance > > > >>>> abstraction because it's a midlayer that should leave that option tothe > > > >>>> drivers. I think drm_writeback_connector needs to be changed to > > > >>>> accommodate that, and, unfortunately, it means current writeback users > > > >>>> need to be changed as well. > > > >>>> > > > >>>> I am not sure, however, if the series at hand is the right > > > >>>> approach. Perhaps writeback can be modified to allocate the stuff for > > > >>>> you if you prefer it that way, as long as the drm_connector is not > > > >>>> embedded in struct drm_writeback_connector. > > > >>>> > > > >>>>> Nack. > > > >>>>> > > > >>>>>> struct drm_writeback_connector writeback; > > > >>>>>> }; > > > >>>>>> > > > >>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > > > >>>>>> index c79d1259e49b..5b1e83380c47 100644 > > > >>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > > > >>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > > > >>>>>> @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, > > > >>>>>> { > > > >>>>>> struct drm_writeback_connector *wb_conn = &rcrtc->writeback; > > > >>>>>> > > > >>>>>> - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); > > > >>>>>> - drm_connector_helper_add(&wb_conn->base, > > > >>>>>> + wb_conn->base = &rcrtc->connector; > > > >>>>>> + wb_conn->encoder = &rcrtc->encoder; > > > >>>>>> + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); > > > >>>>>> + drm_connector_helper_add(wb_conn->base, > > > >>>>>> &rcar_du_wb_conn_helper_funcs); > > > >>>>>> > > > >>>>>> return drm_writeback_connector_init(&rcdu->ddev, wb_conn, > > > >>>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, > > > >>>>>> struct drm_framebuffer *fb; > > > >>>>>> unsigned int i; > > > >>>>>> > > > >>>>>> - state = rcrtc->writeback.base.state; > > > >>>>>> + state = rcrtc->writeback.base->state; > > > >>>>>> if (!state || !state->writeback_job) > > > >>>>>> return; > > > >>>>>>
Hey, > The connector/encoder funcs you do have to pass to > drm_writeback_connector_init() can't use any of the shared driver > infrastructure that assume a certain inheritance. > > See also my reply to Laurent [1]. > > > It well might be that we all misunderstand your design. Do you have a > > complete intel drm_writeback implementation based on this patchset? It > > would be easier to judge if the approach is correct seeing your > > target. > > That would be up to Suraj Kandpal. I have floated the intel drm_writeback implementation. Please refer to [1] to get a better understanding of how we are implementing writeback functionality. [1] https://patchwork.freedesktop.org/series/100617/ Thanks, Suraj Kandpal
Hi Laurent Thanks for responding. On 2/21/2022 11:34 PM, Laurent Pinchart wrote: > Hi Dmitry, > > On Tue, Feb 22, 2022 at 06:32:50AM +0300, Dmitry Baryshkov wrote: >> On Thu, 10 Feb 2022 at 07:59, Laurent Pinchart wrote: >>> On Wed, Feb 09, 2022 at 05:40:29PM -0800, Abhinav Kumar wrote: >>>> Hi Laurent >>>> >>>> Gentle reminder on this. >>> >>> I won't have time before next week I'm afraid. >> >> Laurent, another gentle ping. > > I'm really late on this so I probably deserve a bit of a rougher ping, > but thanks for being gentle :-) > >>>> On 2/6/2022 11:20 PM, Abhinav Kumar wrote: >>>>> On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote: >>>>>> On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart wrote: >>>>>>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: >>>>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: >>>>>>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: >>>>>>>>>> Changing rcar_du driver to accomadate the change of >>>>>>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder >>>>>>>>>> to a pointer the reason for which is explained in the >>>>>>>>>> Patch(drm: add writeback pointers to drm_connector). >>>>>>>>>> >>>>>>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> >>>>>>>>>> --- >>>>>>>>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ >>>>>>>>>> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- >>>>>>>>>> 2 files changed, 7 insertions(+), 3 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>>>>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>>>>>> index 66e8839db708..68f387a04502 100644 >>>>>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc { >>>>>>>>>> const char *const *sources; >>>>>>>>>> unsigned int sources_count; >>>>>>>>>> >>>>>>>>>> + struct drm_connector connector; >>>>>>>>>> + struct drm_encoder encoder; >>>>>>>>> >>>>>>>>> Those fields are, at best, poorly named. Furthermore, there's no need in >>>>>>>>> this driver or in other drivers using drm_writeback_connector to create >>>>>>>>> an encoder or connector manually. Let's not polute all drivers because >>>>>>>>> i915 doesn't have its abstractions right. >>>>>>>> >>>>>>>> i915 uses the quite common model for struct inheritance: >>>>>>>> >>>>>>>> struct intel_connector { >>>>>>>> struct drm_connector base; >>>>>>>> /* ... */ >>>>>>>> } >>>>>>>> >>>>>>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, >>>>>>>> radeon, tilcdc, and vboxvideo. >>>>>>>> >>>>>>>> We could argue about the relative merits of that abstraction, but I >>>>>>>> think the bottom line is that it's popular and the drivers using it are >>>>>>>> not going to be persuaded to move away from it. >>>>>>> >>>>>>> Nobody said inheritance is bad. >>>>>>> >>>>>>>> It's no coincidence that the drivers who've implemented writeback so far >>>>>>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, >>>>>>>> because the drm_writeback_connector midlayer does, forcing the issue. >>>>>>> >>>>>>> Are you sure it's not a coincidence ? :-) >>>>>>> >>>>>>> The encoder and especially connector created by drm_writeback_connector >>>>>>> are there only because KMS requires a drm_encoder and a drm_connector to >>>>>>> be exposed to userspace (and I could argue that using a connector for >>>>>>> writeback is a hack, but that won't change). The connector is "virtual", >>>>>>> I still fail to see why i915 or any other driver would need to wrap it >>>>>>> into something else. The whole point of the drm_writeback_connector >>>>>>> abstraction is that drivers do not have to manage the writeback >>>>>>> drm_connector manually, they shouldn't touch it at all. >>>>>> >>>>>> Laurent, I wanted to shift a bit from the question of drm_connector to >>>>>> the question of drm_encoder being embedded in the drm_writeback_connector. >>>>>> In case of the msm driver the drm_encoder is not a lightweight entity, >>>>>> but a full-featured driver part. Significant part of it can be shared >>>>>> with the writeback implementation, if we allow using a pointer to the >>>>>> external drm_encoder with the drm_writeback_connector. >>>>>> Does the following patch set stand a chance to receive your ack? >>>>>> - Switch drm_writeback_connector to point to drm_encoder rather than >>>>>> embedding it? >>>>>> - Create drm_encoder for the drm_writeback_connector when one is not >>>>>> specified, so the current drivers can be left unchanged. > > The situation is a bit different for the encoder indeed. > > The encoder concept is loosely defined nowadays, with more and more of > the "real" encoders being implemented as a drm_bridge. That's what I > usually recommend when reviewing new drivers. drm_encoder is slowly > becoming an empty shell (see for instance [1]), although that transition > is not enforced globally and will thus take a long time to complete (if > ever). > > This being said, lots of drivers have "featureful" encoder > implementations, and that won't go away any time soon. In those cases, I > could be OK with drivers optionally passing an encoder fo the writeback > helper if the hardware really shares resources between writeback and a > real encoder. I would however be careful there, as in many cases I would > expect the need to pass a custom encoder to originate from an old > software design decision rather than from the hardware architecture. In > those cases it would be best, I think, to move towards cleaning up the > software architecture, but that can be done step by step and I won't > consider that a requirement to implement writeback support. > > In the MSM case in particular, can you explain what resources are shared > between writeback and hardware encoder(s) ? > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > Yes, we indeed have a lot of functionality in our encoder. It shares both interrupt and clock control for all interfaces including writeback. Moreover, like I was mentioning earlier, on some of the chipsets where display hardware is limited, the hardware components mapped to a drm encoder can be shared between the panel and writeback paths. For your reference, please check [1] [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#n174 Hence we are requesting that drm_writeback not embed an encoder but acommodate a pointer to drm_encoder instead. >>>>> I second Dmitry's request here. For the reasons he has mentioned along >>>>> with the possibility of the writeback encoder being shared across >>>>> display pipelines, strengthens our request of the drm encoder being a >>>>> pointer inside the drm_writeback_connector instead of embedding it. >>>>> >>>>> Like I had shown in my RFC, in case the other drivers dont specify one, >>>>> we can allocate one: >>>>> >>>>> https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhinavk@quicinc.com/ >>>>> >>>>> We think this should be a reasonable accomodation to the existing >>>>> drm_writeback driver. >>>>> >>>>>>>> So I think drm_writeback_connector should *not* use the inheritance >>>>>>>> abstraction because it's a midlayer that should leave that option tothe >>>>>>>> drivers. I think drm_writeback_connector needs to be changed to >>>>>>>> accommodate that, and, unfortunately, it means current writeback users >>>>>>>> need to be changed as well. >>>>>>>> >>>>>>>> I am not sure, however, if the series at hand is the right >>>>>>>> approach. Perhaps writeback can be modified to allocate the stuff for >>>>>>>> you if you prefer it that way, as long as the drm_connector is not >>>>>>>> embedded in struct drm_writeback_connector. >>>>>>>> >>>>>>>>> Nack. >>>>>>>>> >>>>>>>>>> struct drm_writeback_connector writeback; >>>>>>>>>> }; >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >>>>>>>>>> index c79d1259e49b..5b1e83380c47 100644 >>>>>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >>>>>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c >>>>>>>>>> @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, >>>>>>>>>> { >>>>>>>>>> struct drm_writeback_connector *wb_conn = &rcrtc->writeback; >>>>>>>>>> >>>>>>>>>> - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); >>>>>>>>>> - drm_connector_helper_add(&wb_conn->base, >>>>>>>>>> + wb_conn->base = &rcrtc->connector; >>>>>>>>>> + wb_conn->encoder = &rcrtc->encoder; >>>>>>>>>> + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); >>>>>>>>>> + drm_connector_helper_add(wb_conn->base, >>>>>>>>>> &rcar_du_wb_conn_helper_funcs); >>>>>>>>>> >>>>>>>>>> return drm_writeback_connector_init(&rcdu->ddev, wb_conn, >>>>>>>>>> @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, >>>>>>>>>> struct drm_framebuffer *fb; >>>>>>>>>> unsigned int i; >>>>>>>>>> >>>>>>>>>> - state = rcrtc->writeback.base.state; >>>>>>>>>> + state = rcrtc->writeback.base->state; >>>>>>>>>> if (!state || !state->writeback_job) >>>>>>>>>> return; >>>>>>>>>> >
Hi Suraj On 2/22/2022 10:17 PM, Kandpal, Suraj wrote: > Hey, > >> The connector/encoder funcs you do have to pass to >> drm_writeback_connector_init() can't use any of the shared driver >> infrastructure that assume a certain inheritance. >> >> See also my reply to Laurent [1]. >> >>> It well might be that we all misunderstand your design. Do you have a >>> complete intel drm_writeback implementation based on this patchset? It >>> would be easier to judge if the approach is correct seeing your >>> target. >> >> That would be up to Suraj Kandpal. > I have floated the intel drm_writeback implementation. > Please refer to [1] to get a better understanding of how we are implementing > writeback functionality. > > [1] https://patchwork.freedesktop.org/series/100617/ > > Thanks, > Suraj Kandpal Based on the discussion in this thread [1] , it seems like having drm_encoder as a pointer seems to have merits for both of us and also in agreement with the folks on this thread and has a better chance of an ack. However drm_connector is not. I had a brief look at your implementation. Any reason you need to go through intel_connector here and not drm_writeback_connector directly? The reason I ask is that I see you are not using prepare_writeback_job hook. If you use that, you can use the drm_writeback_connector passed on from there instead of looping through your list like you are doing in intel_find_writeback_connector. Also, none of the other entries of struct intel_connector are being used for the writeback implementation. So does the drm_writeback_connector in your implementation need to be an intel_connector when its anyway not using other fields? Why cant it be just stored as a drm_writeback_connector itself in your struct intel_wd. @@ -539,6 +540,8 @@ struct intel_connector { struct work_struct modeset_retry_work; struct intel_hdcp hdcp; + + struct drm_writeback_connector wb_conn; }; [1] https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22261-6-suraj.kandpal@intel.com/#24747889 If you are in agreement with this, do you think you can re-spin the series only with the drm_encoder as a pointer without the drm_connector part.
Hi Abhinav, > Based on the discussion in this thread [1] , it seems like having drm_encoder > as a pointer seems to have merits for both of us and also in agreement with > the folks on this thread and has a better chance of an ack. > > However drm_connector is not. > > I had a brief look at your implementation. Any reason you need to go > through intel_connector here and not drm_writeback_connector directly? > > The reason I ask is that I see you are not using prepare_writeback_job hook. > If you use that, you can use the drm_writeback_connector passed on from > there instead of looping through your list like you are doing in > intel_find_writeback_connector. > > Also, none of the other entries of struct intel_connector are being used for > the writeback implementation. So does the drm_writeback_connector in > your implementation need to be an intel_connector when its anyway not > using other fields? Why cant it be just stored as a drm_writeback_connector > itself in your struct intel_wd. The reason we can't do that is i915 driver always assumes that all connectors present in device list is an intel connector and since drm_writeback_connector even though a faux connector in it's initialization calls drm_connector_init() and gets added to the drm device list and hence the i915 driver also expects a corresponding intel connector to go with it. In case I do try to make writeback connector standalone a lot of checks, a lot will have to be added all around the driver as there could be a chance that one of the drm_connector in the drm device list may not be an intel_connector. Yes right now not all fields of intel_connector are being used but we will be working on filling them as we add more functionality to writeback for example introducing content protection. So even if I do float the patch series with just drm_encoder as pointer it won't help us with our implementation if drm_connector isn't a pointer too. Regards, Suraj Kandpal
On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula <jani.nikula@intel.com> wrote: > > On Wed, 02 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jani, > > > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: > >> On Wed, 02 Feb 2022, Laurent Pinchart wrote: > >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > >> >> Changing rcar_du driver to accomadate the change of > >> >> drm_writeback_connector.base and drm_writeback_connector.encoder > >> >> to a pointer the reason for which is explained in the > >> >> Patch(drm: add writeback pointers to drm_connector). > >> >> > >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > >> >> --- > >> >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > >> >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > >> >> 2 files changed, 7 insertions(+), 3 deletions(-) > >> >> > >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> >> index 66e8839db708..68f387a04502 100644 > >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > >> >> const char *const *sources; > >> >> unsigned int sources_count; > >> >> > >> >> + struct drm_connector connector; > >> >> + struct drm_encoder encoder; > >> > > >> > Those fields are, at best, poorly named. Furthermore, there's no need in > >> > this driver or in other drivers using drm_writeback_connector to create > >> > an encoder or connector manually. Let's not polute all drivers because > >> > i915 doesn't have its abstractions right. > >> > >> i915 uses the quite common model for struct inheritance: > >> > >> struct intel_connector { > >> struct drm_connector base; > >> /* ... */ > >> } > >> > >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > >> radeon, tilcdc, and vboxvideo. > >> > >> We could argue about the relative merits of that abstraction, but I > >> think the bottom line is that it's popular and the drivers using it are > >> not going to be persuaded to move away from it. > > > > Nobody said inheritance is bad. > > > >> It's no coincidence that the drivers who've implemented writeback so far > >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > >> because the drm_writeback_connector midlayer does, forcing the issue. > > > > Are you sure it's not a coincidence ? :-) > > > > The encoder and especially connector created by drm_writeback_connector > > are there only because KMS requires a drm_encoder and a drm_connector to > > be exposed to userspace (and I could argue that using a connector for > > writeback is a hack, but that won't change). The connector is "virtual", > > I still fail to see why i915 or any other driver would need to wrap it > > into something else. The whole point of the drm_writeback_connector > > abstraction is that drivers do not have to manage the writeback > > drm_connector manually, they shouldn't touch it at all. > > The thing is, drm_writeback_connector_init() calling > drm_connector_init() on the drm_connector embedded in > drm_writeback_connector leads to that connector being added to the > drm_device's list of connectors. Ditto for the encoder. > > All the driver code that handles drm_connectors would need to take into > account they might not be embedded in intel_connector. Throughout the > driver. Ditto for the encoders. The assumption that a connector is embedded in intel_connector doesn't really play that well with how bridge and panel connectors work.. so in general this seems like a good thing to unwind. But as a point of practicality, i915 is a large driver covering a lot of generations of hw with a lot of users. So I can understand changing this design isn't something that can happen quickly or easily. IMO we should allow i915 to create it's own connector for writeback, and just document clearly that this isn't the approach new drivers should take. I mean, I understand idealism, but sometimes a dose of pragmatism is needed. :-) BR, -R > The point is, you can't initialize a connector or an encoder for a > drm_device in isolation of the rest of the driver, even if it were > supposed to be hidden away. > > BR, > Jani. >
Hi Suraj, On Sat, Feb 26, 2022 at 05:10:06AM +0000, Kandpal, Suraj wrote: > Hi Abhinav, > > > Based on the discussion in this thread [1] , it seems like having drm_encoder > > as a pointer seems to have merits for both of us and also in agreement with > > the folks on this thread and has a better chance of an ack. > > > > However drm_connector is not. > > > > I had a brief look at your implementation. Any reason you need to go > > through intel_connector here and not drm_writeback_connector directly? > > > > The reason I ask is that I see you are not using prepare_writeback_job hook. > > If you use that, you can use the drm_writeback_connector passed on from > > there instead of looping through your list like you are doing in > > intel_find_writeback_connector. > > > > Also, none of the other entries of struct intel_connector are being used for > > the writeback implementation. So does the drm_writeback_connector in > > your implementation need to be an intel_connector when its anyway not > > using other fields? Why cant it be just stored as a drm_writeback_connector > > itself in your struct intel_wd. > > The reason we can't do that is i915 driver always assumes that all connectors > present in device list is an intel connector and since drm_writeback_connector > even though a faux connector in it's initialization calls drm_connector_init() > and gets added to the drm device list and hence the i915 driver also expects > a corresponding intel connector to go with it. In case I do try to make writeback > connector standalone a lot of checks, a lot will have to be added all around the > driver as there could be a chance that one of the drm_connector in the drm device > list may not be an intel_connector. > Yes right now not all fields of intel_connector are being used but we will be working > on filling them as we add more functionality to writeback for example introducing > content protection. > So even if I do float the patch series with just drm_encoder as pointer it won't help > us with our implementation if drm_connector isn't a pointer too. This is a direct consequence of the decision to use connectors for writeback in the userspace API. This disrupts any code that assumes that a connector is a connector. The problem isn't limited to kernelspace, userspace has the same exact problem, which resulted in a hack to avoid breaking everything. Userspace software that needs to deal with writeback needs to set the DRM_CLIENT_CAP_WRITEBACK_CONNECTORS capability to get the writeback connectors exposed by the kernel, but more than that, a large refactoring is then often needed to chase all code paths that assume a connector is a connector. I'm afraid the same applies to the kernel. Drivers that don't use writeback are largely unaffected. Drievrs that want to use writeback need to be refactored to properly handle the fact that writeback connectors are special. i915 will need to go that way.
Hi Rob, On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote: > On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote: > > On Wed, 02 Feb 2022, Laurent Pinchart wrote: > > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: > > >> On Wed, 02 Feb 2022, Laurent Pinchart wrote: > > >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > > >> >> Changing rcar_du driver to accomadate the change of > > >> >> drm_writeback_connector.base and drm_writeback_connector.encoder > > >> >> to a pointer the reason for which is explained in the > > >> >> Patch(drm: add writeback pointers to drm_connector). > > >> >> > > >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > > >> >> --- > > >> >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > > >> >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > > >> >> 2 files changed, 7 insertions(+), 3 deletions(-) > > >> >> > > >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >> >> index 66e8839db708..68f387a04502 100644 > > >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > > >> >> const char *const *sources; > > >> >> unsigned int sources_count; > > >> >> > > >> >> + struct drm_connector connector; > > >> >> + struct drm_encoder encoder; > > >> > > > >> > Those fields are, at best, poorly named. Furthermore, there's no need in > > >> > this driver or in other drivers using drm_writeback_connector to create > > >> > an encoder or connector manually. Let's not polute all drivers because > > >> > i915 doesn't have its abstractions right. > > >> > > >> i915 uses the quite common model for struct inheritance: > > >> > > >> struct intel_connector { > > >> struct drm_connector base; > > >> /* ... */ > > >> } > > >> > > >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > > >> radeon, tilcdc, and vboxvideo. > > >> > > >> We could argue about the relative merits of that abstraction, but I > > >> think the bottom line is that it's popular and the drivers using it are > > >> not going to be persuaded to move away from it. > > > > > > Nobody said inheritance is bad. > > > > > >> It's no coincidence that the drivers who've implemented writeback so far > > >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > > >> because the drm_writeback_connector midlayer does, forcing the issue. > > > > > > Are you sure it's not a coincidence ? :-) > > > > > > The encoder and especially connector created by drm_writeback_connector > > > are there only because KMS requires a drm_encoder and a drm_connector to > > > be exposed to userspace (and I could argue that using a connector for > > > writeback is a hack, but that won't change). The connector is "virtual", > > > I still fail to see why i915 or any other driver would need to wrap it > > > into something else. The whole point of the drm_writeback_connector > > > abstraction is that drivers do not have to manage the writeback > > > drm_connector manually, they shouldn't touch it at all. > > > > The thing is, drm_writeback_connector_init() calling > > drm_connector_init() on the drm_connector embedded in > > drm_writeback_connector leads to that connector being added to the > > drm_device's list of connectors. Ditto for the encoder. > > > > All the driver code that handles drm_connectors would need to take into > > account they might not be embedded in intel_connector. Throughout the > > driver. Ditto for the encoders. > > The assumption that a connector is embedded in intel_connector doesn't > really play that well with how bridge and panel connectors work.. so > in general this seems like a good thing to unwind. > > But as a point of practicality, i915 is a large driver covering a lot > of generations of hw with a lot of users. So I can understand > changing this design isn't something that can happen quickly or > easily. IMO we should allow i915 to create it's own connector for > writeback, and just document clearly that this isn't the approach new > drivers should take. I mean, I understand idealism, but sometimes a > dose of pragmatism is needed. :-) i915 is big, but so is Intel. It's not fair to treat everybody else as a second class citizen and let Intel get away without doing its homework. I want to see this refactoring effort moving forward in i915 (and moving to drm_bridge would then be a good idea too). If writeback support in i915 urgent, then we can discuss *temporary* pragmatic stopgap measures, but not without a real effort to fix the core issue. > > The point is, you can't initialize a connector or an encoder for a > > drm_device in isolation of the rest of the driver, even if it were > > supposed to be hidden away.
On Mon, 28 Feb 2022 at 11:00, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Suraj, > > On Sat, Feb 26, 2022 at 05:10:06AM +0000, Kandpal, Suraj wrote: > > Hi Abhinav, > > > > > Based on the discussion in this thread [1] , it seems like having drm_encoder > > > as a pointer seems to have merits for both of us and also in agreement with > > > the folks on this thread and has a better chance of an ack. > > > > > > However drm_connector is not. > > > > > > I had a brief look at your implementation. Any reason you need to go > > > through intel_connector here and not drm_writeback_connector directly? > > > > > > The reason I ask is that I see you are not using prepare_writeback_job hook. > > > If you use that, you can use the drm_writeback_connector passed on from > > > there instead of looping through your list like you are doing in > > > intel_find_writeback_connector. > > > > > > Also, none of the other entries of struct intel_connector are being used for > > > the writeback implementation. So does the drm_writeback_connector in > > > your implementation need to be an intel_connector when its anyway not > > > using other fields? Why cant it be just stored as a drm_writeback_connector > > > itself in your struct intel_wd. > > > > The reason we can't do that is i915 driver always assumes that all connectors > > present in device list is an intel connector and since drm_writeback_connector > > even though a faux connector in it's initialization calls drm_connector_init() > > and gets added to the drm device list and hence the i915 driver also expects > > a corresponding intel connector to go with it. In case I do try to make writeback > > connector standalone a lot of checks, a lot will have to be added all around the > > driver as there could be a chance that one of the drm_connector in the drm device > > list may not be an intel_connector. > > Yes right now not all fields of intel_connector are being used but we will be working > > on filling them as we add more functionality to writeback for example introducing > > content protection. > > So even if I do float the patch series with just drm_encoder as pointer it won't help > > us with our implementation if drm_connector isn't a pointer too. > > This is a direct consequence of the decision to use connectors for > writeback in the userspace API. This disrupts any code that assumes that > a connector is a connector. The problem isn't limited to kernelspace, > userspace has the same exact problem, which resulted in a hack to avoid > breaking everything. Userspace software that needs to deal with > writeback needs to set the DRM_CLIENT_CAP_WRITEBACK_CONNECTORS > capability to get the writeback connectors exposed by the kernel, but > more than that, a large refactoring is then often needed to chase all > code paths that assume a connector is a connector. > > I'm afraid the same applies to the kernel. Drivers that don't use > writeback are largely unaffected. Drievrs that want to use writeback > need to be refactored to properly handle the fact that writeback > connectors are special. i915 will need to go that way. Laurent, you have frown upon the idea of separating the connector from the drm_writeback_connector struct. What about the encoder? The msm code in question can be found at the patchwork: https://patchwork.freedesktop.org/series/99724/. This series depends on Intel's patch, but should give you the overall feeling of the code being shared between to-the-display and writeback pipelines.
Hi Dmitry, On Mon, Feb 28, 2022 at 11:07:41AM +0300, Dmitry Baryshkov wrote: > On Mon, 28 Feb 2022 at 11:00, Laurent Pinchart wrote: > > On Sat, Feb 26, 2022 at 05:10:06AM +0000, Kandpal, Suraj wrote: > > > Hi Abhinav, > > > > > > > Based on the discussion in this thread [1] , it seems like having drm_encoder > > > > as a pointer seems to have merits for both of us and also in agreement with > > > > the folks on this thread and has a better chance of an ack. > > > > > > > > However drm_connector is not. > > > > > > > > I had a brief look at your implementation. Any reason you need to go > > > > through intel_connector here and not drm_writeback_connector directly? > > > > > > > > The reason I ask is that I see you are not using prepare_writeback_job hook. > > > > If you use that, you can use the drm_writeback_connector passed on from > > > > there instead of looping through your list like you are doing in > > > > intel_find_writeback_connector. > > > > > > > > Also, none of the other entries of struct intel_connector are being used for > > > > the writeback implementation. So does the drm_writeback_connector in > > > > your implementation need to be an intel_connector when its anyway not > > > > using other fields? Why cant it be just stored as a drm_writeback_connector > > > > itself in your struct intel_wd. > > > > > > The reason we can't do that is i915 driver always assumes that all connectors > > > present in device list is an intel connector and since drm_writeback_connector > > > even though a faux connector in it's initialization calls drm_connector_init() > > > and gets added to the drm device list and hence the i915 driver also expects > > > a corresponding intel connector to go with it. In case I do try to make writeback > > > connector standalone a lot of checks, a lot will have to be added all around the > > > driver as there could be a chance that one of the drm_connector in the drm device > > > list may not be an intel_connector. > > > Yes right now not all fields of intel_connector are being used but we will be working > > > on filling them as we add more functionality to writeback for example introducing > > > content protection. > > > So even if I do float the patch series with just drm_encoder as pointer it won't help > > > us with our implementation if drm_connector isn't a pointer too. > > > > This is a direct consequence of the decision to use connectors for > > writeback in the userspace API. This disrupts any code that assumes that > > a connector is a connector. The problem isn't limited to kernelspace, > > userspace has the same exact problem, which resulted in a hack to avoid > > breaking everything. Userspace software that needs to deal with > > writeback needs to set the DRM_CLIENT_CAP_WRITEBACK_CONNECTORS > > capability to get the writeback connectors exposed by the kernel, but > > more than that, a large refactoring is then often needed to chase all > > code paths that assume a connector is a connector. > > > > I'm afraid the same applies to the kernel. Drivers that don't use > > writeback are largely unaffected. Drievrs that want to use writeback > > need to be refactored to properly handle the fact that writeback > > connectors are special. i915 will need to go that way. > > Laurent, you have frown upon the idea of separating the connector from > the drm_writeback_connector struct. What about the encoder? > The msm code in question can be found at the patchwork: > https://patchwork.freedesktop.org/series/99724/. This series depends > on Intel's patch, but should give you the overall feeling of the code > being shared between to-the-display and writeback pipelines. I'm not too fond of separating the encoder either as explained separately in this mail thread, but I won't block that as it's even more difficult to avoid today. drm_encoder is a bit of a historical mistake that we need to keep around because it's exposed to userspace. With drivers more and more reliant on drm_bridge, drm_encoder is less meaningful than it used to be. I would like to see the subsystem continuing in that direction, with drm_encoder becoming an empty shell. Drivers should decouple the CRTC outputs from the drm_encoder object, likely creating driver-specific structures to model a CRTC output (which is largely what the driver-specific subclasses of drm_encoder do today), and create drm_encoder instances only for the purpose of exposing the display topology to userspace. Longer term I can even imagine having a different way to expose the display topology to userspace, without drm_encoder but with objects that will be allowed to support more complex topologies that the CRTC + encoder + connector abstraction can't model. Later :-)
On Mon, 28 Feb 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Rob, > > On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote: >> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote: >> > On Wed, 02 Feb 2022, Laurent Pinchart wrote: >> > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: >> > >> On Wed, 02 Feb 2022, Laurent Pinchart wrote: >> > >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: >> > >> >> Changing rcar_du driver to accomadate the change of >> > >> >> drm_writeback_connector.base and drm_writeback_connector.encoder >> > >> >> to a pointer the reason for which is explained in the >> > >> >> Patch(drm: add writeback pointers to drm_connector). >> > >> >> >> > >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> >> > >> >> --- >> > >> >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ >> > >> >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- >> > >> >> 2 files changed, 7 insertions(+), 3 deletions(-) >> > >> >> >> > >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> > >> >> index 66e8839db708..68f387a04502 100644 >> > >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> > >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >> > >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { >> > >> >> const char *const *sources; >> > >> >> unsigned int sources_count; >> > >> >> >> > >> >> + struct drm_connector connector; >> > >> >> + struct drm_encoder encoder; >> > >> > >> > >> > Those fields are, at best, poorly named. Furthermore, there's no need in >> > >> > this driver or in other drivers using drm_writeback_connector to create >> > >> > an encoder or connector manually. Let's not polute all drivers because >> > >> > i915 doesn't have its abstractions right. >> > >> >> > >> i915 uses the quite common model for struct inheritance: >> > >> >> > >> struct intel_connector { >> > >> struct drm_connector base; >> > >> /* ... */ >> > >> } >> > >> >> > >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, >> > >> radeon, tilcdc, and vboxvideo. >> > >> >> > >> We could argue about the relative merits of that abstraction, but I >> > >> think the bottom line is that it's popular and the drivers using it are >> > >> not going to be persuaded to move away from it. >> > > >> > > Nobody said inheritance is bad. >> > > >> > >> It's no coincidence that the drivers who've implemented writeback so far >> > >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, >> > >> because the drm_writeback_connector midlayer does, forcing the issue. >> > > >> > > Are you sure it's not a coincidence ? :-) >> > > >> > > The encoder and especially connector created by drm_writeback_connector >> > > are there only because KMS requires a drm_encoder and a drm_connector to >> > > be exposed to userspace (and I could argue that using a connector for >> > > writeback is a hack, but that won't change). The connector is "virtual", >> > > I still fail to see why i915 or any other driver would need to wrap it >> > > into something else. The whole point of the drm_writeback_connector >> > > abstraction is that drivers do not have to manage the writeback >> > > drm_connector manually, they shouldn't touch it at all. >> > >> > The thing is, drm_writeback_connector_init() calling >> > drm_connector_init() on the drm_connector embedded in >> > drm_writeback_connector leads to that connector being added to the >> > drm_device's list of connectors. Ditto for the encoder. >> > >> > All the driver code that handles drm_connectors would need to take into >> > account they might not be embedded in intel_connector. Throughout the >> > driver. Ditto for the encoders. >> >> The assumption that a connector is embedded in intel_connector doesn't >> really play that well with how bridge and panel connectors work.. so >> in general this seems like a good thing to unwind. >> >> But as a point of practicality, i915 is a large driver covering a lot >> of generations of hw with a lot of users. So I can understand >> changing this design isn't something that can happen quickly or >> easily. IMO we should allow i915 to create it's own connector for >> writeback, and just document clearly that this isn't the approach new >> drivers should take. I mean, I understand idealism, but sometimes a >> dose of pragmatism is needed. :-) > > i915 is big, but so is Intel. It's not fair to treat everybody else as a > second class citizen and let Intel get away without doing its homework. Laurent, as you accuse us of not doing our homework, I'll point out that we've been embedding drm crtc, encoder and connector ever since modesetting support was added to i915 in 2008, since before *any* of the things you now use as a rationale for asking us to do a massive rewrite of the driver existed. It's been ok to embed those structures for well over ten years. It's a common pattern, basically throughout the kernel. Other drivers do it too, not just i915. There hasn't been the slightest hint this should not be done until this very conversation. > I want to see this refactoring effort moving forward in i915 (and moving > to drm_bridge would then be a good idea too). If writeback support in > i915 urgent, then we can discuss *temporary* pragmatic stopgap measures, > but not without a real effort to fix the core issue. I think the onus is on you to first convince everyone that embedding the drm core kms structures is an antipattern that all drivers, not just i915, should stop using. In OO terms, you're saying they are classes that should be final and not extended. And even then, to be totally honest, refactoring the structures is not going to be anywhere near the top of our list of things to do, for a very long time. BR, Jani. > >> > The point is, you can't initialize a connector or an encoder for a >> > drm_device in isolation of the rest of the driver, even if it were >> > supposed to be hidden away.
Hi Jani, On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote: > On Mon, 28 Feb 2022, Laurent Pinchart wrote: > > On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote: > >> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote: > >> > On Wed, 02 Feb 2022, Laurent Pinchart wrote: > >> > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: > >> > >> On Wed, 02 Feb 2022, Laurent Pinchart wrote: > >> > >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > >> > >> >> Changing rcar_du driver to accomadate the change of > >> > >> >> drm_writeback_connector.base and drm_writeback_connector.encoder > >> > >> >> to a pointer the reason for which is explained in the > >> > >> >> Patch(drm: add writeback pointers to drm_connector). > >> > >> >> > >> > >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > >> > >> >> --- > >> > >> >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > >> > >> >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > >> > >> >> 2 files changed, 7 insertions(+), 3 deletions(-) > >> > >> >> > >> > >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> > >> >> index 66e8839db708..68f387a04502 100644 > >> > >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> > >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >> > >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > >> > >> >> const char *const *sources; > >> > >> >> unsigned int sources_count; > >> > >> >> > >> > >> >> + struct drm_connector connector; > >> > >> >> + struct drm_encoder encoder; > >> > >> > > >> > >> > Those fields are, at best, poorly named. Furthermore, there's no need in > >> > >> > this driver or in other drivers using drm_writeback_connector to create > >> > >> > an encoder or connector manually. Let's not polute all drivers because > >> > >> > i915 doesn't have its abstractions right. > >> > >> > >> > >> i915 uses the quite common model for struct inheritance: > >> > >> > >> > >> struct intel_connector { > >> > >> struct drm_connector base; > >> > >> /* ... */ > >> > >> } > >> > >> > >> > >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > >> > >> radeon, tilcdc, and vboxvideo. > >> > >> > >> > >> We could argue about the relative merits of that abstraction, but I > >> > >> think the bottom line is that it's popular and the drivers using it are > >> > >> not going to be persuaded to move away from it. > >> > > > >> > > Nobody said inheritance is bad. > >> > > > >> > >> It's no coincidence that the drivers who've implemented writeback so far > >> > >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > >> > >> because the drm_writeback_connector midlayer does, forcing the issue. > >> > > > >> > > Are you sure it's not a coincidence ? :-) > >> > > > >> > > The encoder and especially connector created by drm_writeback_connector > >> > > are there only because KMS requires a drm_encoder and a drm_connector to > >> > > be exposed to userspace (and I could argue that using a connector for > >> > > writeback is a hack, but that won't change). The connector is "virtual", > >> > > I still fail to see why i915 or any other driver would need to wrap it > >> > > into something else. The whole point of the drm_writeback_connector > >> > > abstraction is that drivers do not have to manage the writeback > >> > > drm_connector manually, they shouldn't touch it at all. > >> > > >> > The thing is, drm_writeback_connector_init() calling > >> > drm_connector_init() on the drm_connector embedded in > >> > drm_writeback_connector leads to that connector being added to the > >> > drm_device's list of connectors. Ditto for the encoder. > >> > > >> > All the driver code that handles drm_connectors would need to take into > >> > account they might not be embedded in intel_connector. Throughout the > >> > driver. Ditto for the encoders. > >> > >> The assumption that a connector is embedded in intel_connector doesn't > >> really play that well with how bridge and panel connectors work.. so > >> in general this seems like a good thing to unwind. > >> > >> But as a point of practicality, i915 is a large driver covering a lot > >> of generations of hw with a lot of users. So I can understand > >> changing this design isn't something that can happen quickly or > >> easily. IMO we should allow i915 to create it's own connector for > >> writeback, and just document clearly that this isn't the approach new > >> drivers should take. I mean, I understand idealism, but sometimes a > >> dose of pragmatism is needed. :-) > > > > i915 is big, but so is Intel. It's not fair to treat everybody else as a > > second class citizen and let Intel get away without doing its homework. > > Laurent, as you accuse us of not doing our homework, I'll point out that > we've been embedding drm crtc, encoder and connector ever since > modesetting support was added to i915 in 2008, since before *any* of the > things you now use as a rationale for asking us to do a massive rewrite > of the driver existed. > > It's been ok to embed those structures for well over ten years. It's a > common pattern, basically throughout the kernel. Other drivers do it > too, not just i915. There hasn't been the slightest hint this should not > be done until this very conversation. > > > I want to see this refactoring effort moving forward in i915 (and moving > > to drm_bridge would then be a good idea too). If writeback support in > > i915 urgent, then we can discuss *temporary* pragmatic stopgap measures, > > but not without a real effort to fix the core issue. > > I think the onus is on you to first convince everyone that embedding the > drm core kms structures is an antipattern that all drivers, not just > i915, should stop using. In OO terms, you're saying they are classes > that should be final and not extended. > > And even then, to be totally honest, refactoring the structures is not > going to be anywhere near the top of our list of things to do, for a > very long time. I may have not expressed myself correctly. There's nothing wrong as such in embedded those structures in driver-specific structures (a.k.a. C inheritance). That doesn't need to change (albeit for drm_encoder I think we should move away from that pattern, but that's an entirely different issue, and nothing that needs to be addressed soonà. The issue here is assuming that every drm_connector instance can be up-casted to an i915-specific structure. > >> > The point is, you can't initialize a connector or an encoder for a > >> > drm_device in isolation of the rest of the driver, even if it were > >> > supposed to be hidden away.
Hello, On Mon, Feb 28, 2022 at 02:28:27PM +0200, Laurent Pinchart wrote: > On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote: > > On Mon, 28 Feb 2022, Laurent Pinchart wrote: > > > On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote: > > >> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote: > > >> > On Wed, 02 Feb 2022, Laurent Pinchart wrote: > > >> > > On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: > > >> > >> On Wed, 02 Feb 2022, Laurent Pinchart wrote: > > >> > >> > On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > > >> > >> >> Changing rcar_du driver to accomadate the change of > > >> > >> >> drm_writeback_connector.base and drm_writeback_connector.encoder > > >> > >> >> to a pointer the reason for which is explained in the > > >> > >> >> Patch(drm: add writeback pointers to drm_connector). > > >> > >> >> > > >> > >> >> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > > >> > >> >> --- > > >> > >> >> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > > >> > >> >> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > > >> > >> >> 2 files changed, 7 insertions(+), 3 deletions(-) > > >> > >> >> > > >> > >> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >> > >> >> index 66e8839db708..68f387a04502 100644 > > >> > >> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >> > >> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > > >> > >> >> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > > >> > >> >> const char *const *sources; > > >> > >> >> unsigned int sources_count; > > >> > >> >> > > >> > >> >> + struct drm_connector connector; > > >> > >> >> + struct drm_encoder encoder; > > >> > >> > > > >> > >> > Those fields are, at best, poorly named. Furthermore, there's no need in > > >> > >> > this driver or in other drivers using drm_writeback_connector to create > > >> > >> > an encoder or connector manually. Let's not polute all drivers because > > >> > >> > i915 doesn't have its abstractions right. > > >> > >> > > >> > >> i915 uses the quite common model for struct inheritance: > > >> > >> > > >> > >> struct intel_connector { > > >> > >> struct drm_connector base; > > >> > >> /* ... */ > > >> > >> } > > >> > >> > > >> > >> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > > >> > >> radeon, tilcdc, and vboxvideo. > > >> > >> > > >> > >> We could argue about the relative merits of that abstraction, but I > > >> > >> think the bottom line is that it's popular and the drivers using it are > > >> > >> not going to be persuaded to move away from it. > > >> > > > > >> > > Nobody said inheritance is bad. > > >> > > > > >> > >> It's no coincidence that the drivers who've implemented writeback so far > > >> > >> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > > >> > >> because the drm_writeback_connector midlayer does, forcing the issue. > > >> > > > > >> > > Are you sure it's not a coincidence ? :-) > > >> > > > > >> > > The encoder and especially connector created by drm_writeback_connector > > >> > > are there only because KMS requires a drm_encoder and a drm_connector to > > >> > > be exposed to userspace (and I could argue that using a connector for > > >> > > writeback is a hack, but that won't change). The connector is "virtual", > > >> > > I still fail to see why i915 or any other driver would need to wrap it > > >> > > into something else. The whole point of the drm_writeback_connector > > >> > > abstraction is that drivers do not have to manage the writeback > > >> > > drm_connector manually, they shouldn't touch it at all. > > >> > > > >> > The thing is, drm_writeback_connector_init() calling > > >> > drm_connector_init() on the drm_connector embedded in > > >> > drm_writeback_connector leads to that connector being added to the > > >> > drm_device's list of connectors. Ditto for the encoder. > > >> > > > >> > All the driver code that handles drm_connectors would need to take into > > >> > account they might not be embedded in intel_connector. Throughout the > > >> > driver. Ditto for the encoders. > > >> > > >> The assumption that a connector is embedded in intel_connector doesn't > > >> really play that well with how bridge and panel connectors work.. so > > >> in general this seems like a good thing to unwind. > > >> > > >> But as a point of practicality, i915 is a large driver covering a lot > > >> of generations of hw with a lot of users. So I can understand > > >> changing this design isn't something that can happen quickly or > > >> easily. IMO we should allow i915 to create it's own connector for > > >> writeback, and just document clearly that this isn't the approach new > > >> drivers should take. I mean, I understand idealism, but sometimes a > > >> dose of pragmatism is needed. :-) > > > > > > i915 is big, but so is Intel. It's not fair to treat everybody else as a > > > second class citizen and let Intel get away without doing its homework. > > > > Laurent, as you accuse us of not doing our homework, I'll point out that > > we've been embedding drm crtc, encoder and connector ever since > > modesetting support was added to i915 in 2008, since before *any* of the > > things you now use as a rationale for asking us to do a massive rewrite > > of the driver existed. > > > > It's been ok to embed those structures for well over ten years. It's a > > common pattern, basically throughout the kernel. Other drivers do it > > too, not just i915. There hasn't been the slightest hint this should not > > be done until this very conversation. > > > > > I want to see this refactoring effort moving forward in i915 (and moving > > > to drm_bridge would then be a good idea too). If writeback support in > > > i915 urgent, then we can discuss *temporary* pragmatic stopgap measures, > > > but not without a real effort to fix the core issue. > > > > I think the onus is on you to first convince everyone that embedding the > > drm core kms structures is an antipattern that all drivers, not just > > i915, should stop using. In OO terms, you're saying they are classes > > that should be final and not extended. > > > > And even then, to be totally honest, refactoring the structures is not > > going to be anywhere near the top of our list of things to do, for a > > very long time. > > I may have not expressed myself correctly. There's nothing wrong as such > in embedded those structures in driver-specific structures (a.k.a. C > inheritance). That doesn't need to change (albeit for drm_encoder I > think we should move away from that pattern, but that's an entirely > different issue, and nothing that needs to be addressed soonà. > > The issue here is assuming that every drm_connector instance can be > up-casted to an i915-specific structure. Thinking some more about this, I wonder a way forward could be to drop the writeback connectors from the connectors list, or at least make them invisible to drivers. The connectors list is used extensively for two different purposes: tracking all drm_connector instances, and tracking all real connectors. The former is mostly needed by the DRM core for bookkeeping purposes and to expose all drm_connector instances to userspace, while the latter is also used by drivers, in many cases in locations that don't expect writeback connectors. Using a drm_connector to implement writeback isn't something we can revisit, but we could avoid exposing that to drivers by considering "real" connectors and writeback connectors two different types of entities in the APIs the DRM core exposes to drivers. What do you think, would it help for i915 ? > > >> > The point is, you can't initialize a connector or an encoder for a > > >> > drm_device in isolation of the rest of the driver, even if it were > > >> > supposed to be hidden away.
On 2/28/2022 5:42 AM, Laurent Pinchart wrote: > Hello, > > On Mon, Feb 28, 2022 at 02:28:27PM +0200, Laurent Pinchart wrote: >> On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote: >>> On Mon, 28 Feb 2022, Laurent Pinchart wrote: >>>> On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote: >>>>> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote: >>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: >>>>>>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: >>>>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: >>>>>>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: >>>>>>>>>> Changing rcar_du driver to accomadate the change of >>>>>>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder >>>>>>>>>> to a pointer the reason for which is explained in the >>>>>>>>>> Patch(drm: add writeback pointers to drm_connector). >>>>>>>>>> >>>>>>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> >>>>>>>>>> --- >>>>>>>>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ >>>>>>>>>> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- >>>>>>>>>> 2 files changed, 7 insertions(+), 3 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>>>>>> index 66e8839db708..68f387a04502 100644 >>>>>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc { >>>>>>>>>> const char *const *sources; >>>>>>>>>> unsigned int sources_count; >>>>>>>>>> >>>>>>>>>> + struct drm_connector connector; >>>>>>>>>> + struct drm_encoder encoder; >>>>>>>>> >>>>>>>>> Those fields are, at best, poorly named. Furthermore, there's no need in >>>>>>>>> this driver or in other drivers using drm_writeback_connector to create >>>>>>>>> an encoder or connector manually. Let's not polute all drivers because >>>>>>>>> i915 doesn't have its abstractions right. >>>>>>>> >>>>>>>> i915 uses the quite common model for struct inheritance: >>>>>>>> >>>>>>>> struct intel_connector { >>>>>>>> struct drm_connector base; >>>>>>>> /* ... */ >>>>>>>> } >>>>>>>> >>>>>>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, >>>>>>>> radeon, tilcdc, and vboxvideo. >>>>>>>> >>>>>>>> We could argue about the relative merits of that abstraction, but I >>>>>>>> think the bottom line is that it's popular and the drivers using it are >>>>>>>> not going to be persuaded to move away from it. >>>>>>> >>>>>>> Nobody said inheritance is bad. >>>>>>> >>>>>>>> It's no coincidence that the drivers who've implemented writeback so far >>>>>>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, >>>>>>>> because the drm_writeback_connector midlayer does, forcing the issue. >>>>>>> >>>>>>> Are you sure it's not a coincidence ? :-) >>>>>>> >>>>>>> The encoder and especially connector created by drm_writeback_connector >>>>>>> are there only because KMS requires a drm_encoder and a drm_connector to >>>>>>> be exposed to userspace (and I could argue that using a connector for >>>>>>> writeback is a hack, but that won't change). The connector is "virtual", >>>>>>> I still fail to see why i915 or any other driver would need to wrap it >>>>>>> into something else. The whole point of the drm_writeback_connector >>>>>>> abstraction is that drivers do not have to manage the writeback >>>>>>> drm_connector manually, they shouldn't touch it at all. >>>>>> >>>>>> The thing is, drm_writeback_connector_init() calling >>>>>> drm_connector_init() on the drm_connector embedded in >>>>>> drm_writeback_connector leads to that connector being added to the >>>>>> drm_device's list of connectors. Ditto for the encoder. >>>>>> >>>>>> All the driver code that handles drm_connectors would need to take into >>>>>> account they might not be embedded in intel_connector. Throughout the >>>>>> driver. Ditto for the encoders. >>>>> >>>>> The assumption that a connector is embedded in intel_connector doesn't >>>>> really play that well with how bridge and panel connectors work.. so >>>>> in general this seems like a good thing to unwind. >>>>> >>>>> But as a point of practicality, i915 is a large driver covering a lot >>>>> of generations of hw with a lot of users. So I can understand >>>>> changing this design isn't something that can happen quickly or >>>>> easily. IMO we should allow i915 to create it's own connector for >>>>> writeback, and just document clearly that this isn't the approach new >>>>> drivers should take. I mean, I understand idealism, but sometimes a >>>>> dose of pragmatism is needed. :-) >>>> >>>> i915 is big, but so is Intel. It's not fair to treat everybody else as a >>>> second class citizen and let Intel get away without doing its homework. >>> >>> Laurent, as you accuse us of not doing our homework, I'll point out that >>> we've been embedding drm crtc, encoder and connector ever since >>> modesetting support was added to i915 in 2008, since before *any* of the >>> things you now use as a rationale for asking us to do a massive rewrite >>> of the driver existed. >>> >>> It's been ok to embed those structures for well over ten years. It's a >>> common pattern, basically throughout the kernel. Other drivers do it >>> too, not just i915. There hasn't been the slightest hint this should not >>> be done until this very conversation. >>> >>>> I want to see this refactoring effort moving forward in i915 (and moving >>>> to drm_bridge would then be a good idea too). If writeback support in >>>> i915 urgent, then we can discuss *temporary* pragmatic stopgap measures, >>>> but not without a real effort to fix the core issue. >>> >>> I think the onus is on you to first convince everyone that embedding the >>> drm core kms structures is an antipattern that all drivers, not just >>> i915, should stop using. In OO terms, you're saying they are classes >>> that should be final and not extended. >>> >>> And even then, to be totally honest, refactoring the structures is not >>> going to be anywhere near the top of our list of things to do, for a >>> very long time. >> >> I may have not expressed myself correctly. There's nothing wrong as such >> in embedded those structures in driver-specific structures (a.k.a. C >> inheritance). That doesn't need to change (albeit for drm_encoder I >> think we should move away from that pattern, but that's an entirely >> different issue, and nothing that needs to be addressed soonà. >> >> The issue here is assuming that every drm_connector instance can be >> up-casted to an i915-specific structure. > > Thinking some more about this, I wonder a way forward could be to drop > the writeback connectors from the connectors list, or at least make them > invisible to drivers. The connectors list is used extensively for two > different purposes: tracking all drm_connector instances, and tracking > all real connectors. The former is mostly needed by the DRM core for > bookkeeping purposes and to expose all drm_connector instances to > userspace, while the latter is also used by drivers, in many cases in > locations that don't expect writeback connectors. Using a drm_connector > to implement writeback isn't something we can revisit, but we could > avoid exposing that to drivers by considering "real" connectors and > writeback connectors two different types of entities in the APIs the DRM > core exposes to drivers. What do you think, would it help for i915 ? > Hi Jani and Suraj Since atleast there is agreement on changing the drm_encoder to a pointer in the drm_writeback_connector, can we re-arrange the series OR split it into encoder first and then connector so that atleast those bits can go in first? It will benefit both our (i915 & MSM ) implementations. Hi Laurent For the connector part, can you please post a RFC for your proposal? Perhaps myself and Suraj can evaluate our implementations on top of that and the encoder change. Thanks Abhinav >>>>>> The point is, you can't initialize a connector or an encoder for a >>>>>> drm_device in isolation of the rest of the driver, even if it were >>>>>> supposed to be hidden away. >
Hi Abhinav, On Wed, Mar 02, 2022 at 10:28:03AM -0800, Abhinav Kumar wrote: > On 2/28/2022 5:42 AM, Laurent Pinchart wrote: > > On Mon, Feb 28, 2022 at 02:28:27PM +0200, Laurent Pinchart wrote: > >> On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote: > >>> On Mon, 28 Feb 2022, Laurent Pinchart wrote: > >>>> On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote: > >>>>> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote: > >>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: > >>>>>>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: > >>>>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: > >>>>>>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: > >>>>>>>>>> Changing rcar_du driver to accomadate the change of > >>>>>>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder > >>>>>>>>>> to a pointer the reason for which is explained in the > >>>>>>>>>> Patch(drm: add writeback pointers to drm_connector). > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> > >>>>>>>>>> --- > >>>>>>>>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ > >>>>>>>>>> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- > >>>>>>>>>> 2 files changed, 7 insertions(+), 3 deletions(-) > >>>>>>>>>> > >>>>>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >>>>>>>>>> index 66e8839db708..68f387a04502 100644 > >>>>>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >>>>>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > >>>>>>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc { > >>>>>>>>>> const char *const *sources; > >>>>>>>>>> unsigned int sources_count; > >>>>>>>>>> > >>>>>>>>>> + struct drm_connector connector; > >>>>>>>>>> + struct drm_encoder encoder; > >>>>>>>>> > >>>>>>>>> Those fields are, at best, poorly named. Furthermore, there's no need in > >>>>>>>>> this driver or in other drivers using drm_writeback_connector to create > >>>>>>>>> an encoder or connector manually. Let's not polute all drivers because > >>>>>>>>> i915 doesn't have its abstractions right. > >>>>>>>> > >>>>>>>> i915 uses the quite common model for struct inheritance: > >>>>>>>> > >>>>>>>> struct intel_connector { > >>>>>>>> struct drm_connector base; > >>>>>>>> /* ... */ > >>>>>>>> } > >>>>>>>> > >>>>>>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, > >>>>>>>> radeon, tilcdc, and vboxvideo. > >>>>>>>> > >>>>>>>> We could argue about the relative merits of that abstraction, but I > >>>>>>>> think the bottom line is that it's popular and the drivers using it are > >>>>>>>> not going to be persuaded to move away from it. > >>>>>>> > >>>>>>> Nobody said inheritance is bad. > >>>>>>> > >>>>>>>> It's no coincidence that the drivers who've implemented writeback so far > >>>>>>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, > >>>>>>>> because the drm_writeback_connector midlayer does, forcing the issue. > >>>>>>> > >>>>>>> Are you sure it's not a coincidence ? :-) > >>>>>>> > >>>>>>> The encoder and especially connector created by drm_writeback_connector > >>>>>>> are there only because KMS requires a drm_encoder and a drm_connector to > >>>>>>> be exposed to userspace (and I could argue that using a connector for > >>>>>>> writeback is a hack, but that won't change). The connector is "virtual", > >>>>>>> I still fail to see why i915 or any other driver would need to wrap it > >>>>>>> into something else. The whole point of the drm_writeback_connector > >>>>>>> abstraction is that drivers do not have to manage the writeback > >>>>>>> drm_connector manually, they shouldn't touch it at all. > >>>>>> > >>>>>> The thing is, drm_writeback_connector_init() calling > >>>>>> drm_connector_init() on the drm_connector embedded in > >>>>>> drm_writeback_connector leads to that connector being added to the > >>>>>> drm_device's list of connectors. Ditto for the encoder. > >>>>>> > >>>>>> All the driver code that handles drm_connectors would need to take into > >>>>>> account they might not be embedded in intel_connector. Throughout the > >>>>>> driver. Ditto for the encoders. > >>>>> > >>>>> The assumption that a connector is embedded in intel_connector doesn't > >>>>> really play that well with how bridge and panel connectors work.. so > >>>>> in general this seems like a good thing to unwind. > >>>>> > >>>>> But as a point of practicality, i915 is a large driver covering a lot > >>>>> of generations of hw with a lot of users. So I can understand > >>>>> changing this design isn't something that can happen quickly or > >>>>> easily. IMO we should allow i915 to create it's own connector for > >>>>> writeback, and just document clearly that this isn't the approach new > >>>>> drivers should take. I mean, I understand idealism, but sometimes a > >>>>> dose of pragmatism is needed. :-) > >>>> > >>>> i915 is big, but so is Intel. It's not fair to treat everybody else as a > >>>> second class citizen and let Intel get away without doing its homework. > >>> > >>> Laurent, as you accuse us of not doing our homework, I'll point out that > >>> we've been embedding drm crtc, encoder and connector ever since > >>> modesetting support was added to i915 in 2008, since before *any* of the > >>> things you now use as a rationale for asking us to do a massive rewrite > >>> of the driver existed. > >>> > >>> It's been ok to embed those structures for well over ten years. It's a > >>> common pattern, basically throughout the kernel. Other drivers do it > >>> too, not just i915. There hasn't been the slightest hint this should not > >>> be done until this very conversation. > >>> > >>>> I want to see this refactoring effort moving forward in i915 (and moving > >>>> to drm_bridge would then be a good idea too). If writeback support in > >>>> i915 urgent, then we can discuss *temporary* pragmatic stopgap measures, > >>>> but not without a real effort to fix the core issue. > >>> > >>> I think the onus is on you to first convince everyone that embedding the > >>> drm core kms structures is an antipattern that all drivers, not just > >>> i915, should stop using. In OO terms, you're saying they are classes > >>> that should be final and not extended. > >>> > >>> And even then, to be totally honest, refactoring the structures is not > >>> going to be anywhere near the top of our list of things to do, for a > >>> very long time. > >> > >> I may have not expressed myself correctly. There's nothing wrong as such > >> in embedded those structures in driver-specific structures (a.k.a. C > >> inheritance). That doesn't need to change (albeit for drm_encoder I > >> think we should move away from that pattern, but that's an entirely > >> different issue, and nothing that needs to be addressed soonà. > >> > >> The issue here is assuming that every drm_connector instance can be > >> up-casted to an i915-specific structure. > > > > Thinking some more about this, I wonder a way forward could be to drop > > the writeback connectors from the connectors list, or at least make them > > invisible to drivers. The connectors list is used extensively for two > > different purposes: tracking all drm_connector instances, and tracking > > all real connectors. The former is mostly needed by the DRM core for > > bookkeeping purposes and to expose all drm_connector instances to > > userspace, while the latter is also used by drivers, in many cases in > > locations that don't expect writeback connectors. Using a drm_connector > > to implement writeback isn't something we can revisit, but we could > > avoid exposing that to drivers by considering "real" connectors and > > writeback connectors two different types of entities in the APIs the DRM > > core exposes to drivers. What do you think, would it help for i915 ? > > Hi Jani and Suraj > > Since atleast there is agreement on changing the drm_encoder to a > pointer in the drm_writeback_connector, can we re-arrange the series OR > split it into encoder first and then connector so that atleast those > bits can go in first? It will benefit both our (i915 & MSM ) > implementations. > > Hi Laurent > > For the connector part, can you please post a RFC for your proposal? > Perhaps myself and Suraj can evaluate our implementations on top of that > and the encoder change. I'm afraid I won't have time to work on this personally for at least several weeks, if not more. > >>>>>> The point is, you can't initialize a connector or an encoder for a > >>>>>> drm_device in isolation of the rest of the driver, even if it were > >>>>>> supposed to be hidden away.
On 3/2/2022 10:31 AM, Laurent Pinchart wrote: > Hi Abhinav, > > On Wed, Mar 02, 2022 at 10:28:03AM -0800, Abhinav Kumar wrote: >> On 2/28/2022 5:42 AM, Laurent Pinchart wrote: >>> On Mon, Feb 28, 2022 at 02:28:27PM +0200, Laurent Pinchart wrote: >>>> On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote: >>>>> On Mon, 28 Feb 2022, Laurent Pinchart wrote: >>>>>> On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote: >>>>>>> On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote: >>>>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: >>>>>>>>> On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote: >>>>>>>>>> On Wed, 02 Feb 2022, Laurent Pinchart wrote: >>>>>>>>>>> On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote: >>>>>>>>>>>> Changing rcar_du driver to accomadate the change of >>>>>>>>>>>> drm_writeback_connector.base and drm_writeback_connector.encoder >>>>>>>>>>>> to a pointer the reason for which is explained in the >>>>>>>>>>>> Patch(drm: add writeback pointers to drm_connector). >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> >>>>>>>>>>>> --- >>>>>>>>>>>> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ >>>>>>>>>>>> drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- >>>>>>>>>>>> 2 files changed, 7 insertions(+), 3 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>>>>>>>> index 66e8839db708..68f387a04502 100644 >>>>>>>>>>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>>>>>>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h >>>>>>>>>>>> @@ -72,6 +72,8 @@ struct rcar_du_crtc { >>>>>>>>>>>> const char *const *sources; >>>>>>>>>>>> unsigned int sources_count; >>>>>>>>>>>> >>>>>>>>>>>> + struct drm_connector connector; >>>>>>>>>>>> + struct drm_encoder encoder; >>>>>>>>>>> >>>>>>>>>>> Those fields are, at best, poorly named. Furthermore, there's no need in >>>>>>>>>>> this driver or in other drivers using drm_writeback_connector to create >>>>>>>>>>> an encoder or connector manually. Let's not polute all drivers because >>>>>>>>>>> i915 doesn't have its abstractions right. >>>>>>>>>> >>>>>>>>>> i915 uses the quite common model for struct inheritance: >>>>>>>>>> >>>>>>>>>> struct intel_connector { >>>>>>>>>> struct drm_connector base; >>>>>>>>>> /* ... */ >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau, >>>>>>>>>> radeon, tilcdc, and vboxvideo. >>>>>>>>>> >>>>>>>>>> We could argue about the relative merits of that abstraction, but I >>>>>>>>>> think the bottom line is that it's popular and the drivers using it are >>>>>>>>>> not going to be persuaded to move away from it. >>>>>>>>> >>>>>>>>> Nobody said inheritance is bad. >>>>>>>>> >>>>>>>>>> It's no coincidence that the drivers who've implemented writeback so far >>>>>>>>>> (komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction, >>>>>>>>>> because the drm_writeback_connector midlayer does, forcing the issue. >>>>>>>>> >>>>>>>>> Are you sure it's not a coincidence ? :-) >>>>>>>>> >>>>>>>>> The encoder and especially connector created by drm_writeback_connector >>>>>>>>> are there only because KMS requires a drm_encoder and a drm_connector to >>>>>>>>> be exposed to userspace (and I could argue that using a connector for >>>>>>>>> writeback is a hack, but that won't change). The connector is "virtual", >>>>>>>>> I still fail to see why i915 or any other driver would need to wrap it >>>>>>>>> into something else. The whole point of the drm_writeback_connector >>>>>>>>> abstraction is that drivers do not have to manage the writeback >>>>>>>>> drm_connector manually, they shouldn't touch it at all. >>>>>>>> >>>>>>>> The thing is, drm_writeback_connector_init() calling >>>>>>>> drm_connector_init() on the drm_connector embedded in >>>>>>>> drm_writeback_connector leads to that connector being added to the >>>>>>>> drm_device's list of connectors. Ditto for the encoder. >>>>>>>> >>>>>>>> All the driver code that handles drm_connectors would need to take into >>>>>>>> account they might not be embedded in intel_connector. Throughout the >>>>>>>> driver. Ditto for the encoders. >>>>>>> >>>>>>> The assumption that a connector is embedded in intel_connector doesn't >>>>>>> really play that well with how bridge and panel connectors work.. so >>>>>>> in general this seems like a good thing to unwind. >>>>>>> >>>>>>> But as a point of practicality, i915 is a large driver covering a lot >>>>>>> of generations of hw with a lot of users. So I can understand >>>>>>> changing this design isn't something that can happen quickly or >>>>>>> easily. IMO we should allow i915 to create it's own connector for >>>>>>> writeback, and just document clearly that this isn't the approach new >>>>>>> drivers should take. I mean, I understand idealism, but sometimes a >>>>>>> dose of pragmatism is needed. :-) >>>>>> >>>>>> i915 is big, but so is Intel. It's not fair to treat everybody else as a >>>>>> second class citizen and let Intel get away without doing its homework. >>>>> >>>>> Laurent, as you accuse us of not doing our homework, I'll point out that >>>>> we've been embedding drm crtc, encoder and connector ever since >>>>> modesetting support was added to i915 in 2008, since before *any* of the >>>>> things you now use as a rationale for asking us to do a massive rewrite >>>>> of the driver existed. >>>>> >>>>> It's been ok to embed those structures for well over ten years. It's a >>>>> common pattern, basically throughout the kernel. Other drivers do it >>>>> too, not just i915. There hasn't been the slightest hint this should not >>>>> be done until this very conversation. >>>>> >>>>>> I want to see this refactoring effort moving forward in i915 (and moving >>>>>> to drm_bridge would then be a good idea too). If writeback support in >>>>>> i915 urgent, then we can discuss *temporary* pragmatic stopgap measures, >>>>>> but not without a real effort to fix the core issue. >>>>> >>>>> I think the onus is on you to first convince everyone that embedding the >>>>> drm core kms structures is an antipattern that all drivers, not just >>>>> i915, should stop using. In OO terms, you're saying they are classes >>>>> that should be final and not extended. >>>>> >>>>> And even then, to be totally honest, refactoring the structures is not >>>>> going to be anywhere near the top of our list of things to do, for a >>>>> very long time. >>>> >>>> I may have not expressed myself correctly. There's nothing wrong as such >>>> in embedded those structures in driver-specific structures (a.k.a. C >>>> inheritance). That doesn't need to change (albeit for drm_encoder I >>>> think we should move away from that pattern, but that's an entirely >>>> different issue, and nothing that needs to be addressed soonà. >>>> >>>> The issue here is assuming that every drm_connector instance can be >>>> up-casted to an i915-specific structure. >>> >>> Thinking some more about this, I wonder a way forward could be to drop >>> the writeback connectors from the connectors list, or at least make them >>> invisible to drivers. The connectors list is used extensively for two >>> different purposes: tracking all drm_connector instances, and tracking >>> all real connectors. The former is mostly needed by the DRM core for >>> bookkeeping purposes and to expose all drm_connector instances to >>> userspace, while the latter is also used by drivers, in many cases in >>> locations that don't expect writeback connectors. Using a drm_connector >>> to implement writeback isn't something we can revisit, but we could >>> avoid exposing that to drivers by considering "real" connectors and >>> writeback connectors two different types of entities in the APIs the DRM >>> core exposes to drivers. What do you think, would it help for i915 ? >> >> Hi Jani and Suraj >> >> Since atleast there is agreement on changing the drm_encoder to a >> pointer in the drm_writeback_connector, can we re-arrange the series OR >> split it into encoder first and then connector so that atleast those >> bits can go in first? It will benefit both our (i915 & MSM ) >> implementations. >> >> Hi Laurent >> >> For the connector part, can you please post a RFC for your proposal? >> Perhaps myself and Suraj can evaluate our implementations on top of that >> and the encoder change. > > I'm afraid I won't have time to work on this personally for at least > several weeks, if not more. > Hi Laurent Ok sure, I can take this up but I need to understand the proposal a little bit more before proceeding on this. So we will discuss this in another email where we specifically talk about the connector changes. Also, I am willing to take this up once the encoder part is done and merged so that atleast we keep moving on that as MSM WB implementation can proceed with that first. Hi Jani and Suraj Any concerns with splitting up the series into encoder and connector OR re-arranging them? Let me know if you can do this OR I can also split this up keeping Suraj's name in the Co-developed by tag. Thanks Abhinav >>>>>>>> The point is, you can't initialize a connector or an encoder for a >>>>>>>> drm_device in isolation of the rest of the driver, even if it were >>>>>>>> supposed to be hidden away. >
Hi Abhinav, > Hi Laurent > > Ok sure, I can take this up but I need to understand the proposal a little bit > more before proceeding on this. So we will discuss this in another email > where we specifically talk about the connector changes. > > Also, I am willing to take this up once the encoder part is done and merged > so that atleast we keep moving on that as MSM WB implementation can > proceed with that first. > > Hi Jani and Suraj > > Any concerns with splitting up the series into encoder and connector OR re- > arranging them? > > Let me know if you can do this OR I can also split this up keeping Suraj's > name in the Co-developed by tag. I was actually thinking of floating a new patch series with both encoder and connector pointer changes but with a change in initialization functions wherein we allocate a connector and encoder incase the driver doesn’t have its own this should minimize the effect on other drivers already using current drm writeback framework and also allow i915 to create its own connector. We can work on Laurent's suggestion but that would first require the initial RFC patches and then a rework from both of our drivers side to see if they gel well with our current code which will take a considerable amount of time. We can for now however float the patch series up which minimally affects the current drivers and also allows i915 and msm to move forward with its writeback implementation off course with a proper documentation stating new drivers shouldn't try to make their own connectors and encoders and that drm_writeback will do that for them. Once that's done and merged we can work with Laurent on his proposal to improve the drm writeback framework so that this issue can be side stepped entirely in future. For now I would like to keep the encoder and connector pointer changes together. Best Regards, Suraj Kandpal
Hi, On Fri, 4 Mar 2022 at 12:56, Kandpal, Suraj <suraj.kandpal@intel.com> wrote: > > Hi Abhinav, > > Hi Laurent > > > > Ok sure, I can take this up but I need to understand the proposal a little bit > > more before proceeding on this. So we will discuss this in another email > > where we specifically talk about the connector changes. > > > > Also, I am willing to take this up once the encoder part is done and merged > > so that atleast we keep moving on that as MSM WB implementation can > > proceed with that first. > > > > Hi Jani and Suraj > > > > Any concerns with splitting up the series into encoder and connector OR re- > > arranging them? > > > > Let me know if you can do this OR I can also split this up keeping Suraj's > > name in the Co-developed by tag. > I was actually thinking of floating a new patch series with both encoder and > connector pointer changes but with a change in initialization functions wherein > we allocate a connector and encoder incase the driver doesn’t have its own this > should minimize the effect on other drivers already using current drm writeback > framework and also allow i915 to create its own connector. I thought that Laurent was quite strict about the connector. So I'd suggest targeting drm_writeback_connector with an optionally created encoder and the builtin connector > We can work on Laurent's suggestion but that would first require the initial RFC > patches and then a rework from both of our drivers side to see if they gel well > with our current code which will take a considerable amount of time. We can for > now however float the patch series up which minimally affects the current drivers > and also allows i915 and msm to move forward with its writeback implementation > off course with a proper documentation stating new drivers shouldn't try to make > their own connectors and encoders and that drm_writeback will do that for them. > Once that's done and merged we can work with Laurent on his proposal to improve > the drm writeback framework so that this issue can be side stepped entirely in future. > For now I would like to keep the encoder and connector pointer changes together.
Hi, > > Hi Abhinav, > > > Hi Laurent > > > > > > Ok sure, I can take this up but I need to understand the proposal a > > > little bit more before proceeding on this. So we will discuss this > > > in another email where we specifically talk about the connector changes. > > > > > > Also, I am willing to take this up once the encoder part is done and > > > merged so that atleast we keep moving on that as MSM WB > > > implementation can proceed with that first. > > > > > > Hi Jani and Suraj > > > > > > Any concerns with splitting up the series into encoder and connector > > > OR re- arranging them? > > > > > > Let me know if you can do this OR I can also split this up keeping > > > Suraj's name in the Co-developed by tag. > > I was actually thinking of floating a new patch series with both > > encoder and connector pointer changes but with a change in > > initialization functions wherein we allocate a connector and encoder > > incase the driver doesn’t have its own this should minimize the effect > > on other drivers already using current drm writeback framework and also > allow i915 to create its own connector. > > I thought that Laurent was quite strict about the connector. So I'd suggest > targeting drm_writeback_connector with an optionally created encoder and > the builtin connector In that case even if we target that i915 will not be able to move forward with its implementation of writeback as builtin connector does not work for us right now as explained earlier, optionally creating encoder and connector will help everyone move forward and once done we can actually sit and work on how to side step this issue using Laurent's suggestion > > > We can work on Laurent's suggestion but that would first require the > > initial RFC patches and then a rework from both of our drivers side to > > see if they gel well with our current code which will take a > > considerable amount of time. We can for now however float the patch > > series up which minimally affects the current drivers and also allows > > i915 and msm to move forward with its writeback implementation off > > course with a proper documentation stating new drivers shouldn't try to > make their own connectors and encoders and that drm_writeback will do > that for them. > > Once that's done and merged we can work with Laurent on his proposal > > to improve the drm writeback framework so that this issue can be side > stepped entirely in future. > > For now I would like to keep the encoder and connector pointer changes > together. Best Regards, Suraj Kandpal
On Fri, 4 Mar 2022 at 13:47, Kandpal, Suraj <suraj.kandpal@intel.com> wrote: > > Hi, > > > Hi Abhinav, > > > > Hi Laurent > > > > > > > > Ok sure, I can take this up but I need to understand the proposal a > > > > little bit more before proceeding on this. So we will discuss this > > > > in another email where we specifically talk about the connector changes. > > > > > > > > Also, I am willing to take this up once the encoder part is done and > > > > merged so that atleast we keep moving on that as MSM WB > > > > implementation can proceed with that first. > > > > > > > > Hi Jani and Suraj > > > > > > > > Any concerns with splitting up the series into encoder and connector > > > > OR re- arranging them? > > > > > > > > Let me know if you can do this OR I can also split this up keeping > > > > Suraj's name in the Co-developed by tag. > > > I was actually thinking of floating a new patch series with both > > > encoder and connector pointer changes but with a change in > > > initialization functions wherein we allocate a connector and encoder > > > incase the driver doesn’t have its own this should minimize the effect > > > on other drivers already using current drm writeback framework and also > > allow i915 to create its own connector. > > > > I thought that Laurent was quite strict about the connector. So I'd suggest > > targeting drm_writeback_connector with an optionally created encoder and > > the builtin connector > In that case even if we target that i915 will not be able to move forward with its > implementation of writeback as builtin connector does not work for us right now > as explained earlier, optionally creating encoder and connector will help everyone > move forward and once done we can actually sit and work on how to side step this > issue using Laurent's suggestion This is up to Laurent & other DRM maintainers to decide whether this approach would be accepted or not. So far unfortunately I have mostly seen the pushback and a strong suggestion to stop treating each drm_connector as i915_connector. I haven't checked the exact details, but IMO adding a branch if the connector's type is DRM_CONNECTOR_VIRTUAL should be doable. > > > > > We can work on Laurent's suggestion but that would first require the > > > initial RFC patches and then a rework from both of our drivers side to > > > see if they gel well with our current code which will take a > > > considerable amount of time. We can for now however float the patch > > > series up which minimally affects the current drivers and also allows > > > i915 and msm to move forward with its writeback implementation off > > > course with a proper documentation stating new drivers shouldn't try to > > make their own connectors and encoders and that drm_writeback will do > > that for them. > > > Once that's done and merged we can work with Laurent on his proposal > > > to improve the drm writeback framework so that this issue can be side > > stepped entirely in future. > > > For now I would like to keep the encoder and connector pointer changes > > together. > > Best Regards, > Suraj Kandpal
Hi, > > Hi, > > > > Hi Abhinav, > > > > > Hi Laurent > > > > > > > > > > Ok sure, I can take this up but I need to understand the > > > > > proposal a little bit more before proceeding on this. So we will > > > > > discuss this in another email where we specifically talk about the > connector changes. > > > > > > > > > > Also, I am willing to take this up once the encoder part is done > > > > > and merged so that atleast we keep moving on that as MSM WB > > > > > implementation can proceed with that first. > > > > > > > > > > Hi Jani and Suraj > > > > > > > > > > Any concerns with splitting up the series into encoder and > > > > > connector OR re- arranging them? > > > > > > > > > > Let me know if you can do this OR I can also split this up > > > > > keeping Suraj's name in the Co-developed by tag. > > > > I was actually thinking of floating a new patch series with both > > > > encoder and connector pointer changes but with a change in > > > > initialization functions wherein we allocate a connector and > > > > encoder incase the driver doesn’t have its own this should > > > > minimize the effect on other drivers already using current drm > > > > writeback framework and also > > > allow i915 to create its own connector. > > > > > > I thought that Laurent was quite strict about the connector. So I'd > > > suggest targeting drm_writeback_connector with an optionally created > > > encoder and the builtin connector > > In that case even if we target that i915 will not be able to move > > forward with its implementation of writeback as builtin connector does > > not work for us right now as explained earlier, optionally creating > > encoder and connector will help everyone move forward and once done > we > > can actually sit and work on how to side step this issue using > > Laurent's suggestion > > This is up to Laurent & other DRM maintainers to decide whether this > approach would be accepted or not. > So far unfortunately I have mostly seen the pushback and a strong > suggestion to stop treating each drm_connector as i915_connector. > I haven't checked the exact details, but IMO adding a branch if the > connector's type is DRM_CONNECTOR_VIRTUAL should be doable. Bringing in the change where we don’t treat each drm_connector as an intel_connector or even adding a branch which checks if drm_connector is DRM_CONNECTOR_VIRTUAL and conditionally cast it to intel_connector is quite a lot of work for i915. That's why I was suggesting if for now we could move forward with optionally creating both encoder and connector minimizing affects on other drivers as well as allowing us to move forward. What do you think, Launrent? > > > > > > > > We can work on Laurent's suggestion but that would first require > > > > the initial RFC patches and then a rework from both of our drivers > > > > side to see if they gel well with our current code which will take > > > > a considerable amount of time. We can for now however float the > > > > patch series up which minimally affects the current drivers and > > > > also allows > > > > i915 and msm to move forward with its writeback implementation off > > > > course with a proper documentation stating new drivers shouldn't > > > > try to > > > make their own connectors and encoders and that drm_writeback will > > > do that for them. > > > > Once that's done and merged we can work with Laurent on his > > > > proposal to improve the drm writeback framework so that this issue > > > > can be side > > > stepped entirely in future. > > > > For now I would like to keep the encoder and connector pointer > > > > changes > > > together. > > > > > > -- > With best wishes > Dmitry Best Regards, Suraj Kandpal
Hi Suraj On 3/4/2022 6:16 AM, Kandpal, Suraj wrote: > Hi, >>> Hi, >>>>> Hi Abhinav, >>>>>> Hi Laurent >>>>>> >>>>>> Ok sure, I can take this up but I need to understand the >>>>>> proposal a little bit more before proceeding on this. So we will >>>>>> discuss this in another email where we specifically talk about the >> connector changes. >>>>>> >>>>>> Also, I am willing to take this up once the encoder part is done >>>>>> and merged so that atleast we keep moving on that as MSM WB >>>>>> implementation can proceed with that first. >>>>>> >>>>>> Hi Jani and Suraj >>>>>> >>>>>> Any concerns with splitting up the series into encoder and >>>>>> connector OR re- arranging them? >>>>>> >>>>>> Let me know if you can do this OR I can also split this up >>>>>> keeping Suraj's name in the Co-developed by tag. >>>>> I was actually thinking of floating a new patch series with both >>>>> encoder and connector pointer changes but with a change in >>>>> initialization functions wherein we allocate a connector and >>>>> encoder incase the driver doesn’t have its own this should >>>>> minimize the effect on other drivers already using current drm >>>>> writeback framework and also >>>> allow i915 to create its own connector. >>>> I was proposing to split up the encoder and connector because the comments from Laurent meant we were in agreement about the encoder but not about the connector. I am afraid another attempt with the similar idea is only going to stall the series again and hence i gave this option. Eventually its upto Laurent and the other maintainers to guide us forward on this as this series has been stalled for weeks now. >>>> I thought that Laurent was quite strict about the connector. So I'd >>>> suggest targeting drm_writeback_connector with an optionally created >>>> encoder and the builtin connector >>> In that case even if we target that i915 will not be able to move >>> forward with its implementation of writeback as builtin connector does >>> not work for us right now as explained earlier, optionally creating >>> encoder and connector will help everyone move forward and once done >> we >>> can actually sit and work on how to side step this issue using >>> Laurent's suggestion >> >> This is up to Laurent & other DRM maintainers to decide whether this >> approach would be accepted or not. >> So far unfortunately I have mostly seen the pushback and a strong >> suggestion to stop treating each drm_connector as i915_connector. >> I haven't checked the exact details, but IMO adding a branch if the >> connector's type is DRM_CONNECTOR_VIRTUAL should be doable. > Bringing in the change where we don’t treat each drm_connector as > an intel_connector or even adding a branch which checks if > drm_connector is DRM_CONNECTOR_VIRTUAL and conditionally cast it > to intel_connector is quite a lot of work for i915. > That's why I was suggesting if for now we could move forward with optionally > creating both encoder and connector minimizing affects on other drivers as > well as allowing us to move forward. > What do you think, Launrent? > >> >>>> >>>>> We can work on Laurent's suggestion but that would first require >>>>> the initial RFC patches and then a rework from both of our drivers >>>>> side to see if they gel well with our current code which will take >>>>> a considerable amount of time. We can for now however float the >>>>> patch series up which minimally affects the current drivers and >>>>> also allows >>>>> i915 and msm to move forward with its writeback implementation off >>>>> course with a proper documentation stating new drivers shouldn't >>>>> try to >>>> make their own connectors and encoders and that drm_writeback will >>>> do that for them. >>>>> Once that's done and merged we can work with Laurent on his >>>>> proposal to improve the drm writeback framework so that this issue >>>>> can be side >>>> stepped entirely in future. >>>>> For now I would like to keep the encoder and connector pointer >>>>> changes >>>> together. >>> >> >> >> >> -- >> With best wishes >> Dmitry > > Best Regards, > Suraj Kandpal
Hi Abhinav, > > Hi, > >>> Hi, > >>>>> Hi Abhinav, > >>>>>> Hi Laurent > >>>>>> > >>>>>> Ok sure, I can take this up but I need to understand the proposal > >>>>>> a little bit more before proceeding on this. So we will discuss > >>>>>> this in another email where we specifically talk about the > >> connector changes. > >>>>>> > >>>>>> Also, I am willing to take this up once the encoder part is done > >>>>>> and merged so that atleast we keep moving on that as MSM WB > >>>>>> implementation can proceed with that first. > >>>>>> > >>>>>> Hi Jani and Suraj > >>>>>> > >>>>>> Any concerns with splitting up the series into encoder and > >>>>>> connector OR re- arranging them? > >>>>>> > >>>>>> Let me know if you can do this OR I can also split this up > >>>>>> keeping Suraj's name in the Co-developed by tag. > >>>>> I was actually thinking of floating a new patch series with both > >>>>> encoder and connector pointer changes but with a change in > >>>>> initialization functions wherein we allocate a connector and > >>>>> encoder incase the driver doesn’t have its own this should > >>>>> minimize the effect on other drivers already using current drm > >>>>> writeback framework and also > >>>> allow i915 to create its own connector. > >>>> > > I was proposing to split up the encoder and connector because the > comments from Laurent meant we were in agreement about the encoder > but not about the connector. > > I am afraid another attempt with the similar idea is only going to stall the > series again and hence i gave this option. Seems like this patch series currently won't be able to move forward with the connector pointer change so I guess you can split the series to encoder pointer change where we optionally create encoder if required ,keeping my name in co-developed tag so that msm can move forward with its implementation and then we can work to see how the connector issue can be bypassed. Best Regards, Suraj Kandpal > Eventually its upto Laurent and the other maintainers to guide us forward on > this as this series has been stalled for weeks now. > > >>>> I thought that Laurent was quite strict about the connector. So I'd > >>>> suggest targeting drm_writeback_connector with an optionally > >>>> created encoder and the builtin connector > >>> In that case even if we target that i915 will not be able to move > >>> forward with its implementation of writeback as builtin connector > >>> does not work for us right now as explained earlier, optionally > >>> creating encoder and connector will help everyone move forward and > >>> once done > >> we > >>> can actually sit and work on how to side step this issue using > >>> Laurent's suggestion > >> > >> This is up to Laurent & other DRM maintainers to decide whether this > >> approach would be accepted or not. > >> So far unfortunately I have mostly seen the pushback and a strong > >> suggestion to stop treating each drm_connector as i915_connector. > >> I haven't checked the exact details, but IMO adding a branch if the > >> connector's type is DRM_CONNECTOR_VIRTUAL should be doable. > > Bringing in the change where we don’t treat each drm_connector as an > > intel_connector or even adding a branch which checks if drm_connector > > is DRM_CONNECTOR_VIRTUAL and conditionally cast it to intel_connector > > is quite a lot of work for i915. > > That's why I was suggesting if for now we could move forward with > > optionally creating both encoder and connector minimizing affects on > > other drivers as well as allowing us to move forward. > > What do you think, Launrent? > > > >> > >>>> > >>>>> We can work on Laurent's suggestion but that would first require > >>>>> the initial RFC patches and then a rework from both of our drivers > >>>>> side to see if they gel well with our current code which will take > >>>>> a considerable amount of time. We can for now however float the > >>>>> patch series up which minimally affects the current drivers and > >>>>> also allows > >>>>> i915 and msm to move forward with its writeback implementation off > >>>>> course with a proper documentation stating new drivers shouldn't > >>>>> try to > >>>> make their own connectors and encoders and that drm_writeback will > >>>> do that for them. > >>>>> Once that's done and merged we can work with Laurent on his > >>>>> proposal to improve the drm writeback framework so that this issue > >>>>> can be side > >>>> stepped entirely in future. > >>>>> For now I would like to keep the encoder and connector pointer > >>>>> changes > >>>> together. > >>> > >>
Hi Suraj On 3/8/2022 6:30 AM, Kandpal, Suraj wrote: > Hi Abhinav, >>> Hi, >>>>> Hi, >>>>>>> Hi Abhinav, >>>>>>>> Hi Laurent >>>>>>>> >>>>>>>> Ok sure, I can take this up but I need to understand the proposal >>>>>>>> a little bit more before proceeding on this. So we will discuss >>>>>>>> this in another email where we specifically talk about the >>>> connector changes. >>>>>>>> >>>>>>>> Also, I am willing to take this up once the encoder part is done >>>>>>>> and merged so that atleast we keep moving on that as MSM WB >>>>>>>> implementation can proceed with that first. >>>>>>>> >>>>>>>> Hi Jani and Suraj >>>>>>>> >>>>>>>> Any concerns with splitting up the series into encoder and >>>>>>>> connector OR re- arranging them? >>>>>>>> >>>>>>>> Let me know if you can do this OR I can also split this up >>>>>>>> keeping Suraj's name in the Co-developed by tag. >>>>>>> I was actually thinking of floating a new patch series with both >>>>>>> encoder and connector pointer changes but with a change in >>>>>>> initialization functions wherein we allocate a connector and >>>>>>> encoder incase the driver doesn’t have its own this should >>>>>>> minimize the effect on other drivers already using current drm >>>>>>> writeback framework and also >>>>>> allow i915 to create its own connector. >>>>>> >> >> I was proposing to split up the encoder and connector because the >> comments from Laurent meant we were in agreement about the encoder >> but not about the connector. >> >> I am afraid another attempt with the similar idea is only going to stall the >> series again and hence i gave this option. > > Seems like this patch series currently won't be able to move forward with the > connector pointer change so I guess you can split the series to encoder pointer > change where we optionally create encoder if required ,keeping my name in > co-developed tag so that msm can move forward with its implementation and > then we can work to see how the connector issue can be bypassed. > > Best Regards, > Suraj Kandpal Thanks, let me re-spin this and will keep your name as co-developer. Should be able to get it on the list in a week. Thanks Abhinav >> Eventually its upto Laurent and the other maintainers to guide us forward on >> this as this series has been stalled for weeks now. >> >>>>>> I thought that Laurent was quite strict about the connector. So I'd >>>>>> suggest targeting drm_writeback_connector with an optionally >>>>>> created encoder and the builtin connector >>>>> In that case even if we target that i915 will not be able to move >>>>> forward with its implementation of writeback as builtin connector >>>>> does not work for us right now as explained earlier, optionally >>>>> creating encoder and connector will help everyone move forward and >>>>> once done >>>> we >>>>> can actually sit and work on how to side step this issue using >>>>> Laurent's suggestion >>>> >>>> This is up to Laurent & other DRM maintainers to decide whether this >>>> approach would be accepted or not. >>>> So far unfortunately I have mostly seen the pushback and a strong >>>> suggestion to stop treating each drm_connector as i915_connector. >>>> I haven't checked the exact details, but IMO adding a branch if the >>>> connector's type is DRM_CONNECTOR_VIRTUAL should be doable. >>> Bringing in the change where we don’t treat each drm_connector as an >>> intel_connector or even adding a branch which checks if drm_connector >>> is DRM_CONNECTOR_VIRTUAL and conditionally cast it to intel_connector >>> is quite a lot of work for i915. >>> That's why I was suggesting if for now we could move forward with >>> optionally creating both encoder and connector minimizing affects on >>> other drivers as well as allowing us to move forward. >>> What do you think, Launrent? >>> >>>> >>>>>> >>>>>>> We can work on Laurent's suggestion but that would first require >>>>>>> the initial RFC patches and then a rework from both of our drivers >>>>>>> side to see if they gel well with our current code which will take >>>>>>> a considerable amount of time. We can for now however float the >>>>>>> patch series up which minimally affects the current drivers and >>>>>>> also allows >>>>>>> i915 and msm to move forward with its writeback implementation off >>>>>>> course with a proper documentation stating new drivers shouldn't >>>>>>> try to >>>>>> make their own connectors and encoders and that drm_writeback will >>>>>> do that for them. >>>>>>> Once that's done and merged we can work with Laurent on his >>>>>>> proposal to improve the drm writeback framework so that this issue >>>>>>> can be side >>>>>> stepped entirely in future. >>>>>>> For now I would like to keep the encoder and connector pointer >>>>>>> changes >>>>>> together. >>>>> >>>>
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index 66e8839db708..68f387a04502 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h @@ -72,6 +72,8 @@ struct rcar_du_crtc { const char *const *sources; unsigned int sources_count; + struct drm_connector connector; + struct drm_encoder encoder; struct drm_writeback_connector writeback; }; diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c index c79d1259e49b..5b1e83380c47 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c @@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu, { struct drm_writeback_connector *wb_conn = &rcrtc->writeback; - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); - drm_connector_helper_add(&wb_conn->base, + wb_conn->base = &rcrtc->connector; + wb_conn->encoder = &rcrtc->encoder; + wb_conn->encoder->possible_crtcs = 1 << drm_crtc_index(&rcrtc->crtc); + drm_connector_helper_add(wb_conn->base, &rcar_du_wb_conn_helper_funcs); return drm_writeback_connector_init(&rcdu->ddev, wb_conn, @@ -220,7 +222,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, struct drm_framebuffer *fb; unsigned int i; - state = rcrtc->writeback.base.state; + state = rcrtc->writeback.base->state; if (!state || !state->writeback_job) return;
Changing rcar_du driver to accomadate the change of drm_writeback_connector.base and drm_writeback_connector.encoder to a pointer the reason for which is explained in the Patch(drm: add writeback pointers to drm_connector). Signed-off-by: Kandpal Suraj <suraj.kandpal@intel.com> --- drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++ drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-)