Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Daniel Vetter]

On Mon, Aug 08, 2016 at 05:25:38PM +0100, Jose Abreu wrote:
> Hi,
> 
> 
> On 05-08-2016 09:13, Chris Wilson wrote:
> > On Fri, Aug 05, 2016 at 10:06:08AM +0200, Daniel Vetter wrote:
> >> On Fri, Aug 05, 2016 at 12:01:25AM +0100, Russell King - ARM Linux wrote:
> >>> On Thu, Aug 04, 2016 at 06:13:18PM +0100, Jose Abreu wrote:
> >>>> Hi Russell,
> >>>>
> >>>> So, I didn't use framebuffer console but used X instead and it is
> >>>> working as it should. I think we can drop this patch. I am now
> >>>> making interoperability with DVI and I am facing the following
> >>>> scenario:
> >>>>     - I start the driver
> >>>>     - An EDID is sent which tells the driver that HDMI is NOT
> >>>> supported;
> >>>>     - The driver configures itself to a DVI mode;
> >>>>
> >>>> Until this point everything is working as it should. But:
> >>>>
> >>>>     - Now I send an EDID which tells the driver that HDMI is
> >>>> supported;
> >>>>     - As the EDID has the same preferred mode the user will not
> >>>> reconfigure the mode and there will be no change to HDMI mode.
> >>> The EDID should still be read, but as you say, userspace doesn't take
> >>> any action because it sees that the mode parameters are still the same,
> >>> as you have identified.
> >>>
> >>>> The missing change to HDMI mode will cause the test to fail. The
> >>>> workaround that I am using is to reconfigure to another video
> >>>> mode and then configure to the preferred one but I think this
> >>>> could be fixed in the driver, right?
> >>> This one is extremely awkward - to fix it, we would need to check to
> >>> see whether we reconfigure the hardware each time we read the EDID,
> >>> and I don't think that's a particularly nice thing to do.
> >>>
> >>> I'd like to hear what other DRM developers think about this issue.
> >> I pondored whether we should have a counter on each connector for probe
> >> state, which helpers would increment whenever something changes, like:
> >> - connector_status (as tracked by probe helpers)
> >> - anything in the edid changes (when setting it
> >>   drm_mode_connector_update_edid_property)
> >> - other changes (like sink state changes in dpcd or whatever)
> >>
> >> That way userspace would be able to reliably spot such changes and do a
> >> new modeset.
> > Yes, please. I have had similar wishes for state changes and overall
> > modeset counters.
> > -Chris
> >
> 
> What about this: Assuming that the modes probed by EDID have the
> picture aspect ratio field set we can check for one of this
> probed modes when receiving a mode from userspace and then if the
> modes match (except from the picture aspect field, which is not
> set by userspace) then we can use the picture aspect value of the
> probed mode. I am using something like this in my modeset callback:
> 
>     /* 'mode' comes from userspace, 'pmode' comes from EDID */
>     if (mode->picture_aspect_ratio == HDMI_PICTURE_ASPECT_NONE) {
>         struct drm_display_mode *pmode;
> 
>         list_for_each_entry(pmode, &connector.modes, head) {
>             if (drm_mode_equal(pmode, mode))
>                 mode->picture_aspect_ratio =
> pmode->picture_aspect_ratio;
>         }
>     }
> 
> Of course if the EDID has for example two exactly equal modes
> that only differ in the picture aspect ratio then this will not
> work unless user passes the picture aspect when setting the mode.

Erhm, that seems to be an entirely different topic. And patches to fix
this are already on the mailing list, being reviewed. It's roughly your
idea, expect we fully expose the aspect ratio to userspace, and userspace
can select (if there multiple matching modes with different aspect
ratios).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch