On 26/08/2019 17:26, 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, > 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 uninitialized > 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.
I'll wait Laurent's feedback on this one, but I implemented the DW-HDMI negotiation with it, and it works fine, see [1] Tested-by: Neil Armstrong <narmstr...@baylibre.com> [1] https://patchwork.freedesktop.org/patch/msgid/20190827081425.15011-4-narmstr...@baylibre.com > > Signed-off-by: Boris Brezillon <boris.brezil...@collabora.com> > --- > Changes in v2: > * Rework things to support more complex use cases > --- > drivers/gpu/drm/drm_bridge.c | 179 ++++++++++++++++++++++++++++++++++- > include/drm/drm_bridge.h | 82 ++++++++++++++++ > 2 files changed, 260 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 9c74b285da9d..b53732ac997d 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -624,13 +624,184 @@ static int drm_atomic_bridge_check(struct drm_bridge > *bridge, > return 0; > } > > +static int select_bus_fmt_recursive(struct drm_bridge *first, > + struct drm_bridge *cur, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + u32 out_bus_fmt) > +{ > + struct drm_bridge_state *cur_state; > + unsigned int num_in_bus_fmts, i; > + struct drm_bridge *prev; > + u32 *in_bus_fmts; > + int ret; > + > + prev = drm_bridge_chain_get_prev_bridge(cur); > + cur_state = drm_atomic_get_new_bridge_state(crtc_state->state, cur); > + if (WARN_ON(!cur_state)) > + return -EINVAL; > + > + /* > + * Bus format negotiation is not supported by this bridge, let's pass > + * MEDIA_BUS_FMT_FIXED to the previous bridge in the chain and hope > + * that it can handle this situation gracefully (by providing > + * appropriate default values). > + */ > + if (!cur->funcs->atomic_get_input_bus_fmts) { > + if (cur != first) { > + ret = select_bus_fmt_recursive(first, prev, crtc_state, > + conn_state, > + MEDIA_BUS_FMT_FIXED); > + if (ret) > + return ret; > + } > + > + cur_state->input_bus_cfg.fmt = MEDIA_BUS_FMT_FIXED; > + cur_state->output_bus_cfg.fmt = out_bus_fmt; > + return 0; > + } > + > + cur->funcs->atomic_get_input_bus_fmts(cur, cur_state, crtc_state, > + conn_state, out_bus_fmt, > + &num_in_bus_fmts, NULL); > + if (!num_in_bus_fmts) > + return -ENOTSUPP; > + > + in_bus_fmts = kcalloc(num_in_bus_fmts, sizeof(*in_bus_fmts), > + GFP_KERNEL); > + if (!in_bus_fmts) > + return -ENOMEM; > + > + cur->funcs->atomic_get_input_bus_fmts(cur, cur_state, crtc_state, > + conn_state, out_bus_fmt, > + &num_in_bus_fmts, in_bus_fmts); > + > + if (first == cur) { > + cur_state->input_bus_cfg.fmt = in_bus_fmts[0]; > + cur_state->output_bus_cfg.fmt = out_bus_fmt; > + kfree(in_bus_fmts); > + return 0; > + } > + > + for (i = 0; i < num_in_bus_fmts; i++) { > + ret = select_bus_fmt_recursive(first, prev, crtc_state, > + conn_state, in_bus_fmts[i]); > + if (ret != -ENOTSUPP) > + break; > + } > + > + if (!ret) { > + cur_state->input_bus_cfg.fmt = in_bus_fmts[i]; > + cur_state->output_bus_cfg.fmt = out_bus_fmt; > + } > + > + kfree(in_bus_fmts); > + return ret; > +} > + > +/* > + * This function is called by &drm_atomic_bridge_chain_check() just before > + * calling &drm_bridge_funcs.atomic_check() on all elements of the chain. > + * It's providing bus format negotiation between bridge elements. The > + * negotiation happens in reverse order, starting from the last element in > + * the chain up to @bridge. > + * > + * Negotiation starts by retrieving supported output bus formats on the last > + * bridge element and testing them one by one. The test is recursive, meaning > + * that for each tested output format, the whole chain will be walked > backward, > + * and each element will have to choose an input bus format that can be > + * transcoded to the requested output format. When a bridge element does not > + * support transcoding into a specific output format -ENOTSUPP is returned > and > + * the next bridge element will have to try a different format. If none of > the > + * combinations worked, -ENOTSUPP is returned and the atomic modeset will > fail. > + * > + * This implementation is relying on > + * &drm_bridge_funcs.atomic_get_output_bus_fmts() and > + * &drm_bridge_funcs.atomic_get_input_bus_fmts() to gather supported > + * input/output formats. > + * When &drm_bridge_funcs.atomic_get_output_bus_fmts() is not implemented by > + * the last element of the chain, &drm_atomic_bridge_chain_select_bus_fmts() > + * tries a single format: &drm_connector.display_info.bus_formats[0] if > + * available, MEDIA_BUS_FMT_FIXED otherwise. > + * When &drm_bridge_funcs.atomic_get_input_bus_fmts() is not implemented, > + * &drm_atomic_bridge_chain_select_bus_fmts() skips the negotiation on the > + * bridge element that lacks this hook and asks the previous element in the > + * chain to try MEDIA_BUS_FMT_FIXED. It's up to bridge drivers to decide what > + * to do in that case (fail if they want to enforce bus format negotiation, > or > + * provide a reasonable default if they need to support pipelines where not > + * all elements support bus format negotiation). > + */ > +static int > +drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + struct drm_connector *conn = conn_state->connector; > + struct drm_encoder *encoder = bridge->encoder; > + struct drm_bridge_state *last_bridge_state; > + unsigned int i, num_out_bus_fmts; > + struct drm_bridge *last_bridge; > + u32 *out_bus_fmts; > + int ret = 0; > + > + last_bridge = list_last_entry(&encoder->bridge_chain, > + struct drm_bridge, chain_node); > + last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, > + last_bridge); > + if (WARN_ON(!last_bridge_state)) > + return -EINVAL; > + > + if (last_bridge->funcs->atomic_get_output_bus_fmts) > + last_bridge->funcs->atomic_get_output_bus_fmts(last_bridge, > + last_bridge_state, > + crtc_state, > + conn_state, > + &num_out_bus_fmts, > + NULL); > + else > + num_out_bus_fmts = 1; > + > + if (!num_out_bus_fmts) > + return -ENOTSUPP; > + > + out_bus_fmts = kcalloc(num_out_bus_fmts, sizeof(*out_bus_fmts), > + GFP_KERNEL); > + if (!out_bus_fmts) > + return -ENOMEM; > + > + if (last_bridge->funcs->atomic_get_output_bus_fmts) > + last_bridge->funcs->atomic_get_output_bus_fmts(last_bridge, > + last_bridge_state, > + crtc_state, > + conn_state, > + &num_out_bus_fmts, > + out_bus_fmts); > + else if (conn->display_info.num_bus_formats && > + conn->display_info.bus_formats) > + out_bus_fmts[0] = conn->display_info.bus_formats[0]; > + else > + out_bus_fmts[0] = MEDIA_BUS_FMT_FIXED; > + > + for (i = 0; i < num_out_bus_fmts; i++) { > + ret = select_bus_fmt_recursive(bridge, last_bridge, crtc_state, > + conn_state, out_bus_fmts[i]); > + if (ret != -ENOTSUPP) > + break; > + } > + > + kfree(out_bus_fmts); > + > + return ret; > +} > + > /** > * drm_atomic_bridge_chain_check() - Do an atomic check on the bridge chain > * @bridge: bridge control structure > * @crtc_state: new CRTC state > * @conn_state: new connector state > * > - * Calls &drm_bridge_funcs.atomic_check() (falls back on > + * First trigger a bus format negotiation before calling > + * &drm_bridge_funcs.atomic_check() (falls back on > * &drm_bridge_funcs.mode_fixup()) op for all the bridges in the encoder > chain, > * starting from the last bridge to the first. These are called before > calling > * &drm_encoder_helper_funcs.atomic_check() > @@ -644,6 +815,12 @@ int drm_atomic_bridge_chain_check(struct drm_bridge > *bridge, > { > struct drm_encoder *encoder = bridge->encoder; > struct drm_bridge *iter; > + int ret; > + > + ret = drm_atomic_bridge_chain_select_bus_fmts(bridge, crtc_state, > + conn_state); > + if (ret) > + return ret; > > list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { > int ret; > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 95dc58c3a4e8..96df3bedfee4 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -33,15 +33,33 @@ 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_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 > */ > 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 * > @@ -391,6 +409,70 @@ struct drm_bridge_funcs { > void (*atomic_destroy_state)(struct drm_bridge *bridge, > struct drm_bridge_state *state); > > + /** > + * @atomic_get_output_bus_fmts: > + * > + * Return the supported bus formats on the output end of a bridge. > + * This method is called twice. Once with output_fmts set NULL, in this > + * case the driver should set num_output_fmts to the number of > + * supported output bus formats such that the core can allocate an > + * array of the right dimension. The second time it's called with a > + * non-NULL output_fmts, and the driver is expected to fill the > + * output_fmts array. The output_fmts array passed to the driver is > + * guaranteed to be big enough to store the number of entries returned > + * on the first call (no need to check num_output_fmts). > + * > + * This method is only called on the last element of the bridge chain > + * as part of the bus format negotiation process that happens in > + * &drm_atomic_bridge_chain_select_bus_fmts(). > + * This method is optional. When not implemented, the core will > + * fallback to &drm_connector.display_info.bus_formats[0] if > + * &drm_connector.display_info.num_bus_formats > 0, > + * MEDIA_BUS_FMT_FIXED otherwise. > + */ > + void (*atomic_get_output_bus_fmts)(struct drm_bridge *bridge, > + struct drm_bridge_state > *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state > *conn_state, > + unsigned int *num_output_fmts, > + u32 *output_fmts); > + > + /** > + * @atomic_get_input_bus_fmts: > + * > + * Return the supported bus formats on the input end of a bridge for > + * a specific output bus format. > + * This method is called twice. Once with input_fmts set NULL, in this > + * case the driver should set num_input_fmts to the number of > + * supported input bus formats such that the core can allocate an array > + * of the right dimension. The second time it's called with a non-NULL > + * input_fmts, and the driver is expected to fill the input_fmts array. > + * The input_fmts array passed to the driver is guaranteed to be big > + * enough to store the number of entries returned on the first call (no > + * need to check num_input_fmts). > + * > + * This method is called on all element of the bridge chain as part of > + * the bus format negotiation process that happens in > + * &drm_atomic_bridge_chain_select_bus_fmts(). > + * This method is optional. When not implemented, the core will bypass > + * bus format negotiation on this element of the bridge without > + * failing, and the previous element in the chain will be passed > + * MEDIA_BUS_FMT_FIXED as its output bus format. > + * > + * Bridge drivers that need to support being linked to bridges that are > + * not supporting bus format negotiation should handle the > + * output_fmt == MEDIA_BUS_FMT_FIXED case appropriately, by selecting a > + * sensible default value or extracting this information from somewhere > + * else (FW property, &drm_display_mode, &drm_display_info, ...) > + */ > + void (*atomic_get_input_bus_fmts)(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state > *conn_state, > + u32 output_fmt, > + unsigned int *num_input_fmts, > + u32 *input_fmts); > + > /** > * @atomic_check: > * > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
