Hi Andrzej, On Wednesday, 5 December 2018 10:46:40 EET Andrzej Hajda wrote: > On 05.12.2018 07:32, Laurent Pinchart wrote: > > On Tuesday, 4 December 2018 21:13:20 EET Ville Syrjälä wrote: > >> On Tue, Dec 04, 2018 at 08:46:53AM +0100, Andrzej Hajda wrote: > >>> On 03.12.2018 22:38, Ville Syrjälä wrote: > >>>> On Thu, Nov 29, 2018 at 10:08:07AM +0100, Andrzej Hajda wrote: > >>>>> On 21.11.2018 19:19, Laurent Pinchart wrote: > >>>>>> On Tuesday, 20 November 2018 18:13:42 EET Ville Syrjala wrote: > >>>>>>> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >>>>>>> > >>>>>>> Make life easier for drivers by simply passing the connector > >>>>>>> to drm_hdmi_avi_infoframe_from_display_mode() and > >>>>>>> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > >>>>>>> need to worry about is_hdmi2_sink mess. > >>>>>> > >>>>>> While this is good for display controller drivers, the change isn't > >>>>>> great for bridge drivers. Down the road we're looking at moving > >>>>>> connector support out of the bridge drivers. Adding an additional > >>>>>> dependency to connectors in the bridges will make that more > >>>>>> difficult. Ideally bridges should retrieve the information from their > >>>>>> sink, regardless of whether it is a connector or another bridge. > >>>>> > >>>>> I agree with it, and case of sii8620 shows that there are cases where > >>>>> bridge has no direct access to the connector. > >>>> > >>>> It's just a matter of plumbing it through. > >>> > >>> What do you mean exactly? > >> > >> void bridge_foo(... > >> + ,struct drm_connector *connector); > >> > >>>>> On the other side, since you are passing connector to > >>>>> drm_hdmi_avi_infoframe_from_display_mode(), you could drop mode > >>>>> parameter and rename the function to > >>>>> drm_hdmi_avi_infoframe_from_connector() then, unless mode passed and > >>>>> mode set on the connector differs? > >>>> > >>>> Connectors don't have a mode. > >>> > >>> As they are passing video stream they should have it, even if not > >>> directly, for example: > >>> > >>> connector->state->crtc->mode > >> > >> That's not really how atomic works. One shouldn't go digging > >> through the obj->state pointers when we're not holding the > >> relevant locks anymore. The atomic way would be to pass either > >> both crtc state and connector state, or drm_atomic_state + > >> crtc/connector. > > Usually infoframe contents is generated in modesetting/enable callbacks > so the locks should be in place. > > And the locks should be hold for > drm_hdmi_avi_infoframe_from_display_mode as well - it wouldn't be correct if > > generated infoframe is not relevant to actual video mode. I guess that > if connector->state->crtc->mode > > differs from mode passed to drm_hdmi_avi_infoframe_from_display_mode it > is a sign of possible problem. > > And if they do not differ passing mode as an extra argument is redundant. > > > Or a bridge state ? With chained bridges the mode can vary along the > > pipeline, the CRTC adjusted mode will only cover the link between the > > CRTC and the first bridge. It's only a matter of time until we need to > > store other intermediate modes in states. I'd rather prepare for that > > instead of passing the CRTC state to bridges. > > Yes, optional bridge states seems reasonable, volunteers needed :) I'll give it a go eventually, if nobody beats me to it. The exact timing will depend on many variables so I can't estimate it I'm afraid. All I'm asking is to avoid extending the drm_bridge API in a way that would make introduction of bridge states more complex. If someone needs bridge states, for instance to solve the above issue, then they should be added. [snip] -- Regards, Laurent Pinchart _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
