On 22/08/2019 09:48, Boris Brezillon wrote:
> On Thu, 22 Aug 2019 03:55:56 +0300
> Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
>> 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.
>
> Yes, I assumed all pipeline elements would be able to convert any of
> their supported input formats to any of the output ones, which might
> not be the case in practice. I could add a matrix to expose the
> supported conversions and add a pass of 'format eviction' at bridge
> attach time, but I'm not sure that's enough.
>
>>
>> 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 ?
>
> Pick a default value (or a something fixed by a FW property), as it used
> to do before this patchset. There's no way we can convert all bridges at
> once, so we need to support mixed chains formed of bridges that support
> bus format negotiation and bridges that don't.
>
>>
>>> +
>>> + 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.
>
> Oops.
>
> "Note that drivers that don't care about ...".
> I'm just trying to explain why EINVAL is returned when
> ->output_bus_caps is not populated.
>
>>
>>> + */
>>> + 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 ?
>
> Hm, not really. I mean, if the bridge supports several output formats,
> there's no way we can tell which one is the right one for this
> specific setup. The idea being that drivers are responsible for picking
> an appropriate output bus-format based on something else (DT prop?)
> when drm_atomic_bridge_choose_output_bus_cfg() returns an error.
>
>>
>>
>>> + */
>>> + 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.
>
> Hm, not sure how you'd represent that. I guess you want to support
> bridges that can activate several inputs+outputs in parallel. Do we
> even support that right now?
> Also, how do we link the input to the output in that case? By their
> position in the array? And we'd need an extra field in bus_caps to
> point to the corresponding bridge input/output port.
>
> I'm glad we can discuss all those problems, but I'd like to focus on
> what's needed right now, not what could potentially be needed in the
> future. If we can design things to be more future-proof that's great,
> but in my experience, trying to solve problems that do not exist yet
> often leads to complex designs which prove to be wrong when we try to
> apply them to real situations when they finally occur.
I had a thought when implementing bus format negotiation for dw-hdmi.
Depending on the output bus-format, we could have different sets of possible
input drm_bus_caps.
For example, if output is MEDIA_BUS_FMT_RGB888_1X24,
the input bus formats could only be MEDIA_BUS_FMT_RGB888_1X24 or MEDIA_BUS_FMT_YUV8_1X24.
Same for MEDIA_BUS_FMT_RGB101010_1X30, MEDIA_BUS_FMT_RGB121212_1X36, MEDIA_BUS_FMT_RGB161616_1X48...
And the special MEDIA_BUS_FMT_UYYVYY8_0_5X24, where input must be output.
How could we handle this ? I mean, could we have a fixed
output drm_bus_caps and be able to dynamically change the input drm_bus_caps ?
Adding matrix in struct drm_bridge seems over-engineered, no ? Since it should take
in account the actual drm_display_mode.
Maybe by passing a dynamic input drm_bus_caps to drm_atomic_bridge_choose_input_bus_cfg()
and take the default struct drm_bridge one if not provided ?
This would really simplify the bridge atomic_check implementation by reusing
the drm_atomic_bridge_choose_*() functions.
Neil
>
>>
>>> */
>>> 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)
>>>
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
