From: Rob Clark <robdclark@gmail.com> To: Jani Nikula <jani.nikula@intel.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Kandpal Suraj <suraj.kandpal@intel.com>, Carsten Haitzler <carsten.haitzler@arm.com>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>, Abhinav Kumar <quic_abhinavk@quicinc.com>, dri-devel <dri-devel@lists.freedesktop.org>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, arun.r.murthy@intel.com Subject: Re: [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes Date: Sat, 26 Feb 2022 10:27:59 -0800 [thread overview] Message-ID: <CAF6AEGtdnWWhGp7U7nAPD4r3Uoe5BJKVm2rQ2MS=q5GqF6MYDA@mail.gmail.com> (raw) In-Reply-To: <87v8xxs2hz.fsf@intel.com> 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. >
WARNING: multiple messages have this Message-ID (diff)
From: Rob Clark <robdclark@gmail.com> To: Jani Nikula <jani.nikula@intel.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Carsten Haitzler <carsten.haitzler@arm.com>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>, Abhinav Kumar <quic_abhinavk@quicinc.com>, dri-devel <dri-devel@lists.freedesktop.org>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Subject: Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes Date: Sat, 26 Feb 2022 10:27:59 -0800 [thread overview] Message-ID: <CAF6AEGtdnWWhGp7U7nAPD4r3Uoe5BJKVm2rQ2MS=q5GqF6MYDA@mail.gmail.com> (raw) In-Reply-To: <87v8xxs2hz.fsf@intel.com> 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. >
next prev parent reply other threads:[~2022-02-26 18:27 UTC|newest] Thread overview: 98+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-02-02 8:54 [PATCH 0/6] drm writeback connector changes Kandpal Suraj 2022-02-02 8:54 ` [Intel-gfx] " Kandpal Suraj 2022-02-02 8:54 ` [PATCH 1/6] drm: add writeback pointers to drm_connector Kandpal Suraj 2022-02-02 8:54 ` [Intel-gfx] " Kandpal Suraj 2022-02-02 10:28 ` Dmitry Baryshkov 2022-02-02 10:28 ` [Intel-gfx] " Dmitry Baryshkov 2022-02-03 8:46 ` Kandpal, Suraj 2022-02-03 8:46 ` [Intel-gfx] " Kandpal, Suraj 2022-02-02 11:17 ` kernel test robot 2022-02-02 11:17 ` kernel test robot 2022-02-02 20:07 ` kernel test robot 2022-02-02 20:07 ` kernel test robot 2022-02-02 20:07 ` kernel test robot 2022-02-02 8:54 ` [PATCH 2/6] drm/arm/komeda : change driver to use drm_writeback_connector.base pointer Kandpal Suraj 2022-02-02 8:54 ` [Intel-gfx] " Kandpal Suraj 2022-02-02 8:54 ` [PATCH 3/6] drm/vkms: change vkms " Kandpal Suraj 2022-02-02 8:54 ` [Intel-gfx] " Kandpal Suraj 2022-02-02 8:54 ` [PATCH 4/6] drm/vc4: vc4 driver changes to accommodate changes done in drm_writeback_connector structure Kandpal Suraj 2022-02-02 8:54 ` [Intel-gfx] " Kandpal Suraj 2022-02-02 8:54 ` [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes Kandpal Suraj 2022-02-02 8:54 ` [Intel-gfx] " Kandpal Suraj 2022-02-02 12:42 ` Laurent Pinchart 2022-02-02 12:42 ` [Intel-gfx] " Laurent Pinchart 2022-02-02 13:15 ` Jani Nikula 2022-02-02 13:15 ` [Intel-gfx] " Jani Nikula 2022-02-02 13:26 ` Laurent Pinchart 2022-02-02 13:26 ` [Intel-gfx] " Laurent Pinchart 2022-02-02 15:38 ` Jani Nikula 2022-02-02 15:38 ` [Intel-gfx] " Jani Nikula 2022-02-26 18:27 ` Rob Clark [this message] 2022-02-26 18:27 ` Rob Clark 2022-02-28 8:03 ` Laurent Pinchart 2022-02-28 8:03 ` [Intel-gfx] " Laurent Pinchart 2022-02-28 12:09 ` Jani Nikula 2022-02-28 12:09 ` [Intel-gfx] " Jani Nikula 2022-02-28 12:28 ` Laurent Pinchart 2022-02-28 12:28 ` [Intel-gfx] " Laurent Pinchart 2022-02-28 13:42 ` Laurent Pinchart 2022-02-28 13:42 ` [Intel-gfx] " Laurent Pinchart 2022-03-02 18:28 ` Abhinav Kumar 2022-03-02 18:28 ` [Intel-gfx] " Abhinav Kumar 2022-03-02 18:31 ` Laurent Pinchart 2022-03-02 18:31 ` [Intel-gfx] " Laurent Pinchart 2022-03-03 17:32 ` Abhinav Kumar 2022-03-03 17:32 ` [Intel-gfx] " Abhinav Kumar 2022-03-04 9:56 ` Kandpal, Suraj 2022-03-04 9:56 ` [Intel-gfx] " Kandpal, Suraj 2022-03-04 10:39 ` Dmitry Baryshkov 2022-03-04 10:39 ` [Intel-gfx] " Dmitry Baryshkov 2022-03-04 10:47 ` Kandpal, Suraj 2022-03-04 10:47 ` [Intel-gfx] " Kandpal, Suraj 2022-03-04 11:25 ` Dmitry Baryshkov 2022-03-04 11:25 ` [Intel-gfx] " Dmitry Baryshkov 2022-03-04 14:16 ` Kandpal, Suraj 2022-03-04 14:16 ` [Intel-gfx] " Kandpal, Suraj 2022-03-04 20:47 ` Abhinav Kumar 2022-03-04 20:47 ` [Intel-gfx] " Abhinav Kumar 2022-03-08 14:30 ` Kandpal, Suraj 2022-03-08 14:30 ` [Intel-gfx] " Kandpal, Suraj 2022-03-08 19:44 ` Abhinav Kumar 2022-03-08 19:44 ` [Intel-gfx] " Abhinav Kumar 2022-02-06 23:32 ` Dmitry Baryshkov 2022-02-06 23:32 ` [Intel-gfx] " Dmitry Baryshkov 2022-02-07 7:20 ` Abhinav Kumar 2022-02-07 7:20 ` [Intel-gfx] " Abhinav Kumar 2022-02-10 1:40 ` Abhinav Kumar 2022-02-10 1:40 ` [Intel-gfx] " Abhinav Kumar 2022-02-10 4:58 ` Laurent Pinchart 2022-02-10 4:58 ` [Intel-gfx] " Laurent Pinchart 2022-02-22 3:32 ` Dmitry Baryshkov 2022-02-22 3:32 ` [Intel-gfx] " Dmitry Baryshkov 2022-02-22 7:34 ` Laurent Pinchart 2022-02-22 7:34 ` [Intel-gfx] " Laurent Pinchart 2022-02-24 0:27 ` Abhinav Kumar 2022-02-24 0:27 ` [Intel-gfx] " Abhinav Kumar 2022-02-02 13:34 ` Ville Syrjälä 2022-02-02 13:34 ` [Intel-gfx] " Ville Syrjälä 2022-02-02 13:40 ` Dmitry Baryshkov 2022-02-02 13:40 ` [Intel-gfx] " Dmitry Baryshkov 2022-02-02 15:57 ` Jani Nikula 2022-02-02 15:57 ` [Intel-gfx] " Jani Nikula 2022-02-23 6:17 ` Kandpal, Suraj 2022-02-23 6:17 ` [Intel-gfx] " Kandpal, Suraj 2022-02-25 23:26 ` Abhinav Kumar 2022-02-25 23:26 ` [Intel-gfx] " Abhinav Kumar 2022-02-26 5:10 ` Kandpal, Suraj 2022-02-26 5:10 ` [Intel-gfx] " Kandpal, Suraj 2022-02-28 8:00 ` Laurent Pinchart 2022-02-28 8:00 ` [Intel-gfx] " Laurent Pinchart 2022-02-28 8:07 ` Dmitry Baryshkov 2022-02-28 8:07 ` [Intel-gfx] " Dmitry Baryshkov 2022-02-28 8:28 ` Laurent Pinchart 2022-02-28 8:28 ` [Intel-gfx] " Laurent Pinchart 2022-02-02 8:54 ` [PATCH 6/6] drm/arm: changes to malidp " Kandpal Suraj 2022-02-02 8:54 ` [Intel-gfx] " Kandpal Suraj 2022-02-02 10:01 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm writeback connector changes Patchwork 2022-02-02 10:27 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2022-02-02 12:22 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAF6AEGtdnWWhGp7U7nAPD4r3Uoe5BJKVm2rQ2MS=q5GqF6MYDA@mail.gmail.com' \ --to=robdclark@gmail.com \ --cc=arun.r.murthy@intel.com \ --cc=carsten.haitzler@arm.com \ --cc=dmitry.baryshkov@linaro.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=jani.nikula@intel.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=quic_abhinavk@quicinc.com \ --cc=suraj.kandpal@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.