Hi,
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
>
> To prepare for DRM_BRIDGE_ATTACH_NO_CONNECTOR support,
> fix so the bpc is found using the output format.
>
> This avoids the use of the connector stored in the private data.
>
> Signed-off-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> Cc: Robert Foss <robert.foss@xxxxxxxxxx>
> Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Jonas Karlman <jonas@xxxxxxxxx>
> Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index d681ab68205c..dc6ec40bc1ef 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -33,6 +33,7 @@
> #include <drm/drm_panel.h>
> #include <drm/drm_print.h>
> #include <drm/drm_probe_helper.h>
> +#include <drm/media-bus-format.h>
>
> #define SN_DEVICE_REV_REG 0x08
> #define SN_DPPLL_SRC_REG 0x0A
> @@ -823,9 +824,11 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> }
>
> -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> +static unsigned int ti_sn_bridge_get_bpp(struct drm_bridge_state *bridge_state)
> {
> - if (pdata->connector.display_info.bpc <= 6)
> + int bpc = media_bus_format_to_bpc(bridge_state->output_bus_cfg.format);
> +
> + if (bpc <= 6)
> return 18;
> else
> return 24;
Unfortunately this doesn't work as hoped. :(
`bridge_state->output_bus_cfg.format` is 0 in my testing...
...and then media_bus_format_to_bpc() returns an error, which is
negative and <= 6. That's not super ideal...
I guess this gets down to what the output bus format is _supposed_ to
be for eDP. Based on my understanding of eDP there isn't really a
concept of a fixed "bus format" that the panel ought to work at. There
is a concept of the number of bits per component (6, 8, 10, 12) that a
panel can run at but then after that I believe the format is standard,
or at least it's something that's dynamic / negotiated. Also note that
the format on the bus is more related to the current mode we're
running the panel in than some inherent property of the panel. For
instance, a 10 bpc panel can likely have data provided in 8 bpc and 6
bpc. I've also seen 6 bpc panels that can accept 8 bpc data and can
dither it. In any case, this type of stuff is really all dynamic for
eDP. The old value we were looking at was really being interpreted as
the "max" bpc that the panel could run in and we'd choose to run the
panel in 8 bpc if the panel supported it and 6 bpc otherwise (this
bridge chip only supports 6bpc or 8bpc).
So I guess the question is: how do we move forward here?
Do we need to somehow figure out how to get "output_bus_cfg.format"
filled in? Any suggestions for how to do that? Do we just implement
atomic_get_output_bus_fmts() and then pick one of
MEDIA_BUS_FMT_RGB666_1X18 or MEDIA_BUS_FMT_RBG888_1X24 based on the
bpc in the connector state? ...or do we just list both of them and
something magically will pick the right one? Hrm, I tried that and it
didn't magically work, but I didn't debug further...
One thing I realized is that, at least in theory, we might not know
whether we want to run in 6 bpc or 8 bpc until we've done link
training. It's at least somewhat plausible that there could be edge
cases where we'd want to fall back to the low bpc if link training
failed at the higher bpc. The driver doesn't support that right now,
but ideally the design wouldn't preclude it. At the moment link
training happens in enable. Maybe that's a problem to worry about
another day, though?
-Doug
