| Message ID | 20200122111700.1924960-7-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add support for bus-format negotiation | expand |
On 2020-01-22 12:16, Boris Brezillon wrote: > drm_bridge_state is extended to describe the input and output bus > configurations. These bus configurations are exposed through the > drm_bus_cfg struct which encodes the configuration of a physical > bus between two components in an output pipeline, usually between > two bridges, an encoder and a bridge, or a bridge and a connector. > > The bus configuration is stored in drm_bridge_state separately for > the input and output buses, as seen from the point of view of each > bridge. The bus configuration of a bridge output is usually identical > to the configuration of the next bridge's input, but may differ if > the signals are modified between the two bridges, for instance by an > inverter on the board. The input and output configurations of a > bridge may differ if the bridge modifies the signals internally, > for instance by performing format conversion, or*modifying signals > polarities. > > Bus format negotiation is automated by the core, drivers just have > to implement the ->atomic_get_{output,input}_bus_fmts() hooks if they > want to take part to this negotiation. Negotiation happens in reverse > order, starting from the last element of the chain (the one directly > connected to the display) up to the first element of the chain (the one > connected to the encoder). > During this negotiation all supported formats are tested until we find > one that works, meaning that the formats array should be in decreasing > preference order (assuming the driver has a preference order). > > Note that the bus format negotiation works even if some elements in the > chain don't implement the ->atomic_get_{output,input}_bus_fmts() hooks. > In that case, the core advertises only MEDIA_BUS_FMT_FIXED and lets > the previous bridge element decide what to do (most of the time, bridge > drivers will pick a default bus format or extract this piece of > information from somewhere else, like a FW property). > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > [narmstrong: fixed doc in include/drm/drm_bridge.h:69 fmt->format] > Reviewed by: Jernej Skrabec <jernej.skrabec@siol.net> > Tested-by: Jonas Karlman <jonas@kwiboo.se> > --- > Changes in v7: > * Adapt the code to deal with the fact that not all bridges in the > chain have a bridge state > --- > drivers/gpu/drm/drm_atomic_helper.c | 41 +++++ > drivers/gpu/drm/drm_bridge.c | 253 +++++++++++++++++++++++++++- > include/drm/drm_atomic.h | 42 +++++ > include/drm/drm_atomic_helper.h | 8 + > include/drm/drm_bridge.h | 82 +++++++++ > 5 files changed, 425 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index afe14f72a824..ea1926b5bb80 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3528,3 +3528,44 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, > return ret; > } > EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set); > + > +/** > + * drm_atomic_helper_bridge_propagate_bus_fmt() - Propagate output format to > + * the input end of a bridge > + * @bridge: bridge control structure > + * @bridge_state: new bridge state > + * @crtc_state: new CRTC state > + * @conn_state: new connector state > + * @output_fmt: tested output bus format > + * @num_input_fmts: will contain the size of the returned array > + * > + * This helper is a pluggable implementation of the > + * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that don't > + * modify the bus configuration between their input and their output. It > + * returns an array of input formats with a single element set to @output_fmt. > + * > + * RETURNS: > + * a valid format array of size @num_input_fmts, or NULL if the allocation > + * failed > + */ > +u32 * > +drm_atomic_helper_bridge_propagate_bus_fmt(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; > + > + input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL); > + if (!input_fmts) { > + *num_input_fmts = 0; > + return NULL; > + } > + > + *num_input_fmts = 1; > + input_fmts[0] = output_fmt; > + return input_fmts; > +} > +EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt); > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index dd3a2a9f87d3..0c28816146ba 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -598,13 +598,247 @@ static int drm_atomic_bridge_check(struct drm_bridge *bridge, > return 0; > } > > +static int select_bus_fmt_recursive(struct drm_bridge *first_bridge, > + struct drm_bridge *cur_bridge, > + 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_bridge; > + u32 *in_bus_fmts; > + int ret; > + > + prev_bridge = drm_bridge_get_prev_bridge(cur_bridge); > + cur_state = drm_atomic_get_new_bridge_state(crtc_state->state, > + cur_bridge); > + > + /* > + * If 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_bridge->funcs->atomic_get_input_bus_fmts) { > + if (cur_bridge != first_bridge) { > + ret = select_bus_fmt_recursive(first_bridge, > + prev_bridge, crtc_state, > + conn_state, > + MEDIA_BUS_FMT_FIXED); > + if (ret) > + return ret; > + } > + > + /* > + * Driver does not implement the atomic state hooks, but that's > + * fine, as long as it does not access the bridge state. > + */ > + if (cur_state) { > + cur_state->input_bus_cfg.format = MEDIA_BUS_FMT_FIXED; > + cur_state->output_bus_cfg.format = out_bus_fmt; > + } > + > + return 0; > + } > + > + /* > + * If the driver implements ->atomic_get_input_bus_fmts() it > + * should also implement the atomic state hooks. > + */ > + if (WARN_ON(!cur_state)) > + return -EINVAL; > + > + in_bus_fmts = cur_bridge->funcs->atomic_get_input_bus_fmts(cur_bridge, > + cur_state, > + crtc_state, > + conn_state, > + out_bus_fmt, > + &num_in_bus_fmts); > + if (!num_in_bus_fmts) > + return -ENOTSUPP; > + else if (!in_bus_fmts) > + return -ENOMEM; > + > + if (first_bridge == cur_bridge) { > + cur_state->input_bus_cfg.format = in_bus_fmts[0]; > + cur_state->output_bus_cfg.format = out_bus_fmt; > + kfree(in_bus_fmts); > + return 0; > + } > + > + for (i = 0; i < num_in_bus_fmts; i++) { > + ret = select_bus_fmt_recursive(first_bridge, prev_bridge, > + crtc_state, conn_state, > + in_bus_fmts[i]); > + if (ret != -ENOTSUPP) > + break; > + } > + > + if (!ret) { > + cur_state->input_bus_cfg.format = in_bus_fmts[i]; > + cur_state->output_bus_cfg.format = 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 performs 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 (last_bridge->funcs->atomic_get_output_bus_fmts) { > + const struct drm_bridge_funcs *funcs = last_bridge->funcs; > + > + /* > + * If the driver implements ->atomic_get_output_bus_fmts() it > + * should also implement the atomic state hooks. > + */ > + if (WARN_ON(last_bridge_state)) This looks wrong, with this changed to WARN_ON(!last_bridge_state) my RK3328 HDMI2.0/YUV444/YUV420/10-bit branch at [1] starts working. With WARN_ON(last_bridge_state) I get: [ 6.606658] WARNING: CPU: 0 PID: 167 at drivers/gpu/drm/drm_bridge.c:746 drm_atomic_bridge_chain_check+0x2b8/0x308 [ 6.606673] Hardware name: Pine64 Rock64 (DT) [ 6.606754] Call trace: [ 6.606759] drm_atomic_bridge_chain_check+0x2b8/0x308 [ 6.606764] drm_atomic_helper_check_modeset+0x89c/0xab8 [ 6.606768] drm_atomic_helper_check+0x1c/0xa0 [ 6.606772] drm_atomic_check_only+0x464/0x708 [ 6.606777] drm_atomic_commit+0x18/0x58 [1] https://github.com/Kwiboo/linux-rockchip/commits/next-20200122-bus-format Regards, Jonas > + return -EINVAL; > + > + out_bus_fmts = funcs->atomic_get_output_bus_fmts(last_bridge, > + last_bridge_state, > + crtc_state, > + conn_state, > + &num_out_bus_fmts); > + if (!num_out_bus_fmts) > + return -ENOTSUPP; > + else if (!out_bus_fmts) > + return -ENOMEM; > + } else { > + num_out_bus_fmts = 1; > + out_bus_fmts = kmalloc(sizeof(*out_bus_fmts), GFP_KERNEL); > + if (!out_bus_fmts) > + return -ENOMEM; > + > + 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; > +} > + > +static void > +drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge, > + struct drm_connector *conn, > + struct drm_atomic_state *state) > +{ > + struct drm_bridge_state *bridge_state, *next_bridge_state; > + struct drm_bridge *next_bridge; > + u32 output_flags = 0; > + > + bridge_state = drm_atomic_get_new_bridge_state(state, bridge); > + > + /* No bridge state attached to this bridge => nothing to propagate. */ > + if (!bridge_state) > + return; > + > + next_bridge = drm_bridge_get_next_bridge(bridge); > + > + /* > + * Let's try to apply the most common case here, that is, propagate > + * display_info flags for the last bridge, and propagate the input > + * flags of the next bridge element to the output end of the current > + * bridge when the bridge is not the last one. > + * There are exceptions to this rule, like when signal inversion is > + * happening at the board level, but that's something drivers can deal > + * with from their &drm_bridge_funcs.atomic_check() implementation by > + * simply overriding the flags value we've set here. > + */ > + if (!next_bridge) { > + output_flags = conn->display_info.bus_flags; > + } else { > + next_bridge_state = drm_atomic_get_new_bridge_state(state, > + next_bridge); > + /* > + * No bridge state attached to the next bridge, just leave the > + * flags to 0. > + */ > + if (next_bridge_state) > + output_flags = next_bridge_state->input_bus_cfg.flags; > + } > + > + bridge_state->output_bus_cfg.flags = output_flags; > + > + /* > + * Propage the output flags to the input end of the bridge. Again, it's > + * not necessarily what all bridges want, but that's what most of them > + * do, and by doing that by default we avoid forcing drivers to > + * duplicate the "dummy propagation" logic. > + */ > + bridge_state->input_bus_cfg.flags = output_flags; > +} > + > /** > * 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() > @@ -616,16 +850,33 @@ int drm_atomic_bridge_chain_check(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; > struct drm_bridge *iter; > + int ret; > > if (!bridge) > return 0; > > + ret = drm_atomic_bridge_chain_select_bus_fmts(bridge, crtc_state, > + conn_state); > + if (ret) > + return ret; > + > encoder = bridge->encoder; > list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { > int ret; > > + /* > + * Bus flags are propagated by default. If a bridge needs to > + * tweak the input bus flags for any reason, it should happen > + * in its &drm_bridge_funcs.atomic_check() implementation such > + * that preceding bridges in the chain can propagate the new > + * bus flags. > + */ > + drm_atomic_bridge_propagate_bus_flags(iter, conn, > + crtc_state->state); > + > ret = drm_atomic_bridge_check(iter, crtc_state, conn_state); > if (ret) > return ret; > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index 82a888769b3d..52d65a055491 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -995,6 +995,38 @@ drm_atomic_crtc_effectively_active(const struct drm_crtc_state *state) > return state->active || state->self_refresh_active; > } > > +/** > + * struct drm_bus_cfg - bus configuration > + * > + * This structure stores the configuration of a physical bus between two > + * components in an output pipeline, usually between two bridges, an encoder > + * and a bridge, or a bridge and a connector. > + * > + * The bus configuration is stored in &drm_bridge_state separately for the > + * input and output buses, as seen from the point of view of each bridge. The > + * bus configuration of a bridge output is usually identical to the > + * configuration of the next bridge's input, but may differ if the signals are > + * modified between the two bridges, for instance by an inverter on the board. > + * The input and output configurations of a bridge may differ if the bridge > + * modifies the signals internally, for instance by performing format > + * conversion, or modifying signals polarities. > + */ > +struct drm_bus_cfg { > + /** > + * @format: format used on this bus (one of the MEDIA_BUS_FMT_* format) > + * > + * This field should not be directly modified by drivers > + * (&drm_atomic_bridge_chain_select_bus_fmts() takes care of the bus > + * format negotiation). > + */ > + u32 format; > + > + /** > + * @flags: DRM_BUS_* flags used on this bus > + */ > + u32 flags; > +}; > + > /** > * struct drm_bridge_state - Atomic bridge state object > */ > @@ -1008,6 +1040,16 @@ struct drm_bridge_state { > * @bridge: the bridge this state refers to > */ > struct drm_bridge *bridge; > + > + /** > + * @input_bus_cfg: input bus configuration > + */ > + struct drm_bus_cfg input_bus_cfg; > + > + /** > + * @output_bus_cfg: input bus configuration > + */ > + struct drm_bus_cfg output_bus_cfg; > }; > > static inline struct drm_bridge_state * > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 9db3cac48f4f..b268180c97eb 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -224,4 +224,12 @@ drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state, > return old_plane_state->crtc && !new_plane_state->crtc; > } > > +u32 * > +drm_atomic_helper_bridge_propagate_bus_fmt(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); > + > #endif /* DRM_ATOMIC_HELPER_H_ */ > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 04815ab948dd..e9f407993c8b 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -370,6 +370,72 @@ 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. > + * The returned array must be allocated with kmalloc() and will be > + * freed by the caller. If the allocation fails, NULL should be > + * returned. num_output_fmts must be set to the returned array size. > + * Formats listed in the returned array should be listed in decreasing > + * preference order (the core will try all formats until it finds one > + * that works). > + * > + * 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 > + * fall back to &drm_connector.display_info.bus_formats[0] if > + * &drm_connector.display_info.num_bus_formats > 0, > + * or to MEDIA_BUS_FMT_FIXED otherwise. > + */ > + u32 *(*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); > + > + /** > + * @atomic_get_input_bus_fmts: > + * > + * Return the supported bus formats on the input end of a bridge for > + * a specific output bus format. > + * > + * The returned array must be allocated with kmalloc() and will be > + * freed by the caller. If the allocation fails, NULL should be > + * returned. num_output_fmts must be set to the returned array size. > + * Formats listed in the returned array should be listed in decreasing > + * preference order (the core will try all formats until it finds one > + * that works). When the format is not supported NULL should be > + * returned and *num_output_fmts should be set to 0. > + * > + * This method is called on all elements 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, ...) > + * > + * Note: Even if input format selection on the first bridge has no > + * impact on the negotiation process (bus format negotiation stops once > + * we reach the first element of the chain), drivers are expected to > + * return accurate input formats as the input format may be used to > + * configure the CRTC output appropriately. > + */ > + u32 *(*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); > + > /** > * @atomic_check: > * > @@ -384,6 +450,14 @@ struct drm_bridge_funcs { > * called when &drm_bridge_funcs.atomic_check() is implemented, so only > * one of them should be provided. > * > + * If drivers need to tweak &drm_bridge_state.input_bus_cfg.flags or > + * &drm_bridge_state.output_bus_cfg.flags it should should happen in > + * this function. By default the &drm_bridge_state.output_bus_cfg.flags > + * field is set to the next bridge > + * &drm_bridge_state.input_bus_cfg.flags value or > + * &drm_connector.display_info.bus_flags if the bridge is the last > + * element in the chain. > + * > * RETURNS: > * zero if the check passed, a negative error code otherwise. > */ > @@ -571,6 +645,14 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge, > void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge, > struct drm_atomic_state *state); > > +u32 * > +drm_atomic_helper_bridge_propagate_bus_fmt(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); > + > #ifdef CONFIG_DRM_PANEL_BRIDGE > struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel); > struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel, >
On Wed, 22 Jan 2020 23:44:28 +0000 (UTC) Jonas Karlman <jonas@kwiboo.se> wrote: > > +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 (last_bridge->funcs->atomic_get_output_bus_fmts) { > > + const struct drm_bridge_funcs *funcs = last_bridge->funcs; > > + > > + /* > > + * If the driver implements ->atomic_get_output_bus_fmts() it > > + * should also implement the atomic state hooks. > > + */ > > + if (WARN_ON(last_bridge_state)) > > This looks wrong, with this changed to WARN_ON(!last_bridge_state) > my RK3328 HDMI2.0/YUV444/YUV420/10-bit branch at [1] starts working. > > With WARN_ON(last_bridge_state) I get: > > [ 6.606658] WARNING: CPU: 0 PID: 167 at drivers/gpu/drm/drm_bridge.c:746 drm_atomic_bridge_chain_check+0x2b8/0x308 > [ 6.606673] Hardware name: Pine64 Rock64 (DT) > > [ 6.606754] Call trace: > [ 6.606759] drm_atomic_bridge_chain_check+0x2b8/0x308 > [ 6.606764] drm_atomic_helper_check_modeset+0x89c/0xab8 > [ 6.606768] drm_atomic_helper_check+0x1c/0xa0 > [ 6.606772] drm_atomic_check_only+0x464/0x708 > [ 6.606777] drm_atomic_commit+0x18/0x58 Add const drm_bridge_funcs ... = { ... .atomic_reset = drm_atomic_helper_bridge_reset, .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, ... }; and that should work. > > [1] https://github.com/Kwiboo/linux-rockchip/commits/next-20200122-bus-format > > Regards, > Jonas
On 2020-01-23 08:39, Boris Brezillon wrote: > On Wed, 22 Jan 2020 23:44:28 +0000 (UTC) > Jonas Karlman <jonas@kwiboo.se> wrote: > >>> +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 (last_bridge->funcs->atomic_get_output_bus_fmts) { >>> + const struct drm_bridge_funcs *funcs = last_bridge->funcs; >>> + >>> + /* >>> + * If the driver implements ->atomic_get_output_bus_fmts() it >>> + * should also implement the atomic state hooks. >>> + */ >>> + if (WARN_ON(last_bridge_state)) >> >> This looks wrong, with this changed to WARN_ON(!last_bridge_state) >> my RK3328 HDMI2.0/YUV444/YUV420/10-bit branch at [1] starts working. >> >> With WARN_ON(last_bridge_state) I get: >> >> [ 6.606658] WARNING: CPU: 0 PID: 167 at drivers/gpu/drm/drm_bridge.c:746 drm_atomic_bridge_chain_check+0x2b8/0x308 >> [ 6.606673] Hardware name: Pine64 Rock64 (DT) >> >> [ 6.606754] Call trace: >> [ 6.606759] drm_atomic_bridge_chain_check+0x2b8/0x308 >> [ 6.606764] drm_atomic_helper_check_modeset+0x89c/0xab8 >> [ 6.606768] drm_atomic_helper_check+0x1c/0xa0 >> [ 6.606772] drm_atomic_check_only+0x464/0x708 >> [ 6.606777] drm_atomic_commit+0x18/0x58 > > Add > > const drm_bridge_funcs ... = { > ... > .atomic_reset = drm_atomic_helper_bridge_reset, > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > ... > }; > > and that should work. That is what I added and what made that this warning is being triggered. The comment state that atomic state is needed, but the check warns when there is a state. I have this: static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { ... .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts, .atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts, .atomic_check = dw_hdmi_bridge_atomic_check, .atomic_reset = drm_atomic_helper_bridge_reset, ... }; and static const struct drm_bridge_funcs dw_hdmi_rockchip_bridge_funcs = { ... .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_get_input_bus_fmts = dw_hdmi_rockchip_get_input_bus_fmts, .atomic_check = dw_hdmi_rockchip_bridge_atomic_check, .atomic_reset = drm_atomic_helper_bridge_reset, }; after applying the following I got a hdmi signal again diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 0c28816146ba..7e7b0fac8f4f 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -743,7 +743,7 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, * If the driver implements ->atomic_get_output_bus_fmts() it * should also implement the atomic state hooks. */ - if (WARN_ON(last_bridge_state)) + if (WARN_ON(!last_bridge_state)) return -EINVAL; out_bus_fmts = funcs->atomic_get_output_bus_fmts(last_bridge, Regards, Jonas > >> >> [1] https://github.com/Kwiboo/linux-rockchip/commits/next-20200122-bus-format >> >> Regards, >> Jonas
On Thu, 23 Jan 2020 07:52:59 +0000 (UTC) Jonas Karlman <jonas@kwiboo.se> wrote: > On 2020-01-23 08:39, Boris Brezillon wrote: > > On Wed, 22 Jan 2020 23:44:28 +0000 (UTC) > > Jonas Karlman <jonas@kwiboo.se> wrote: > > > >>> +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 (last_bridge->funcs->atomic_get_output_bus_fmts) { > >>> + const struct drm_bridge_funcs *funcs = last_bridge->funcs; > >>> + > >>> + /* > >>> + * If the driver implements ->atomic_get_output_bus_fmts() it > >>> + * should also implement the atomic state hooks. > >>> + */ > >>> + if (WARN_ON(last_bridge_state)) > >> > >> This looks wrong, with this changed to WARN_ON(!last_bridge_state) > >> my RK3328 HDMI2.0/YUV444/YUV420/10-bit branch at [1] starts working. > >> > >> With WARN_ON(last_bridge_state) I get: > >> > >> [ 6.606658] WARNING: CPU: 0 PID: 167 at drivers/gpu/drm/drm_bridge.c:746 drm_atomic_bridge_chain_check+0x2b8/0x308 > >> [ 6.606673] Hardware name: Pine64 Rock64 (DT) > >> > >> [ 6.606754] Call trace: > >> [ 6.606759] drm_atomic_bridge_chain_check+0x2b8/0x308 > >> [ 6.606764] drm_atomic_helper_check_modeset+0x89c/0xab8 > >> [ 6.606768] drm_atomic_helper_check+0x1c/0xa0 > >> [ 6.606772] drm_atomic_check_only+0x464/0x708 > >> [ 6.606777] drm_atomic_commit+0x18/0x58 > > > > Add > > > > const drm_bridge_funcs ... = { > > ... > > .atomic_reset = drm_atomic_helper_bridge_reset, > > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > > ... > > }; > > > > and that should work. > > That is what I added and what made that this warning is being triggered. > The comment state that atomic state is needed, but the check warns when there is a state. > > I have this: > > static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { > ... > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > .atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts, > .atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts, > .atomic_check = dw_hdmi_bridge_atomic_check, > .atomic_reset = drm_atomic_helper_bridge_reset, > ... > }; > > and > > static const struct drm_bridge_funcs dw_hdmi_rockchip_bridge_funcs = { > ... > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > .atomic_get_input_bus_fmts = dw_hdmi_rockchip_get_input_bus_fmts, > .atomic_check = dw_hdmi_rockchip_bridge_atomic_check, > .atomic_reset = drm_atomic_helper_bridge_reset, > }; > > after applying the following I got a hdmi signal again > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 0c28816146ba..7e7b0fac8f4f 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -743,7 +743,7 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, > * If the driver implements ->atomic_get_output_bus_fmts() it > * should also implement the atomic state hooks. > */ > - if (WARN_ON(last_bridge_state)) > + if (WARN_ON(!last_bridge_state)) > return -EINVAL; My bad, I didn't read your email carefully. You're right, I'll fix it in v8. Thanks, Boris
On 23/01/2020 08:52, Jonas Karlman wrote: > On 2020-01-23 08:39, Boris Brezillon wrote: >> On Wed, 22 Jan 2020 23:44:28 +0000 (UTC) >> Jonas Karlman <jonas@kwiboo.se> wrote: >> >>>> +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 (last_bridge->funcs->atomic_get_output_bus_fmts) { >>>> + const struct drm_bridge_funcs *funcs = last_bridge->funcs; >>>> + >>>> + /* >>>> + * If the driver implements ->atomic_get_output_bus_fmts() it >>>> + * should also implement the atomic state hooks. >>>> + */ >>>> + if (WARN_ON(last_bridge_state)) >>> >>> This looks wrong, with this changed to WARN_ON(!last_bridge_state) >>> my RK3328 HDMI2.0/YUV444/YUV420/10-bit branch at [1] starts working. >>> >>> With WARN_ON(last_bridge_state) I get: >>> >>> [ 6.606658] WARNING: CPU: 0 PID: 167 at drivers/gpu/drm/drm_bridge.c:746 drm_atomic_bridge_chain_check+0x2b8/0x308 >>> [ 6.606673] Hardware name: Pine64 Rock64 (DT) >>> >>> [ 6.606754] Call trace: >>> [ 6.606759] drm_atomic_bridge_chain_check+0x2b8/0x308 >>> [ 6.606764] drm_atomic_helper_check_modeset+0x89c/0xab8 >>> [ 6.606768] drm_atomic_helper_check+0x1c/0xa0 >>> [ 6.606772] drm_atomic_check_only+0x464/0x708 >>> [ 6.606777] drm_atomic_commit+0x18/0x58 >> >> Add >> >> const drm_bridge_funcs ... = { >> ... >> .atomic_reset = drm_atomic_helper_bridge_reset, >> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, >> .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, >> ... >> }; >> >> and that should work. > > That is what I added and what made that this warning is being triggered. > The comment state that atomic state is needed, but the check warns when there is a state. > > I have this: > > static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { > ... > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > .atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts, > .atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts, > .atomic_check = dw_hdmi_bridge_atomic_check, > .atomic_reset = drm_atomic_helper_bridge_reset, > ... > }; > > and > > static const struct drm_bridge_funcs dw_hdmi_rockchip_bridge_funcs = { > ... > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > .atomic_get_input_bus_fmts = dw_hdmi_rockchip_get_input_bus_fmts, > .atomic_check = dw_hdmi_rockchip_bridge_atomic_check, > .atomic_reset = drm_atomic_helper_bridge_reset, > }; > > after applying the following I got a hdmi signal again > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 0c28816146ba..7e7b0fac8f4f 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -743,7 +743,7 @@ drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge, > * If the driver implements ->atomic_get_output_bus_fmts() it > * should also implement the atomic state hooks. > */ > - if (WARN_ON(last_bridge_state)) > + if (WARN_ON(!last_bridge_state)) > return -EINVAL; > > out_bus_fmts = funcs->atomic_get_output_bus_fmts(last_bridge, > > Regards, > Jonas > >> >>> >>> [1] https://github.com/Kwiboo/linux-rockchip/commits/next-20200122-bus-format >>> >>> Regards, >>> Jonas With this fix : Tested-by: Neil Armstrong <narmstrong@baylibre.com> Neil
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index afe14f72a824..ea1926b5bb80 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3528,3 +3528,44 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, return ret; } EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set); + +/** + * drm_atomic_helper_bridge_propagate_bus_fmt() - Propagate output format to + * the input end of a bridge + * @bridge: bridge control structure + * @bridge_state: new bridge state + * @crtc_state: new CRTC state + * @conn_state: new connector state + * @output_fmt: tested output bus format + * @num_input_fmts: will contain the size of the returned array + * + * This helper is a pluggable implementation of the + * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that don't + * modify the bus configuration between their input and their output. It + * returns an array of input formats with a single element set to @output_fmt. + * + * RETURNS: + * a valid format array of size @num_input_fmts, or NULL if the allocation + * failed + */ +u32 * +drm_atomic_helper_bridge_propagate_bus_fmt(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; + + input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL); + if (!input_fmts) { + *num_input_fmts = 0; + return NULL; + } + + *num_input_fmts = 1; + input_fmts[0] = output_fmt; + return input_fmts; +} +EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt); diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index dd3a2a9f87d3..0c28816146ba 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -598,13 +598,247 @@ static int drm_atomic_bridge_check(struct drm_bridge *bridge, return 0; } +static int select_bus_fmt_recursive(struct drm_bridge *first_bridge, + struct drm_bridge *cur_bridge, + 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_bridge; + u32 *in_bus_fmts; + int ret; + + prev_bridge = drm_bridge_get_prev_bridge(cur_bridge); + cur_state = drm_atomic_get_new_bridge_state(crtc_state->state, + cur_bridge); + + /* + * If 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_bridge->funcs->atomic_get_input_bus_fmts) { + if (cur_bridge != first_bridge) { + ret = select_bus_fmt_recursive(first_bridge, + prev_bridge, crtc_state, + conn_state, + MEDIA_BUS_FMT_FIXED); + if (ret) + return ret; + } + + /* + * Driver does not implement the atomic state hooks, but that's + * fine, as long as it does not access the bridge state. + */ + if (cur_state) { + cur_state->input_bus_cfg.format = MEDIA_BUS_FMT_FIXED; + cur_state->output_bus_cfg.format = out_bus_fmt; + } + + return 0; + } + + /* + * If the driver implements ->atomic_get_input_bus_fmts() it + * should also implement the atomic state hooks. + */ + if (WARN_ON(!cur_state)) + return -EINVAL; + + in_bus_fmts = cur_bridge->funcs->atomic_get_input_bus_fmts(cur_bridge, + cur_state, + crtc_state, + conn_state, + out_bus_fmt, + &num_in_bus_fmts); + if (!num_in_bus_fmts) + return -ENOTSUPP; + else if (!in_bus_fmts) + return -ENOMEM; + + if (first_bridge == cur_bridge) { + cur_state->input_bus_cfg.format = in_bus_fmts[0]; + cur_state->output_bus_cfg.format = out_bus_fmt; + kfree(in_bus_fmts); + return 0; + } + + for (i = 0; i < num_in_bus_fmts; i++) { + ret = select_bus_fmt_recursive(first_bridge, prev_bridge, + crtc_state, conn_state, + in_bus_fmts[i]); + if (ret != -ENOTSUPP) + break; + } + + if (!ret) { + cur_state->input_bus_cfg.format = in_bus_fmts[i]; + cur_state->output_bus_cfg.format = 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 performs 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 (last_bridge->funcs->atomic_get_output_bus_fmts) { + const struct drm_bridge_funcs *funcs = last_bridge->funcs; + + /* + * If the driver implements ->atomic_get_output_bus_fmts() it + * should also implement the atomic state hooks. + */ + if (WARN_ON(last_bridge_state)) + return -EINVAL; + + out_bus_fmts = funcs->atomic_get_output_bus_fmts(last_bridge, + last_bridge_state, + crtc_state, + conn_state, + &num_out_bus_fmts); + if (!num_out_bus_fmts) + return -ENOTSUPP; + else if (!out_bus_fmts) + return -ENOMEM; + } else { + num_out_bus_fmts = 1; + out_bus_fmts = kmalloc(sizeof(*out_bus_fmts), GFP_KERNEL); + if (!out_bus_fmts) + return -ENOMEM; + + 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; +} + +static void +drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge, + struct drm_connector *conn, + struct drm_atomic_state *state) +{ + struct drm_bridge_state *bridge_state, *next_bridge_state; + struct drm_bridge *next_bridge; + u32 output_flags = 0; + + bridge_state = drm_atomic_get_new_bridge_state(state, bridge); + + /* No bridge state attached to this bridge => nothing to propagate. */ + if (!bridge_state) + return; + + next_bridge = drm_bridge_get_next_bridge(bridge); + + /* + * Let's try to apply the most common case here, that is, propagate + * display_info flags for the last bridge, and propagate the input + * flags of the next bridge element to the output end of the current + * bridge when the bridge is not the last one. + * There are exceptions to this rule, like when signal inversion is + * happening at the board level, but that's something drivers can deal + * with from their &drm_bridge_funcs.atomic_check() implementation by + * simply overriding the flags value we've set here. + */ + if (!next_bridge) { + output_flags = conn->display_info.bus_flags; + } else { + next_bridge_state = drm_atomic_get_new_bridge_state(state, + next_bridge); + /* + * No bridge state attached to the next bridge, just leave the + * flags to 0. + */ + if (next_bridge_state) + output_flags = next_bridge_state->input_bus_cfg.flags; + } + + bridge_state->output_bus_cfg.flags = output_flags; + + /* + * Propage the output flags to the input end of the bridge. Again, it's + * not necessarily what all bridges want, but that's what most of them + * do, and by doing that by default we avoid forcing drivers to + * duplicate the "dummy propagation" logic. + */ + bridge_state->input_bus_cfg.flags = output_flags; +} + /** * 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() @@ -616,16 +850,33 @@ int drm_atomic_bridge_chain_check(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; struct drm_bridge *iter; + int ret; if (!bridge) return 0; + ret = drm_atomic_bridge_chain_select_bus_fmts(bridge, crtc_state, + conn_state); + if (ret) + return ret; + encoder = bridge->encoder; list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { int ret; + /* + * Bus flags are propagated by default. If a bridge needs to + * tweak the input bus flags for any reason, it should happen + * in its &drm_bridge_funcs.atomic_check() implementation such + * that preceding bridges in the chain can propagate the new + * bus flags. + */ + drm_atomic_bridge_propagate_bus_flags(iter, conn, + crtc_state->state); + ret = drm_atomic_bridge_check(iter, crtc_state, conn_state); if (ret) return ret; diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 82a888769b3d..52d65a055491 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -995,6 +995,38 @@ drm_atomic_crtc_effectively_active(const struct drm_crtc_state *state) return state->active || state->self_refresh_active; } +/** + * struct drm_bus_cfg - bus configuration + * + * This structure stores the configuration of a physical bus between two + * components in an output pipeline, usually between two bridges, an encoder + * and a bridge, or a bridge and a connector. + * + * The bus configuration is stored in &drm_bridge_state separately for the + * input and output buses, as seen from the point of view of each bridge. The + * bus configuration of a bridge output is usually identical to the + * configuration of the next bridge's input, but may differ if the signals are + * modified between the two bridges, for instance by an inverter on the board. + * The input and output configurations of a bridge may differ if the bridge + * modifies the signals internally, for instance by performing format + * conversion, or modifying signals polarities. + */ +struct drm_bus_cfg { + /** + * @format: format used on this bus (one of the MEDIA_BUS_FMT_* format) + * + * This field should not be directly modified by drivers + * (&drm_atomic_bridge_chain_select_bus_fmts() takes care of the bus + * format negotiation). + */ + u32 format; + + /** + * @flags: DRM_BUS_* flags used on this bus + */ + u32 flags; +}; + /** * struct drm_bridge_state - Atomic bridge state object */ @@ -1008,6 +1040,16 @@ struct drm_bridge_state { * @bridge: the bridge this state refers to */ struct drm_bridge *bridge; + + /** + * @input_bus_cfg: input bus configuration + */ + struct drm_bus_cfg input_bus_cfg; + + /** + * @output_bus_cfg: input bus configuration + */ + struct drm_bus_cfg output_bus_cfg; }; static inline struct drm_bridge_state * diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 9db3cac48f4f..b268180c97eb 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -224,4 +224,12 @@ drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state, return old_plane_state->crtc && !new_plane_state->crtc; } +u32 * +drm_atomic_helper_bridge_propagate_bus_fmt(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); + #endif /* DRM_ATOMIC_HELPER_H_ */ diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 04815ab948dd..e9f407993c8b 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -370,6 +370,72 @@ 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. + * The returned array must be allocated with kmalloc() and will be + * freed by the caller. If the allocation fails, NULL should be + * returned. num_output_fmts must be set to the returned array size. + * Formats listed in the returned array should be listed in decreasing + * preference order (the core will try all formats until it finds one + * that works). + * + * 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 + * fall back to &drm_connector.display_info.bus_formats[0] if + * &drm_connector.display_info.num_bus_formats > 0, + * or to MEDIA_BUS_FMT_FIXED otherwise. + */ + u32 *(*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); + + /** + * @atomic_get_input_bus_fmts: + * + * Return the supported bus formats on the input end of a bridge for + * a specific output bus format. + * + * The returned array must be allocated with kmalloc() and will be + * freed by the caller. If the allocation fails, NULL should be + * returned. num_output_fmts must be set to the returned array size. + * Formats listed in the returned array should be listed in decreasing + * preference order (the core will try all formats until it finds one + * that works). When the format is not supported NULL should be + * returned and *num_output_fmts should be set to 0. + * + * This method is called on all elements 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, ...) + * + * Note: Even if input format selection on the first bridge has no + * impact on the negotiation process (bus format negotiation stops once + * we reach the first element of the chain), drivers are expected to + * return accurate input formats as the input format may be used to + * configure the CRTC output appropriately. + */ + u32 *(*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); + /** * @atomic_check: * @@ -384,6 +450,14 @@ struct drm_bridge_funcs { * called when &drm_bridge_funcs.atomic_check() is implemented, so only * one of them should be provided. * + * If drivers need to tweak &drm_bridge_state.input_bus_cfg.flags or + * &drm_bridge_state.output_bus_cfg.flags it should should happen in + * this function. By default the &drm_bridge_state.output_bus_cfg.flags + * field is set to the next bridge + * &drm_bridge_state.input_bus_cfg.flags value or + * &drm_connector.display_info.bus_flags if the bridge is the last + * element in the chain. + * * RETURNS: * zero if the check passed, a negative error code otherwise. */ @@ -571,6 +645,14 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge, void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge, struct drm_atomic_state *state); +u32 * +drm_atomic_helper_bridge_propagate_bus_fmt(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); + #ifdef CONFIG_DRM_PANEL_BRIDGE struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel); struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
drm_bridge_state is extended to describe the input and output bus configurations. These bus configurations are exposed through the drm_bus_cfg struct which encodes the configuration of a physical bus between two components in an output pipeline, usually between two bridges, an encoder and a bridge, or a bridge and a connector. The bus configuration is stored in drm_bridge_state separately for the input and output buses, as seen from the point of view of each bridge. The bus configuration of a bridge output is usually identical to the configuration of the next bridge's input, but may differ if the signals are modified between the two bridges, for instance by an inverter on the board. The input and output configurations of a bridge may differ if the bridge modifies the signals internally, for instance by performing format conversion, or*modifying signals polarities. Bus format negotiation is automated by the core, drivers just have to implement the ->atomic_get_{output,input}_bus_fmts() hooks if they want to take part to this negotiation. Negotiation happens in reverse order, starting from the last element of the chain (the one directly connected to the display) up to the first element of the chain (the one connected to the encoder). During this negotiation all supported formats are tested until we find one that works, meaning that the formats array should be in decreasing preference order (assuming the driver has a preference order). Note that the bus format negotiation works even if some elements in the chain don't implement the ->atomic_get_{output,input}_bus_fmts() hooks. In that case, the core advertises only MEDIA_BUS_FMT_FIXED and lets the previous bridge element decide what to do (most of the time, bridge drivers will pick a default bus format or extract this piece of information from somewhere else, like a FW property). Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> [narmstrong: fixed doc in include/drm/drm_bridge.h:69 fmt->format] Reviewed by: Jernej Skrabec <jernej.skrabec@siol.net> Tested-by: Jonas Karlman <jonas@kwiboo.se> --- Changes in v7: * Adapt the code to deal with the fact that not all bridges in the chain have a bridge state --- drivers/gpu/drm/drm_atomic_helper.c | 41 +++++ drivers/gpu/drm/drm_bridge.c | 253 +++++++++++++++++++++++++++- include/drm/drm_atomic.h | 42 +++++ include/drm/drm_atomic_helper.h | 8 + include/drm/drm_bridge.h | 82 +++++++++ 5 files changed, 425 insertions(+), 1 deletion(-)