On 22/08/2019 12:09, Boris Brezillon wrote: > On Thu, 22 Aug 2019 11:29:40 +0200 > Neil Armstrong <narmstrong@xxxxxxxxxxxx> 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. >> > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel