[Intel-gfx] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions

[Laurent Pinchart]

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 at linux.intel.com>
> >>>>>>> 
> >>>>>>> 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