On 22/08/2019 12:34, Boris Brezillon wrote:
On Thu, 22 Aug 2019 12:22:02 +0200 Neil Armstrong narmstrong@baylibre.com wrote:
On 22/08/2019 12:09, Boris Brezillon wrote:
On Thu, 22 Aug 2019 11:29:40 +0200 Neil Armstrong narmstrong@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