| Message ID | 20230814-kms-hdmi-connector-state-v1-0-048054df3654@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/connector: Create HDMI Connector infrastructure | expand |
On 14/08/2023 15:56, Maxime Ripard wrote: > Hi, > > Here's a series that creates a subclass of drm_connector specifically > targeted at HDMI controllers. > > The idea behind this series came from a recent discussion on IRC during > which we discussed infoframes generation of i915 vs everything else. > > Infoframes generation code still requires some decent boilerplate, with > each driver doing some variation of it. > > In parallel, while working on vc4, we ended up converting a lot of i915 > logic (mostly around format / bpc selection, and scrambler setup) to > apply on top of a driver that relies only on helpers. > > While currently sitting in the vc4 driver, none of that logic actually > relies on any driver or hardware-specific behaviour. > > The only missing piec to make it shareable are a bunch of extra > variables stored in a state (current bpc, format, RGB range selection, > etc.). > > Thus, I decided to create some generic subclass of drm_connector to > address HDMI connectors, with a bunch of helpers that will take care of > all the "HDMI Spec" related code. Scrambler setup is missing at the > moment but can easily be plugged in. > > Last week, Hans Verkuil also expressed interest in retrieving the > infoframes generated from userspace to create an infoframe-decode tool. > This series thus leverages the infoframe generation code to expose it > through debugfs. Some background here: I maintain the edid-decode utility to parse and verify EDIDs, and an infoframe-decode counterpart would be very nice. I can add support for exposing infoframes to debugfs in HDMI receivers as well, and that will help parse and verify received infoframes for correctness. I added the linux-media mailinglist as well, since this will be of interest for that subsystem as well. Regards, Hans > > This entire series is only build-tested at the moment. Let me know what > you think, > Maxime > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > --- > Maxime Ripard (13): > drm/connector: Introduce an HDMI connector > drm/connector: hdmi: Create a custom state > drm/connector: hdmi: Add Broadcast RGB property > drm/connector: hdmi: Add helper to get the RGB range > drm/connector: hdmi: Add output BPC to the connector state > drm/connector: hdmi: Add support for output format > drm/connector: hdmi: Calculate TMDS character rate > drm/connector: hdmi: Add custom hook to filter TMDS character rate > drm/connector: hdmi: Compute bpc and format automatically > drm/connector: hdmi: Add Infoframes generation > drm/connector: hdmi: Create Infoframe DebugFS entries > drm/vc4: hdmi: Create destroy state implementation > drm/vc4: hdmi: Switch to HDMI connector > > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/drm_hdmi_connector.c | 1112 ++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/vc4/vc4_hdmi.c | 720 ++++------------------ > drivers/gpu/drm/vc4/vc4_hdmi.h | 37 +- > drivers/gpu/drm/vc4/vc4_hdmi_phy.c | 4 +- > include/drm/drm_connector.h | 256 ++++++++ > 6 files changed, 1508 insertions(+), 622 deletions(-) > --- > base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4 > change-id: 20230814-kms-hdmi-connector-state-616787e67927 > > Best regards,
On Mon, Aug 14, 2023 at 03:56:12PM +0200, Maxime Ripard wrote: > Hi, > > Here's a series that creates a subclass of drm_connector specifically > targeted at HDMI controllers. > > The idea behind this series came from a recent discussion on IRC during > which we discussed infoframes generation of i915 vs everything else. > > Infoframes generation code still requires some decent boilerplate, with > each driver doing some variation of it. > > In parallel, while working on vc4, we ended up converting a lot of i915 > logic (mostly around format / bpc selection, and scrambler setup) to > apply on top of a driver that relies only on helpers. > > While currently sitting in the vc4 driver, none of that logic actually > relies on any driver or hardware-specific behaviour. > > The only missing piec to make it shareable are a bunch of extra > variables stored in a state (current bpc, format, RGB range selection, > etc.). > > Thus, I decided to create some generic subclass of drm_connector to > address HDMI connectors, with a bunch of helpers that will take care of > all the "HDMI Spec" related code. Scrambler setup is missing at the > moment but can easily be plugged in. > > Last week, Hans Verkuil also expressed interest in retrieving the > infoframes generated from userspace to create an infoframe-decode tool. > This series thus leverages the infoframe generation code to expose it > through debugfs. > > This entire series is only build-tested at the moment. Let me know what > you think, > Maxime I think the idea overall makes sense, we we probably need it to roll out actual hdmi support to all the hdmi drivers we have. But there's the eternal issue of "C sucks at multiple inheritance". Which means if you have a driver that subclasses drm_connector already for it's driver needs it defacto cannot, or only under some serious pains, use this. Which is kinda why in practice we tend to not subclass, but stuff subclass fields into a name sub-structure. So essentially struct drm_connector.hdmi and struct drm_connector_state.hdmi instead of drm_hdmi_connector and drm_hdmi_connector_state. The helper functions to set it all up would all still be the same roughly. It's less typesafe but I think the gain in practical use (like you could make i915 use the helpers probably, which with this approach here is practically impossible). The only other nit is that we probably want to put some of the hdmi properties into struct drm_mode_config because there's no reason to have per-connector valid values. Also, it might be really good if you can find a co-conspirator who also wants to use this in their driver, then with some i915 extracting we'd have three, which should ensure the helper api is solid. Cheers, Sima > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > --- > Maxime Ripard (13): > drm/connector: Introduce an HDMI connector > drm/connector: hdmi: Create a custom state > drm/connector: hdmi: Add Broadcast RGB property > drm/connector: hdmi: Add helper to get the RGB range > drm/connector: hdmi: Add output BPC to the connector state > drm/connector: hdmi: Add support for output format > drm/connector: hdmi: Calculate TMDS character rate > drm/connector: hdmi: Add custom hook to filter TMDS character rate > drm/connector: hdmi: Compute bpc and format automatically > drm/connector: hdmi: Add Infoframes generation > drm/connector: hdmi: Create Infoframe DebugFS entries > drm/vc4: hdmi: Create destroy state implementation > drm/vc4: hdmi: Switch to HDMI connector > > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/drm_hdmi_connector.c | 1112 ++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/vc4/vc4_hdmi.c | 720 ++++------------------ > drivers/gpu/drm/vc4/vc4_hdmi.h | 37 +- > drivers/gpu/drm/vc4/vc4_hdmi_phy.c | 4 +- > include/drm/drm_connector.h | 256 ++++++++ > 6 files changed, 1508 insertions(+), 622 deletions(-) > --- > base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4 > change-id: 20230814-kms-hdmi-connector-state-616787e67927 > > Best regards, > -- > Maxime Ripard <mripard@kernel.org> >
Hi, On Tue, Aug 22, 2023 at 04:16:08PM +0200, Daniel Vetter wrote: > On Mon, Aug 14, 2023 at 03:56:12PM +0200, Maxime Ripard wrote: > > Here's a series that creates a subclass of drm_connector specifically > > targeted at HDMI controllers. > > > > The idea behind this series came from a recent discussion on IRC during > > which we discussed infoframes generation of i915 vs everything else. > > > > Infoframes generation code still requires some decent boilerplate, with > > each driver doing some variation of it. > > > > In parallel, while working on vc4, we ended up converting a lot of i915 > > logic (mostly around format / bpc selection, and scrambler setup) to > > apply on top of a driver that relies only on helpers. > > > > While currently sitting in the vc4 driver, none of that logic actually > > relies on any driver or hardware-specific behaviour. > > > > The only missing piec to make it shareable are a bunch of extra > > variables stored in a state (current bpc, format, RGB range selection, > > etc.). > > > > Thus, I decided to create some generic subclass of drm_connector to > > address HDMI connectors, with a bunch of helpers that will take care of > > all the "HDMI Spec" related code. Scrambler setup is missing at the > > moment but can easily be plugged in. > > > > Last week, Hans Verkuil also expressed interest in retrieving the > > infoframes generated from userspace to create an infoframe-decode tool. > > This series thus leverages the infoframe generation code to expose it > > through debugfs. > > > > This entire series is only build-tested at the moment. Let me know what > > you think, > > I think the idea overall makes sense, we we probably need it to roll out > actual hdmi support to all the hdmi drivers we have. But there's the > eternal issue of "C sucks at multiple inheritance". > > Which means if you have a driver that subclasses drm_connector already for > it's driver needs it defacto cannot, or only under some serious pains, use > this. That's what vc4 is doing, and it went fine I think? it was mostly a matter of subclassing drm_hdmi_connector instead of drm_connector, and adjusting the various pointers and accessors here and there. It does create a fairly big diffstat, but nothing too painful. > Which is kinda why in practice we tend to not subclass, but stuff > subclass fields into a name sub-structure. So essentially struct > drm_connector.hdmi and struct drm_connector_state.hdmi instead of > drm_hdmi_connector and drm_hdmi_connector_state. The helper functions to > set it all up would all still be the same roughly. It's less typesafe but > I think the gain in practical use (like you could make i915 use the > helpers probably, which with this approach here is practically > impossible). Ack. > The only other nit is that we probably want to put some of the hdmi > properties into struct drm_mode_config because there's no reason to have > per-connector valid values. What property would you want to move? > Also, it might be really good if you can find a co-conspirator who also > wants to use this in their driver, then with some i915 extracting we'd > have three, which should ensure the helper api is solid. I can convert sunxi (old) HDMI driver if needed. I'm not sure how helpful it would be since it doesn't support bpc > 8, but it could be a nice showcase still for "simple" HDMI controllers. Maxime
On Tue, Aug 22, 2023 at 04:35:55PM +0200, Maxime Ripard wrote: > Hi, > > On Tue, Aug 22, 2023 at 04:16:08PM +0200, Daniel Vetter wrote: > > On Mon, Aug 14, 2023 at 03:56:12PM +0200, Maxime Ripard wrote: > > > Here's a series that creates a subclass of drm_connector specifically > > > targeted at HDMI controllers. > > > > > > The idea behind this series came from a recent discussion on IRC during > > > which we discussed infoframes generation of i915 vs everything else. > > > > > > Infoframes generation code still requires some decent boilerplate, with > > > each driver doing some variation of it. > > > > > > In parallel, while working on vc4, we ended up converting a lot of i915 > > > logic (mostly around format / bpc selection, and scrambler setup) to > > > apply on top of a driver that relies only on helpers. > > > > > > While currently sitting in the vc4 driver, none of that logic actually > > > relies on any driver or hardware-specific behaviour. > > > > > > The only missing piec to make it shareable are a bunch of extra > > > variables stored in a state (current bpc, format, RGB range selection, > > > etc.). > > > > > > Thus, I decided to create some generic subclass of drm_connector to > > > address HDMI connectors, with a bunch of helpers that will take care of > > > all the "HDMI Spec" related code. Scrambler setup is missing at the > > > moment but can easily be plugged in. > > > > > > Last week, Hans Verkuil also expressed interest in retrieving the > > > infoframes generated from userspace to create an infoframe-decode tool. > > > This series thus leverages the infoframe generation code to expose it > > > through debugfs. > > > > > > This entire series is only build-tested at the moment. Let me know what > > > you think, > > > > I think the idea overall makes sense, we we probably need it to roll out > > actual hdmi support to all the hdmi drivers we have. But there's the > > eternal issue of "C sucks at multiple inheritance". > > > > Which means if you have a driver that subclasses drm_connector already for > > it's driver needs it defacto cannot, or only under some serious pains, use > > this. > > That's what vc4 is doing, and it went fine I think? it was mostly a > matter of subclassing drm_hdmi_connector instead of drm_connector, and > adjusting the various pointers and accessors here and there. > > It does create a fairly big diffstat, but nothing too painful. Yeah it's the massive churn that's the pain for refactoring existing bigger drivers. Plus what do you do when you both need a hdmi connector and a dp connector (or a writeback connector). > > Which is kinda why in practice we tend to not subclass, but stuff > > subclass fields into a name sub-structure. So essentially struct > > drm_connector.hdmi and struct drm_connector_state.hdmi instead of > > drm_hdmi_connector and drm_hdmi_connector_state. The helper functions to > > set it all up would all still be the same roughly. It's less typesafe but > > I think the gain in practical use (like you could make i915 use the > > helpers probably, which with this approach here is practically > > impossible). > > Ack. > > > The only other nit is that we probably want to put some of the hdmi > > properties into struct drm_mode_config because there's no reason to have > > per-connector valid values. > > What property would you want to move? The rgb broadcast property looked very much like it's connector invariant. Just the one I noticed, I didn't check all the others. > > Also, it might be really good if you can find a co-conspirator who also > > wants to use this in their driver, then with some i915 extracting we'd > > have three, which should ensure the helper api is solid. > > I can convert sunxi (old) HDMI driver if needed. I'm not sure how > helpful it would be since it doesn't support bpc > 8, but it could be a > nice showcase still for "simple" HDMI controllers. Yeah that might be good. Or perhaps poke Rob Clark whether msm is interested and someone could do a conversion for dpu5 or so? Cheers, Sima
On Tue, 22 Aug 2023, Maxime Ripard <mripard@kernel.org> wrote: > Hi, > > On Tue, Aug 22, 2023 at 04:16:08PM +0200, Daniel Vetter wrote: >> On Mon, Aug 14, 2023 at 03:56:12PM +0200, Maxime Ripard wrote: >> > Here's a series that creates a subclass of drm_connector specifically >> > targeted at HDMI controllers. >> > >> > The idea behind this series came from a recent discussion on IRC during >> > which we discussed infoframes generation of i915 vs everything else. >> > >> > Infoframes generation code still requires some decent boilerplate, with >> > each driver doing some variation of it. >> > >> > In parallel, while working on vc4, we ended up converting a lot of i915 >> > logic (mostly around format / bpc selection, and scrambler setup) to >> > apply on top of a driver that relies only on helpers. >> > >> > While currently sitting in the vc4 driver, none of that logic actually >> > relies on any driver or hardware-specific behaviour. >> > >> > The only missing piec to make it shareable are a bunch of extra >> > variables stored in a state (current bpc, format, RGB range selection, >> > etc.). >> > >> > Thus, I decided to create some generic subclass of drm_connector to >> > address HDMI connectors, with a bunch of helpers that will take care of >> > all the "HDMI Spec" related code. Scrambler setup is missing at the >> > moment but can easily be plugged in. >> > >> > Last week, Hans Verkuil also expressed interest in retrieving the >> > infoframes generated from userspace to create an infoframe-decode tool. >> > This series thus leverages the infoframe generation code to expose it >> > through debugfs. >> > >> > This entire series is only build-tested at the moment. Let me know what >> > you think, >> >> I think the idea overall makes sense, we we probably need it to roll out >> actual hdmi support to all the hdmi drivers we have. But there's the >> eternal issue of "C sucks at multiple inheritance". >> >> Which means if you have a driver that subclasses drm_connector already for >> it's driver needs it defacto cannot, or only under some serious pains, use >> this. > > That's what vc4 is doing, and it went fine I think? it was mostly a > matter of subclassing drm_hdmi_connector instead of drm_connector, and > adjusting the various pointers and accessors here and there. > > It does create a fairly big diffstat, but nothing too painful. The main pain point is not the diffstat per se, but that *all* casts to subclass need to check what the connector type is before doing so. You'll also get fun NULL conditions that you need to check and handle if the type isn't what you'd like it to be. Currently i915 can just assume all drm_connectors it encounters are intel_connectors that it created, always. Basically this has blocked the writeback connector stuff for a few years now in i915, because writeback forces a different subclassing, and what should be a small change in i915 turns into huge churn. BR, Jani. > >> Which is kinda why in practice we tend to not subclass, but stuff >> subclass fields into a name sub-structure. So essentially struct >> drm_connector.hdmi and struct drm_connector_state.hdmi instead of >> drm_hdmi_connector and drm_hdmi_connector_state. The helper functions to >> set it all up would all still be the same roughly. It's less typesafe but >> I think the gain in practical use (like you could make i915 use the >> helpers probably, which with this approach here is practically >> impossible). > > Ack. > >> The only other nit is that we probably want to put some of the hdmi >> properties into struct drm_mode_config because there's no reason to have >> per-connector valid values. > > What property would you want to move? > >> Also, it might be really good if you can find a co-conspirator who also >> wants to use this in their driver, then with some i915 extracting we'd >> have three, which should ensure the helper api is solid. > > I can convert sunxi (old) HDMI driver if needed. I'm not sure how > helpful it would be since it doesn't support bpc > 8, but it could be a > nice showcase still for "simple" HDMI controllers. > > Maxime
On Tue, Aug 22, 2023 at 05:51:39PM +0300, Jani Nikula wrote: > On Tue, 22 Aug 2023, Maxime Ripard <mripard@kernel.org> wrote: > > Hi, > > > > On Tue, Aug 22, 2023 at 04:16:08PM +0200, Daniel Vetter wrote: > >> On Mon, Aug 14, 2023 at 03:56:12PM +0200, Maxime Ripard wrote: > >> > Here's a series that creates a subclass of drm_connector specifically > >> > targeted at HDMI controllers. > >> > > >> > The idea behind this series came from a recent discussion on IRC during > >> > which we discussed infoframes generation of i915 vs everything else. > >> > > >> > Infoframes generation code still requires some decent boilerplate, with > >> > each driver doing some variation of it. > >> > > >> > In parallel, while working on vc4, we ended up converting a lot of i915 > >> > logic (mostly around format / bpc selection, and scrambler setup) to > >> > apply on top of a driver that relies only on helpers. > >> > > >> > While currently sitting in the vc4 driver, none of that logic actually > >> > relies on any driver or hardware-specific behaviour. > >> > > >> > The only missing piec to make it shareable are a bunch of extra > >> > variables stored in a state (current bpc, format, RGB range selection, > >> > etc.). > >> > > >> > Thus, I decided to create some generic subclass of drm_connector to > >> > address HDMI connectors, with a bunch of helpers that will take care of > >> > all the "HDMI Spec" related code. Scrambler setup is missing at the > >> > moment but can easily be plugged in. > >> > > >> > Last week, Hans Verkuil also expressed interest in retrieving the > >> > infoframes generated from userspace to create an infoframe-decode tool. > >> > This series thus leverages the infoframe generation code to expose it > >> > through debugfs. > >> > > >> > This entire series is only build-tested at the moment. Let me know what > >> > you think, > >> > >> I think the idea overall makes sense, we we probably need it to roll out > >> actual hdmi support to all the hdmi drivers we have. But there's the > >> eternal issue of "C sucks at multiple inheritance". > >> > >> Which means if you have a driver that subclasses drm_connector already for > >> it's driver needs it defacto cannot, or only under some serious pains, use > >> this. > > > > That's what vc4 is doing, and it went fine I think? it was mostly a > > matter of subclassing drm_hdmi_connector instead of drm_connector, and > > adjusting the various pointers and accessors here and there. > > > > It does create a fairly big diffstat, but nothing too painful. > > The main pain point is not the diffstat per se, but that *all* casts to > subclass need to check what the connector type is before doing > so. You'll also get fun NULL conditions that you need to check and > handle if the type isn't what you'd like it to be. > > Currently i915 can just assume all drm_connectors it encounters are > intel_connectors that it created, always. > > Basically this has blocked the writeback connector stuff for a few years > now in i915, because writeback forces a different subclassing, and what > should be a small change in i915 turns into huge churn. Yeah after the writeback experience I'm heavily leaning towards "this was a mistake". For writeback we could refactor it I think by just moving it all (which I hope isn't too much churn), and then removing the then empty types (which is where the big churn kicks in, so maybe just add that to gpu/todo.rst). Cheers, Sima > > BR, > Jani. > > > > > >> Which is kinda why in practice we tend to not subclass, but stuff > >> subclass fields into a name sub-structure. So essentially struct > >> drm_connector.hdmi and struct drm_connector_state.hdmi instead of > >> drm_hdmi_connector and drm_hdmi_connector_state. The helper functions to > >> set it all up would all still be the same roughly. It's less typesafe but > >> I think the gain in practical use (like you could make i915 use the > >> helpers probably, which with this approach here is practically > >> impossible). > > > > Ack. > > > >> The only other nit is that we probably want to put some of the hdmi > >> properties into struct drm_mode_config because there's no reason to have > >> per-connector valid values. > > > > What property would you want to move? > > > >> Also, it might be really good if you can find a co-conspirator who also > >> wants to use this in their driver, then with some i915 extracting we'd > >> have three, which should ensure the helper api is solid. > > > > I can convert sunxi (old) HDMI driver if needed. I'm not sure how > > helpful it would be since it doesn't support bpc > 8, but it could be a > > nice showcase still for "simple" HDMI controllers. > > > > Maxime > > -- > Jani Nikula, Intel Open Source Graphics Center
Hi, Here's a series that creates a subclass of drm_connector specifically targeted at HDMI controllers. The idea behind this series came from a recent discussion on IRC during which we discussed infoframes generation of i915 vs everything else. Infoframes generation code still requires some decent boilerplate, with each driver doing some variation of it. In parallel, while working on vc4, we ended up converting a lot of i915 logic (mostly around format / bpc selection, and scrambler setup) to apply on top of a driver that relies only on helpers. While currently sitting in the vc4 driver, none of that logic actually relies on any driver or hardware-specific behaviour. The only missing piec to make it shareable are a bunch of extra variables stored in a state (current bpc, format, RGB range selection, etc.). Thus, I decided to create some generic subclass of drm_connector to address HDMI connectors, with a bunch of helpers that will take care of all the "HDMI Spec" related code. Scrambler setup is missing at the moment but can easily be plugged in. Last week, Hans Verkuil also expressed interest in retrieving the infoframes generated from userspace to create an infoframe-decode tool. This series thus leverages the infoframe generation code to expose it through debugfs. This entire series is only build-tested at the moment. Let me know what you think, Maxime Signed-off-by: Maxime Ripard <mripard@kernel.org> --- Maxime Ripard (13): drm/connector: Introduce an HDMI connector drm/connector: hdmi: Create a custom state drm/connector: hdmi: Add Broadcast RGB property drm/connector: hdmi: Add helper to get the RGB range drm/connector: hdmi: Add output BPC to the connector state drm/connector: hdmi: Add support for output format drm/connector: hdmi: Calculate TMDS character rate drm/connector: hdmi: Add custom hook to filter TMDS character rate drm/connector: hdmi: Compute bpc and format automatically drm/connector: hdmi: Add Infoframes generation drm/connector: hdmi: Create Infoframe DebugFS entries drm/vc4: hdmi: Create destroy state implementation drm/vc4: hdmi: Switch to HDMI connector drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_hdmi_connector.c | 1112 ++++++++++++++++++++++++++++++++++ drivers/gpu/drm/vc4/vc4_hdmi.c | 720 ++++------------------ drivers/gpu/drm/vc4/vc4_hdmi.h | 37 +- drivers/gpu/drm/vc4/vc4_hdmi_phy.c | 4 +- include/drm/drm_connector.h | 256 ++++++++ 6 files changed, 1508 insertions(+), 622 deletions(-) --- base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4 change-id: 20230814-kms-hdmi-connector-state-616787e67927 Best regards,