Hi Boris,
Thank you for the patch.
On Thu, Aug 08, 2019 at 05:11:45PM +0200, Boris Brezillon wrote:
> This takes the form of various helpers, plus the addition of 2 new
> structs:
> * drm_bus_caps: describe the bus capabilities of a bridge/encoder. For
> bridges we have one for the input port and one for the output port.
> Encoders just have one output port.
> * drm_bus_cfg: added to the drm_bridge_state. It's supposed to store
> bus configuration information. For now we just have format and flags
> but more fields might be added in the future. Again, each
> drm_bridge_state contains 2 drm_bus_cfg elements: one for the output
> port and one for the input port
>
> Helpers can be used from bridge drivers' ->atomic_check() implementation
> to negotiate the bus format to use. Negotiation happens in reserve order,
s/reserve/reverse/
> starting from the last element of the bridge chain (the one directly
> connected to the display) to the first element of the chain (the one
> connected to the encoder).
>
> Negotiation usually takes place in 2 steps:
> * make sure the input end of the next element in the chain picked a
> format that's supported by the output end of our bridge. That's done
> with drm_atomic_bridge_choose_output_bus_cfg().
> * choose a format for the input end of our bridge based on what's
> supported by the previous bridge in the chain. This is achieved by
> calling drm_atomic_bridge_choose_input_bus_cfg()
>
> Note that those helpers are a bit lax when it comes to unitialized
> bus format validation. That's intentional. If we don't do that we'll
> basically break things if we start adding support for bus format
> negotiation to some elements of the pipeline leaving others on the
> side, and that's likely to happen for all external bridges since we
> can't necessarily tell where they are used.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_bridge.c | 187 +++++++++++++++++++++++++++++++++++
> include/drm/drm_bridge.h | 46 +++++++++
> include/drm/drm_encoder.h | 6 ++
> 3 files changed, 239 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 9efb27087e70..ef072df42422 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -602,6 +602,193 @@ void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder,
> }
> EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
>
> +int drm_find_best_bus_format(const struct drm_bus_caps *a,
> + const struct drm_bus_caps *b,
> + const struct drm_display_mode *mode,
> + u32 *selected_bus_fmt)
> +{
> + unsigned int i, j;
> +
> + /*
> + * Some drm_bridge/drm_encoder don't care about the input/output bus
> + * format, let's set the format to zero in that case (this is only
> + * valid if both side of the link don't care).
> + */
> + if (!a->num_supported_fmts && !b->num_supported_fmts) {
> + *selected_bus_fmt = 0;
> + return 0;
> + } else if (b->num_supported_fmts > 1 && b->supported_fmts) {
> + *selected_bus_fmt = b->supported_fmts[0];
> + return 0;
> + } else if (a->num_supported_fmts > 1 && a->supported_fmts) {
> + *selected_bus_fmt = a->supported_fmts[0];
> + return 0;
> + } else if (!a->num_supported_fmts || !a->supported_fmts ||
> + !b->num_supported_fmts || !b->supported_fmts) {
> + return -EINVAL;
> + }
> +
> + /*
> + * TODO:
> + * Dummy algo picking the first match. We probably want something
> + * smarter where the narrowest format (in term of bus width) that
> + * does not incur data loss is picked, and if all possible formats
> + * are lossy, pick the one that's less lossy among all the choices
> + * we have. In order to do that we'd need to convert MEDIA_BUS_FMT_
> + * modes into something like drm_format_info.
> + */
> + for (i = 0; i < a->num_supported_fmts; i++) {
> + for (j = 0; j < b->num_supported_fmts; j++) {
> + if (a->supported_fmts[i] == b->supported_fmts[j]) {
> + *selected_bus_fmt = a->supported_fmts[i];
> + return 0;
> + }
> + }
> + }
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_find_best_bus_format);
> +
> +/**
> + * drm_atomic_bridge_choose_input_bus_cfg() - Choose bus config for the input
> + * end
> + * @bridge_state: bridge state
> + * @crtc_state: CRTC state
> + * @conn_state: connector state
> + *
> + * Choose a bus format for the input side of a bridge based on what the
> + * previous bridge in the chain and this bridge support. Can be called from
> + * bridge drivers' &drm_bridge_funcs.atomic_check() implementation.
> + *
> + * RETURNS:
> + * 0 if a matching format was found, a negative error code otherwise
> + */
> +int
> +drm_atomic_bridge_choose_input_bus_cfg(struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct drm_bridge *self = bridge_state->bridge;
> + struct drm_bus_caps *prev_output_bus_caps;
> + struct drm_bridge *prev;
> + int ret;
> +
> + prev = drm_bridge_chain_get_prev_bridge(self);
> + if (!prev)
> + prev_output_bus_caps = &self->encoder->output_bus_caps;
> + else
> + prev_output_bus_caps = &prev->output_bus_caps;
> +
> + ret = drm_find_best_bus_format(prev_output_bus_caps,
> + &self->input_bus_caps, &crtc_state->mode,
> + &bridge_state->input_bus_cfg.fmt);
> + if (ret)
> + return ret;
I think most bridges that support multiple input and output formats will
not necessarily be able to transcode, meaning that for a particular
output format a particular corresponding input format will need to be
picked (or possibly one of a subset of input formats). I'm thus not sure
how useful this helper will be in practice.
I also fear that this negotiation mechanism is too simple and will lock
is with support for very simple systems only :-S Let's assume a system
with a display device (D), two bridges (B0 and B1) and a connector (C).
Let's further assume that the bus format required by the connector is
fixed. With this patch series, B1 will pick an output format identical
to the format on the connector, and will then pick an input format among
the ones supported by both B0 and B1. Let's assume there are multiple
options, Fa and Fb, and drm_find_best_bus_format() will pick one of
them, say Fa. Then B0 will configure its output with that format, and
will proceed with finding an input format supported by D. It could be
that the output format Fa for B0 requires an input format that is not
supported by D, while Fb would have produced a working pipeline.
I don't think this example is really far-fetched, and while we don't
need to support it immediately, we need an API that makes it possible to
support it, as changing the API later would require reworking all the
drivers that use it.
> +
> + /*
> + * TODO:
> + * Should we fill/check the ->flag field too?
> + */
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_bridge_choose_input_bus_cfg);
> +
> +/**
> + * drm_atomic_bridge_choose_output_bus_cfg() - Choose bus config for the output
> + * end
> + * @bridge_state: bridge state
> + * @crtc_state: CRTC state
> + * @conn_state: connector state
> + *
> + * Choose a bus format for the output side of a bridge based on what the next
> + * bridge in the chain and this bridge support. Can be called from bridge
> + * drivers' &drm_bridge_funcs.atomic_check() implementation.
> + *
> + * RETURNS:
> + * 0 if a matching format was found, a negative error code otherwise
> + */
> +int
> +drm_atomic_bridge_choose_output_bus_cfg(struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct drm_atomic_state *state = crtc_state->state;
> + struct drm_bridge *self = bridge_state->bridge;
> + struct drm_bridge_state *next_bridge_state;
> + struct drm_bridge *next;
> + u32 fmt;
> +
> + next = drm_bridge_chain_get_next_bridge(self);
> + if (!next) {
> + struct drm_connector *conn = conn_state->connector;
> + struct drm_display_info *di = &conn->display_info;
> +
> + /*
> + * FIXME:
> + * We currently pick the first supported format
> + * unconditionally. It seems to fit all current use cases but
> + * might be too limited for panels/displays that can configure
> + * the bus format dynamically.
> + */
> + if (di->num_bus_formats && di->bus_formats)
> + bridge_state->output_bus_cfg.fmt = di->bus_formats[0];
> + else
> + bridge_state->output_bus_cfg.fmt = 0;
What is the bridge driver supposed to do in this case ?
> +
> + bridge_state->output_bus_cfg.flags = di->bus_flags;
> + return 0;
> + }
> +
> + /*
> + * We expect output_bus_caps to contain at least one format. Note
> + * that don't care about bus format negotiation can simply not
> + * call this helper.
I'm not sure to parse the second sentence correctly.
> + */
> + if (!self->output_bus_caps.num_supported_fmts ||!
Extra ! at the end of the line ?
> + !self->output_bus_caps.supported_fmts)
> + return -EINVAL;
> +
> + next_bridge_state = drm_atomic_get_new_bridge_state(state, next);
> +
> + /*
> + * The next bridge is expected to have called
> + * &drm_atomic_bridge_choose_input_bus_cfg() as part of its
> + * &drm_bridge_funcs.atomic_check() implementation, but this hook is
> + * optional, and even if it's implemented, calling
> + * &drm_atomic_bridge_choose_input_bus_cfg() is not mandated.
> + * If fmt is zero, that means the next element in the chain doesn't
> + * care about bus format negotiation (probably because there's only
> + * one possible setting). In that case, we still have to select one
> + * bus format for the output port of our bridge, and this is only
> + * possible if the bridge supports only one format.
Theoritically we could also just pick the first one (or actually any of
the supported formats), couldn't we ?
> + */
> + fmt = next_bridge_state->input_bus_cfg.fmt;
> + if (fmt) {
> + unsigned int i;
> +
> + for (i = 0; i < self->output_bus_caps.num_supported_fmts; i++) {
> + if (self->output_bus_caps.supported_fmts[i] == fmt)
> + break;
> + }
> +
> + if (i == self->output_bus_caps.num_supported_fmts)
> + return -ENOTSUPP;
> + } else if (self->output_bus_caps.num_supported_fmts == 1) {
> + fmt = self->output_bus_caps.supported_fmts[0];
> + } else {
> + return -EINVAL;
> + }
> +
> + /*
> + * TODO:
> + * Should we fill/check the ->flag field too?
> + */
> + bridge_state->output_bus_cfg.fmt = fmt;
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_bridge_choose_output_bus_cfg);
> +
> static int drm_atomic_bridge_check(struct drm_bridge *bridge,
> struct drm_crtc_state *crtc_state,
> struct drm_connector_state *conn_state)
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 2f69adb7b0f3..c578800a05e0 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -33,15 +33,45 @@ struct drm_bridge;
> struct drm_bridge_timings;
> struct drm_panel;
>
> +/**
> + * struct drm_bus_cfg - bus configuration
> + * @fmt: format used on this bus
> + * @flags: DRM_BUS_ flags used on this bus
> + *
> + * Encodes the bus format and bus flags used by one end of the bridge or
> + * by the encoder output.
> + */
> +struct drm_bus_cfg {
> + u32 fmt;
> + u32 flags;
> +};
> +
> +/**
> + * struct drm_bus_caps - bus capabilities
> + * @supported_fmts: array of MEDIA_BUS_FMT_ formats
> + * @num_supported_fmts: size of the supported_fmts array
> + *
> + * Used by the core to negotiate the bus format at runtime.
> + */
> +struct drm_bus_caps {
> + const u32 *supported_fmts;
> + unsigned int num_supported_fmts;
> +};
> +
> /**
> * struct drm_bridge_state - Atomic bridge state object
> * @base: inherit from &drm_private_state
> * @bridge: the bridge this state refers to
> + * @input_bus_info: input bus information
> + * @output_bus_info: output bus information
I would make this an array, to prepare for bridges that will have more
than one input or one output.
> */
> struct drm_bridge_state {
> struct drm_private_state base;
>
> struct drm_bridge *bridge;
> +
> + struct drm_bus_cfg input_bus_cfg;
> + struct drm_bus_cfg output_bus_cfg;
> };
>
> static inline struct drm_bridge_state *
> @@ -465,6 +495,9 @@ struct drm_bridge {
> const struct drm_bridge_funcs *funcs;
> /** @driver_private: pointer to the bridge driver's internal context */
> void *driver_private;
> +
> + struct drm_bus_caps input_bus_caps;
> + struct drm_bus_caps output_bus_caps;
> };
>
> static inline struct drm_bridge *
> @@ -479,6 +512,14 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> struct drm_bridge *previous);
>
> +int
> +drm_atomic_bridge_choose_input_bus_cfg(struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state);
> +int
> +drm_atomic_bridge_choose_output_bus_cfg(struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state);
> struct drm_bridge *
> drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder);
> struct drm_bridge *
> @@ -562,6 +603,11 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state,
> return drm_priv_to_bridge_state(obj_state);
> }
>
> +int drm_find_best_bus_format(const struct drm_bus_caps *a,
> + const struct drm_bus_caps *b,
> + const struct drm_display_mode *mode,
> + u32 *selected_bus_fmt);
> +
> #ifdef CONFIG_DRM_PANEL_BRIDGE
> struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> u32 connector_type);
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 0899f7f34e3a..e891921b3aed 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -25,6 +25,7 @@
>
> #include <linux/list.h>
> #include <linux/ctype.h>
> +#include <drm/drm_bridge.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_mode.h>
> #include <drm/drm_mode_object.h>
> @@ -184,6 +185,11 @@ struct drm_encoder {
> * functions as part of its encoder enable/disable handling.
> */
> uint32_t custom_bridge_enable_disable_seq : 1;
> +
> + /**
> + * @output_bus_caps: Supported output bus caps
> + */
> + struct drm_bus_caps output_bus_caps;
> };
>
> #define obj_to_encoder(x) container_of(x, struct drm_encoder, base)
>
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
