[PATCH RFC 14/19] drm/bridge: Add the necessary bits to support bus format negotiation

[Boris Brezillon]

On Thu, 22 Aug 2019 11:29:40 +0200
Neil Armstrong <narmstrong at baylibre.com> wrote:

> >>> +/**
> >>> + * 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.

Sounds exactly like what Laurent was describing.

> 
> 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 ?

If all output ends of bridges have fixed formats (meaning that
something sets it to a fixed value, like a DT prop), there's no need
for negotiation at all, since the output of the previous element in
the chain will force input format of the current element.

> 
> Adding matrix in struct drm_bridge seems over-engineered, no ?

Will have to try it to give an answer to that question :).

> Since it should take
> in account the actual drm_display_mode.

Taking the display_mode into account is yet another thing, though it
might impact the way we want to handle bus-format negotiation. The way
I see it, we have 2 issues to solve:

1/ make sure the atomic modeset does not fail if there's at least one
   bus-format combination that works (this is what the
   supported-transcoding matrix is for)
2/ make sure that we pick the best option among all the possibilities.
   That requires defining what 'best' means. In some cases you might
   want to reduce power-consumption/bandwidth in others you might want
   to preserve image-quality (relative to the selected mode of course)

I decided to ignore problem #2 for now, but that's definitely something
we should work on at some point. Problem #1 is a problem we should
solve now. If we find a solution that solves both it's even better :-).

> 
> 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 ?

IIUC, you propose to restrict the set of input formats based on the
selected output format from inside the driver ->atomic_check()
implementation. That's basically like open-coding the
supported-transcoding matrix: you'll have to have a list of supported
input formats for each output format (or set of output formats), right?

Note that we could implement the supported-transcoding thing with a hook
instead of a matrix, but, no matter the solution we pick, it will always
be something that can answer this question "is bridge X able to convert
input format A into output format B?".

The nice thing about having the transcoding matrix calculated at attach
time is that you reject combinations that are not possible early instead
of having to do this work at each atomic_check(). Also, if you're not
rejecting impossible combinations early, I think you still have the
problem Laurent was talking about when you have more than 2 elements in
the chain.

> This would really simplify the bridge atomic_check implementation by reusing
> the drm_atomic_bridge_choose_*() functions.
>