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

[Neil Armstrong]

On 22/08/2019 12:34, Boris Brezillon wrote:
> On Thu, 22 Aug 2019 12:22:02 +0200
> Neil Armstrong <narmstrong at baylibre.com> wrote:
> 
>> 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 ?
> 
> Okay, so you need to check/reject bus-formats at runtime (the mode is
> not known at attach time). Do you have an idea of what would be checked
> in those hooks?
> 

The output bus format is dependent on the mode (drm_mode_is_420_only() and drm_mode_is_420_also())
and the display info(info->hdmi.scdc.supported, info->bpc and info->color_formats).

dw_hdmi_bus_fmt_yuv420_check would check :
drm_mode_is_420_only(info, mode) || (!info->hdmi.scdc.supported && drm_mode_is_420_also(info, mode))
and if encoder supports the corresponding input mode

dw_hdmi_bus_fmt_no_yuv420_check would be reverse.

dw_hdmi_bus_fmt_yuv444_check would check :
info->color_formats & DRM_COLOR_FORMAT_YCRCB444
and if encoder supports the corresponding input mode

dw_hdmi_bus_fmt_rgb_10bit_check would check:
info->bpc >= 10
and if encoder supports the corresponding input mode

10/12/16 bit selection is best-effort (at least how intel implemented it), this means
we would put the 16/12/10bits bus-formats first and 8bit last.

Same for YUV vs RGB, we should select YUV first if encoder supports it, otherwise RGB.

Neil