| Message ID | cover.1664966047.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/connector: connector iterator with filtering | expand |
On Wed, 05 Oct 2022, Jani Nikula <jani.nikula@intel.com> wrote: > Currently i915 assumes all drm_connectors it encounters are embedded in > intel_connectors that i915 allocated. The drm_writeback_connector forces > a design where this is not the case; we can't provide our own connector, > and writeback embeds the drm_connector it initializes itself. > > To use drm writeback, none of the i915 connector iteration could assume > the drm connector is embedded in intel_connector. Checking this is > tedious, and would require an intermediate step with > drm_connector. Here's an idea I came up with; filtering at the drm > connector iterator level with a caller supplied function. Not too much > code, and could be used for other things as well. > > Mind you, we'd still much rather modify drm writeback to allow passing > the connector i915 allocated, instead of the current midlayer design > that forces drivers to a certain model. Working around this is a bunch > of error prone and tedious code that we really could do without. Any feedback on this one? BR, Jani. > > > BR, > Jani. > > > Cc: Arun R Murthy <arun.r.murthy@intel.com> > Cc: Dave Airlie <airlied@gmail.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Suraj Kandpal <suraj.kandpal@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Jani Nikula (2): > drm/connector: add connector list iteration with filtering > drm/i915: iterate intel_connectors only > > drivers/gpu/drm/drm_connector.c | 57 +++++++++++++++---- > drivers/gpu/drm/i915/display/intel_display.c | 3 +- > .../drm/i915/display/intel_display_types.h | 7 +++ > drivers/gpu/drm/i915/display/intel_dp.c | 6 +- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +- > drivers/gpu/drm/i915/display/intel_hdcp.c | 3 +- > drivers/gpu/drm/i915/display/intel_hotplug.c | 12 ++-- > .../drm/i915/display/intel_modeset_setup.c | 6 +- > drivers/gpu/drm/i915/display/intel_opregion.c | 9 ++- > include/drm/drm_connector.h | 9 +++ > 10 files changed, 89 insertions(+), 26 deletions(-)
On 10/5/22 06:51, Jani Nikula wrote: > Currently i915 assumes all drm_connectors it encounters are embedded in > intel_connectors that i915 allocated. The drm_writeback_connector forces > a design where this is not the case; we can't provide our own connector, > and writeback embeds the drm_connector it initializes itself. > > To use drm writeback, none of the i915 connector iteration could assume > the drm connector is embedded in intel_connector. Checking this is > tedious, and would require an intermediate step with > drm_connector. Here's an idea I came up with; filtering at the drm > connector iterator level with a caller supplied function. Not too much > code, and could be used for other things as well. > We've been trying to hook up drm_writeback_connector in amdgpu and this would be really helpful. I've had to do liberal sprinkling of "!= DRM_MODE_CONNECTOR_WRITEBACK" all over the place. > Mind you, we'd still much rather modify drm writeback to allow passing > the connector i915 allocated, instead of the current midlayer design > that forces drivers to a certain model. Working around this is a bunch > of error prone and tedious code that we really could do without. > I think this would be even better but also be much more work and impact every driver that implements writeback. FWIW, there was no way for me to add writeback connector handling without KASAN. Interpreting the connector wrong in one place leads to memory corruption and undefined behavior and is almost impossible to spot without KASAN. This series is Acked-by: Harry Wentland <harry.wentland@amd.com> Harry > > BR, > Jani. > > > Cc: Arun R Murthy <arun.r.murthy@intel.com> > Cc: Dave Airlie <airlied@gmail.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Suraj Kandpal <suraj.kandpal@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Jani Nikula (2): > drm/connector: add connector list iteration with filtering > drm/i915: iterate intel_connectors only > > drivers/gpu/drm/drm_connector.c | 57 +++++++++++++++---- > drivers/gpu/drm/i915/display/intel_display.c | 3 +- > .../drm/i915/display/intel_display_types.h | 7 +++ > drivers/gpu/drm/i915/display/intel_dp.c | 6 +- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +- > drivers/gpu/drm/i915/display/intel_hdcp.c | 3 +- > drivers/gpu/drm/i915/display/intel_hotplug.c | 12 ++-- > .../drm/i915/display/intel_modeset_setup.c | 6 +- > drivers/gpu/drm/i915/display/intel_opregion.c | 9 ++- > include/drm/drm_connector.h | 9 +++ > 10 files changed, 89 insertions(+), 26 deletions(-) >
On Fri, 25 Nov 2022, Harry Wentland <harry.wentland@amd.com> wrote: > On 10/5/22 06:51, Jani Nikula wrote: >> Currently i915 assumes all drm_connectors it encounters are embedded in >> intel_connectors that i915 allocated. The drm_writeback_connector forces >> a design where this is not the case; we can't provide our own connector, >> and writeback embeds the drm_connector it initializes itself. >> >> To use drm writeback, none of the i915 connector iteration could assume >> the drm connector is embedded in intel_connector. Checking this is >> tedious, and would require an intermediate step with >> drm_connector. Here's an idea I came up with; filtering at the drm >> connector iterator level with a caller supplied function. Not too much >> code, and could be used for other things as well. >> > > We've been trying to hook up drm_writeback_connector in amdgpu and > this would be really helpful. I've had to do liberal sprinkling > of "!= DRM_MODE_CONNECTOR_WRITEBACK" all over the place. > >> Mind you, we'd still much rather modify drm writeback to allow passing >> the connector i915 allocated, instead of the current midlayer design >> that forces drivers to a certain model. Working around this is a bunch >> of error prone and tedious code that we really could do without. >> > > I think this would be even better but also be much more work and impact > every driver that implements writeback. FWIW, there was no way for me > to add writeback connector handling without KASAN. Interpreting the > connector wrong in one place leads to memory corruption and > undefined behavior and is almost impossible to spot without KASAN. Laurent, I once again plead - could we please reconsider this and give drivers the option to allocate and provide the drm_connector to writeback themselves? All things considered I think that would be the safer option. BR, Jani. > > This series is > Acked-by: Harry Wentland <harry.wentland@amd.com> > > Harry > >> >> BR, >> Jani. >> >> >> Cc: Arun R Murthy <arun.r.murthy@intel.com> >> Cc: Dave Airlie <airlied@gmail.com> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Cc: Suraj Kandpal <suraj.kandpal@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> Jani Nikula (2): >> drm/connector: add connector list iteration with filtering >> drm/i915: iterate intel_connectors only >> >> drivers/gpu/drm/drm_connector.c | 57 +++++++++++++++---- >> drivers/gpu/drm/i915/display/intel_display.c | 3 +- >> .../drm/i915/display/intel_display_types.h | 7 +++ >> drivers/gpu/drm/i915/display/intel_dp.c | 6 +- >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +- >> drivers/gpu/drm/i915/display/intel_hdcp.c | 3 +- >> drivers/gpu/drm/i915/display/intel_hotplug.c | 12 ++-- >> .../drm/i915/display/intel_modeset_setup.c | 6 +- >> drivers/gpu/drm/i915/display/intel_opregion.c | 9 ++- >> include/drm/drm_connector.h | 9 +++ >> 10 files changed, 89 insertions(+), 26 deletions(-) >> >
Hi Jani, On Tue, Nov 29, 2022 at 11:29:45AM +0200, Jani Nikula wrote: > On Fri, 25 Nov 2022, Harry Wentland <harry.wentland@amd.com> wrote: > > On 10/5/22 06:51, Jani Nikula wrote: > >> Currently i915 assumes all drm_connectors it encounters are embedded in > >> intel_connectors that i915 allocated. The drm_writeback_connector forces > >> a design where this is not the case; we can't provide our own connector, > >> and writeback embeds the drm_connector it initializes itself. > >> > >> To use drm writeback, none of the i915 connector iteration could assume > >> the drm connector is embedded in intel_connector. Checking this is > >> tedious, and would require an intermediate step with > >> drm_connector. Here's an idea I came up with; filtering at the drm > >> connector iterator level with a caller supplied function. Not too much > >> code, and could be used for other things as well. > >> > > > > We've been trying to hook up drm_writeback_connector in amdgpu and > > this would be really helpful. I've had to do liberal sprinkling > > of "!= DRM_MODE_CONNECTOR_WRITEBACK" all over the place. > > > >> Mind you, we'd still much rather modify drm writeback to allow passing > >> the connector i915 allocated, instead of the current midlayer design > >> that forces drivers to a certain model. Working around this is a bunch > >> of error prone and tedious code that we really could do without. > > > > I think this would be even better but also be much more work and impact > > every driver that implements writeback. FWIW, there was no way for me > > to add writeback connector handling without KASAN. Interpreting the > > connector wrong in one place leads to memory corruption and > > undefined behavior and is almost impossible to spot without KASAN. > > Laurent, I once again plead - could we please reconsider this and give > drivers the option to allocate and provide the drm_connector to > writeback themselves? > > All things considered I think that would be the safer option. I do think all this stems from the decision of using connectors for writeback, and we're paying the price for it now. It's painful in drivers, and that's why I would prefer hiding it from drivers and handling it completely in the DRM core. Ideally the connector enumeration API exposed to drivers should not even enumerate the writeback connectors by default. I'm just a contributor to the subsystem, so I don't make the call. If there's a general consensus it's better to require all drivers to handle writeback connectors explicitly everywhere (Daniel and Dave may want to chime in here), I can be overruled, like anybody else. > > This series is > > Acked-by: Harry Wentland <harry.wentland@amd.com> > > > >> Cc: Arun R Murthy <arun.r.murthy@intel.com> > >> Cc: Dave Airlie <airlied@gmail.com> > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Cc: Suraj Kandpal <suraj.kandpal@intel.com> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > >> Jani Nikula (2): > >> drm/connector: add connector list iteration with filtering > >> drm/i915: iterate intel_connectors only > >> > >> drivers/gpu/drm/drm_connector.c | 57 +++++++++++++++---- > >> drivers/gpu/drm/i915/display/intel_display.c | 3 +- > >> .../drm/i915/display/intel_display_types.h | 7 +++ > >> drivers/gpu/drm/i915/display/intel_dp.c | 6 +- > >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +- > >> drivers/gpu/drm/i915/display/intel_hdcp.c | 3 +- > >> drivers/gpu/drm/i915/display/intel_hotplug.c | 12 ++-- > >> .../drm/i915/display/intel_modeset_setup.c | 6 +- > >> drivers/gpu/drm/i915/display/intel_opregion.c | 9 ++- > >> include/drm/drm_connector.h | 9 +++ > >> 10 files changed, 89 insertions(+), 26 deletions(-)
On Tue, 29 Nov 2022, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Jani, > > On Tue, Nov 29, 2022 at 11:29:45AM +0200, Jani Nikula wrote: >> On Fri, 25 Nov 2022, Harry Wentland <harry.wentland@amd.com> wrote: >> > On 10/5/22 06:51, Jani Nikula wrote: >> >> Currently i915 assumes all drm_connectors it encounters are embedded in >> >> intel_connectors that i915 allocated. The drm_writeback_connector forces >> >> a design where this is not the case; we can't provide our own connector, >> >> and writeback embeds the drm_connector it initializes itself. >> >> >> >> To use drm writeback, none of the i915 connector iteration could assume >> >> the drm connector is embedded in intel_connector. Checking this is >> >> tedious, and would require an intermediate step with >> >> drm_connector. Here's an idea I came up with; filtering at the drm >> >> connector iterator level with a caller supplied function. Not too much >> >> code, and could be used for other things as well. >> >> >> > >> > We've been trying to hook up drm_writeback_connector in amdgpu and >> > this would be really helpful. I've had to do liberal sprinkling >> > of "!= DRM_MODE_CONNECTOR_WRITEBACK" all over the place. >> > >> >> Mind you, we'd still much rather modify drm writeback to allow passing >> >> the connector i915 allocated, instead of the current midlayer design >> >> that forces drivers to a certain model. Working around this is a bunch >> >> of error prone and tedious code that we really could do without. >> > >> > I think this would be even better but also be much more work and impact >> > every driver that implements writeback. FWIW, there was no way for me >> > to add writeback connector handling without KASAN. Interpreting the >> > connector wrong in one place leads to memory corruption and >> > undefined behavior and is almost impossible to spot without KASAN. >> >> Laurent, I once again plead - could we please reconsider this and give >> drivers the option to allocate and provide the drm_connector to >> writeback themselves? >> >> All things considered I think that would be the safer option. > > I do think all this stems from the decision of using connectors for > writeback, and we're paying the price for it now. It's painful in > drivers, and that's why I would prefer hiding it from drivers and > handling it completely in the DRM core. Unfortunately, the current abstraction leaks to drivers in a way that's worse than just openly embracing the connector usage. :( And we still have to provide the struct drm_connector_funcs pointer to writeback just as well. > Ideally the connector enumeration API exposed to drivers should not > even enumerate the writeback connectors by default. > > I'm just a contributor to the subsystem, so I don't make the call. If > there's a general consensus it's better to require all drivers to handle > writeback connectors explicitly everywhere (Daniel and Dave may want to > chime in here), I can be overruled, like anybody else. Well, outside of i915 I'm just a contributor too, and I prefer trying to find consensus between us rather than seeking maintainers to overrule other contributors! Frankly, we pretty much already agreed to do what you originally suggested, even if we thought it was going to be a bunch of work, but we, and seems like AMD too, are finding it to also be quite error prone. It's just inherently clumsy to check the "sub class" type in C in runtime, or to even be sure when such checks aren't required. BR, Jani. > >> > This series is >> > Acked-by: Harry Wentland <harry.wentland@amd.com> >> > >> >> Cc: Arun R Murthy <arun.r.murthy@intel.com> >> >> Cc: Dave Airlie <airlied@gmail.com> >> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Cc: Suraj Kandpal <suraj.kandpal@intel.com> >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> >> >> Jani Nikula (2): >> >> drm/connector: add connector list iteration with filtering >> >> drm/i915: iterate intel_connectors only >> >> >> >> drivers/gpu/drm/drm_connector.c | 57 +++++++++++++++---- >> >> drivers/gpu/drm/i915/display/intel_display.c | 3 +- >> >> .../drm/i915/display/intel_display_types.h | 7 +++ >> >> drivers/gpu/drm/i915/display/intel_dp.c | 6 +- >> >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +- >> >> drivers/gpu/drm/i915/display/intel_hdcp.c | 3 +- >> >> drivers/gpu/drm/i915/display/intel_hotplug.c | 12 ++-- >> >> .../drm/i915/display/intel_modeset_setup.c | 6 +- >> >> drivers/gpu/drm/i915/display/intel_opregion.c | 9 ++- >> >> include/drm/drm_connector.h | 9 +++ >> >> 10 files changed, 89 insertions(+), 26 deletions(-)