| Message ID | 20220219002844.362157-1-marex@denx.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/bridge: Add support for selecting DSI host HS clock from DSI bridge | expand |
Hi, On Sat, Feb 19, 2022 at 01:28:37AM +0100, Marek Vasut wrote: > This patch series attempts to address a problem of missing support for DSI > bridge-to-bridge or panel-to-bridge clock frequency negotiation. The problem > has two variants. > > First, a DSI->to->x bridge derives its own internal clock from DSI HS clock, > but the DSI HS clock cannot be set to arbitrary values. TS358767 is one such > bridge in case it operates without Xtal. In that case, the TC358767 driver > must be able to negotiate the specific suitable DSI HS clock frequency for > the chip. > > Second, both DSI->to->x bridges and DSI hosts currently calculate, or rather > guess and hope they both guess the same number as their neighbor, the DSI HS > clock frequency from form of PLL=(width * height * bpp / lanes / 2). This is > dangerous, since the PLL capabilities on both ends of the DSI bus might differ > and the DSI host could easily end up generating wildly different clock than > what the DSI bridge/panel expects to receive. > > This series attempts to address these negotiation problems by extending the > existing .atomic_get_input_bus_fmts callback into .atomic_get_input_bus_cfgs > callback in struct drm_bridge_funcs {}. The extended version returns not only > a list of a list of bus formats supported by a bridge, but the entire list of > struct drm_bus_cfg, which currently contains format and bus flags, but can be > extended with other members, like desired clock frequency, as required. > > This series demonstrates such extension by adding the support for negotiating > the DSI clock and by implementing such support in DW DSI Host and TC358767 DSI > bridge. We discussed it a bit on IRC as well but there's another issue with this, let's imagine this setup: encoder -> DSI-to-DPI bridge -> DPI-to-HDMI bridge -> HDMI Monitor HDMI is fairly favorable, and it would probably use pixel clocks of either 148.5, 297 or 594MHz. Let's simplify this a bit and assume your DSI-to-DPI bridge can only operate at a frequency equivalent to 297MHz. 594Mhz is going to be used by those new fancy monitors, and thus the preferred mode is likely to be using 594MHz. With your solution, it effectively means that when the system will boot up, the preferred mode will be reported to the userspace (and the fbdev emulation), whatever is coming next is going to use it, and you're just going to... refuse it because it never worked in the first place. You'll leave a blank display, and that's it. That's not a great behavior, really. And since you don't get a state until you start a commit, this would need to be able to work without one. Of course, some state parameters will affect the clock (like the bpc count) so it won't be perfect, but we can at least try. Another thing is that the clock that needs to be negociated is likely to be device specific. It's probably going to be fairly similar across similar devices (like all the DSI bridges you mentioned are using the HS clock), but I'm not sure we can make that assumption. I think we could make something that work by asking the previous bridge in the chain for a given clock rate with a given mode, and then filter out / adjust anything we don't like. It would then be able to first check if it can provide that clock in the first place, and then the rate it has, and would be free to forward the query up to the encoder. And since it's tied to the mode, it would work with mode_valid too. Maxime
On 2/24/22 16:40, Maxime Ripard wrote: > Hi, Hi, > On Sat, Feb 19, 2022 at 01:28:37AM +0100, Marek Vasut wrote: >> This patch series attempts to address a problem of missing support for DSI >> bridge-to-bridge or panel-to-bridge clock frequency negotiation. The problem >> has two variants. >> >> First, a DSI->to->x bridge derives its own internal clock from DSI HS clock, >> but the DSI HS clock cannot be set to arbitrary values. TS358767 is one such >> bridge in case it operates without Xtal. In that case, the TC358767 driver >> must be able to negotiate the specific suitable DSI HS clock frequency for >> the chip. >> >> Second, both DSI->to->x bridges and DSI hosts currently calculate, or rather >> guess and hope they both guess the same number as their neighbor, the DSI HS >> clock frequency from form of PLL=(width * height * bpp / lanes / 2). This is >> dangerous, since the PLL capabilities on both ends of the DSI bus might differ >> and the DSI host could easily end up generating wildly different clock than >> what the DSI bridge/panel expects to receive. >> >> This series attempts to address these negotiation problems by extending the >> existing .atomic_get_input_bus_fmts callback into .atomic_get_input_bus_cfgs >> callback in struct drm_bridge_funcs {}. The extended version returns not only >> a list of a list of bus formats supported by a bridge, but the entire list of >> struct drm_bus_cfg, which currently contains format and bus flags, but can be >> extended with other members, like desired clock frequency, as required. >> >> This series demonstrates such extension by adding the support for negotiating >> the DSI clock and by implementing such support in DW DSI Host and TC358767 DSI >> bridge. > > We discussed it a bit on IRC as well but there's another issue with > this, let's imagine this setup: > > encoder -> DSI-to-DPI bridge -> DPI-to-HDMI bridge -> HDMI Monitor > > HDMI is fairly favorable, and it would probably use pixel clocks of > either 148.5, 297 or 594MHz. Let's simplify this a bit and assume your > DSI-to-DPI bridge can only operate at a frequency equivalent to 297MHz. > > 594Mhz is going to be used by those new fancy monitors, and thus the > preferred mode is likely to be using 594MHz. > > With your solution, it effectively means that when the system will boot > up, the preferred mode will be reported to the userspace (and the fbdev > emulation), whatever is coming next is going to use it, and you're just > going to... refuse it because it never worked in the first place. You'll > leave a blank display, and that's it. That's not a great behavior, > really. If you cannot support such a panel with this kind of scanout engine, what else would you do than blank screen ? > And since you don't get a state until you start a commit, this would > need to be able to work without one. Of course, some state parameters > will affect the clock (like the bpc count) so it won't be perfect, but > we can at least try. > > Another thing is that the clock that needs to be negociated is likely to > be device specific. It's probably going to be fairly similar across > similar devices (like all the DSI bridges you mentioned are using the HS > clock), but I'm not sure we can make that assumption. The bridge (data sink) should be able to figure out what kind of clock it needs from the source and then request those, yes. With DSI you can make an assumption about what kind of clock frequencies each link mode would require, but in general, you cannot assume much. > I think we could make something that work by asking the previous bridge > in the chain for a given clock rate with a given mode, and then filter > out / adjust anything we don't like. It would then be able to first > check if it can provide that clock in the first place, and then the rate > it has, and would be free to forward the query up to the encoder. And > since it's tied to the mode, it would work with mode_valid too. It seems to me this is similar to this solution, except it must happen when the mode is available ? But then the question also comes to mind, should select_bus_fmt_recursive() be called only after mode is available too ?
On Thu, Feb 24, 2022 at 09:24:57PM +0100, Marek Vasut wrote: > On 2/24/22 16:40, Maxime Ripard wrote: > > Hi, > > Hi, > > > On Sat, Feb 19, 2022 at 01:28:37AM +0100, Marek Vasut wrote: > > > This patch series attempts to address a problem of missing support for DSI > > > bridge-to-bridge or panel-to-bridge clock frequency negotiation. The problem > > > has two variants. > > > > > > First, a DSI->to->x bridge derives its own internal clock from DSI HS clock, > > > but the DSI HS clock cannot be set to arbitrary values. TS358767 is one such > > > bridge in case it operates without Xtal. In that case, the TC358767 driver > > > must be able to negotiate the specific suitable DSI HS clock frequency for > > > the chip. > > > > > > Second, both DSI->to->x bridges and DSI hosts currently calculate, or rather > > > guess and hope they both guess the same number as their neighbor, the DSI HS > > > clock frequency from form of PLL=(width * height * bpp / lanes / 2). This is > > > dangerous, since the PLL capabilities on both ends of the DSI bus might differ > > > and the DSI host could easily end up generating wildly different clock than > > > what the DSI bridge/panel expects to receive. > > > > > > This series attempts to address these negotiation problems by extending the > > > existing .atomic_get_input_bus_fmts callback into .atomic_get_input_bus_cfgs > > > callback in struct drm_bridge_funcs {}. The extended version returns not only > > > a list of a list of bus formats supported by a bridge, but the entire list of > > > struct drm_bus_cfg, which currently contains format and bus flags, but can be > > > extended with other members, like desired clock frequency, as required. > > > > > > This series demonstrates such extension by adding the support for negotiating > > > the DSI clock and by implementing such support in DW DSI Host and TC358767 DSI > > > bridge. > > > > We discussed it a bit on IRC as well but there's another issue with > > this, let's imagine this setup: > > > > encoder -> DSI-to-DPI bridge -> DPI-to-HDMI bridge -> HDMI Monitor > > > > HDMI is fairly favorable, and it would probably use pixel clocks of > > either 148.5, 297 or 594MHz. Let's simplify this a bit and assume your > > DSI-to-DPI bridge can only operate at a frequency equivalent to 297MHz. > > > > 594Mhz is going to be used by those new fancy monitors, and thus the > > preferred mode is likely to be using 594MHz. > > > > With your solution, it effectively means that when the system will boot > > up, the preferred mode will be reported to the userspace (and the fbdev > > emulation), whatever is coming next is going to use it, and you're just > > going to... refuse it because it never worked in the first place. You'll > > leave a blank display, and that's it. That's not a great behavior, > > really. > > If you cannot support such a panel with this kind of scanout engine, what > else would you do than blank screen ? An HDMI monitor typically supports multiple pixel clocks. If you can't reach one there's usually plenty of other options that could work. And if there's none, then so be it, we shouldn't report any either. > > And since you don't get a state until you start a commit, this would > > need to be able to work without one. Of course, some state parameters > > will affect the clock (like the bpc count) so it won't be perfect, but > > we can at least try. > > > > Another thing is that the clock that needs to be negociated is likely to > > be device specific. It's probably going to be fairly similar across > > similar devices (like all the DSI bridges you mentioned are using the HS > > clock), but I'm not sure we can make that assumption. > > The bridge (data sink) should be able to figure out what kind of clock it > needs from the source and then request those, yes. With DSI you can make an > assumption about what kind of clock frequencies each link mode would > require, but in general, you cannot assume much. You're modifying the core, you shouldn't be reasoning about DSI, but any display interface really. > > I think we could make something that work by asking the previous bridge > > in the chain for a given clock rate with a given mode, and then filter > > out / adjust anything we don't like. It would then be able to first > > check if it can provide that clock in the first place, and then the rate > > it has, and would be free to forward the query up to the encoder. And > > since it's tied to the mode, it would work with mode_valid too. > > It seems to me this is similar to this solution, except it must happen when > the mode is available ? The mode will pretty much always be available. You can't really reason about a clock rate without a mode. > But then the question also comes to mind, should > select_bus_fmt_recursive() be called only after mode is available too > ? Most of the drivers that use either atomic_get_input_bus_fmts or atomic_get_output_bus_fmts don't use the state at all, so yes. The only exception is the HDMI output format selection in dw-hdmi, but that's a bit of a mess across drivers, each one going its own separate way to achieve this. So, yeah. Usually formats are less important when it comes to whether a device can actually support a given mode than the clock it feeds from though, so it has a lesser impact. Maxime
This patch series attempts to address a problem of missing support for DSI bridge-to-bridge or panel-to-bridge clock frequency negotiation. The problem has two variants. First, a DSI->to->x bridge derives its own internal clock from DSI HS clock, but the DSI HS clock cannot be set to arbitrary values. TS358767 is one such bridge in case it operates without Xtal. In that case, the TC358767 driver must be able to negotiate the specific suitable DSI HS clock frequency for the chip. Second, both DSI->to->x bridges and DSI hosts currently calculate, or rather guess and hope they both guess the same number as their neighbor, the DSI HS clock frequency from form of PLL=(width * height * bpp / lanes / 2). This is dangerous, since the PLL capabilities on both ends of the DSI bus might differ and the DSI host could easily end up generating wildly different clock than what the DSI bridge/panel expects to receive. This series attempts to address these negotiation problems by extending the existing .atomic_get_input_bus_fmts callback into .atomic_get_input_bus_cfgs callback in struct drm_bridge_funcs {}. The extended version returns not only a list of a list of bus formats supported by a bridge, but the entire list of struct drm_bus_cfg, which currently contains format and bus flags, but can be extended with other members, like desired clock frequency, as required. This series demonstrates such extension by adding the support for negotiating the DSI clock and by implementing such support in DW DSI Host and TC358767 DSI bridge. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Maxime Ripard <maxime@cerno.tech> Cc: Neil Armstrong <narmstrong@baylibre.com> Cc: Sam Ravnborg <sam@ravnborg.org> TC358767 part depends on: https://patchwork.freedesktop.org/series/100372 Marek Vasut (7): drm/bridge: Pass struct drm_bus_cfg to select_bus_fmt_recursive() drm/bridge: Add new atomic_get_input_bus_cfgs callback drm/bridge: Extend struct drm_bus_cfg with clock field drm/bridge: dw-mipi-dsi: Move PLL setup into atomic_enable drm/bridge: dw-mipi-dsi: Pass bridge state into dw_mipi_dsi_get_lane_mbps() drm/bridge: dw-mipi-dsi: Prefer DSI bus clock settings from bridge_state drm/bridge: tc358767: Add support for PLL clock derivation from DSI HS clock drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 22 +++-- drivers/gpu/drm/bridge/tc358767.c | 97 +++++++++++++++---- drivers/gpu/drm/drm_bridge.c | 78 +++++++++++---- .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 1 + drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 17 ++-- include/drm/bridge/dw_mipi_dsi.h | 2 + include/drm/drm_atomic.h | 5 + include/drm/drm_bridge.h | 42 ++++++++ 8 files changed, 214 insertions(+), 50 deletions(-)