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

[Boris Brezillon <boris.brezillon@collabora.com>]

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?

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel