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

[Neil Armstrong]

On 22/08/2019 12:09, Boris Brezillon wrote:
> 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 :).

Another thought, I was thinking about a matrix with an optional callback checking
is this particular matrix line is possible, an example :

struct drm_bridge_bus_fmt_assoc dw_hdmi_bus_fmt_matrix[] = {
	{ MEDIA_BUS_FMT_RGB888_1X24,
		{MEDIA_BUS_FMT_RGB888_1X24, MEDIA_BUS_FMT_YUV8_1X24, 0},
		{dw_hdmi_bus_fmt_no_yuv420_check, NULL},
	},
	{ MEDIA_BUS_FMT_YUV8_1X24,
		{MEDIA_BUS_FMT_YUV8_1X24, 0},
		{dw_hdmi_bus_fmt_no_yuv420_check,
		 dw_hdmi_bus_fmt_yuv444_check,
		 NULL},
	},
	{ MEDIA_BUS_FMT_RGB101010_1X30,
		{MEDIA_BUS_FMT_RGB101010_1X30, MEDIA_BUS_FMT_YUV10_1X30, 0},
		{dw_hdmi_bus_fmt_no_yuv420_check,
		 dw_hdmi_bus_fmt_rgb_10bit_check,
		 NULL},
	},
	{ MEDIA_BUS_FMT_UYYVYY8_0_5X24,
		{MEDIA_BUS_FMT_UYYVYY8_0_5X24, 0},
		dw_hdmi_bus_fmt_yuv420_check,
	},
	{ MEDIA_BUS_FMT_UYYVYY10_0_5X30,
		{MEDIA_BUS_FMT_UYYVYY8_0_5X24, 0},
		{dw_hdmi_bus_fmt_yuv420_check,
		 dw_hdmi_bus_fmt_yuv420_10bit_check,
		 NULL},
	},
};

Or with some flags combinations to check on the mode and some on the edid ?

Neil

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