On 22 April 2016 at 14:02, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Fri, Apr 22, 2016 at 12:18:07PM -0300, Ezequiel Garcia wrote:
>> On 22 Apr 10:15 AM, Daniel Vetter wrote:
>> > On Thu, Apr 21, 2016 at 03:13:51PM -0300, Ezequiel Garcia wrote:
>> > > Daniel,
>> > >
>> > > Thanks a lot for the quick reply!
>> > >
>> > > On 20 Apr 01:34 PM, Daniel Vetter wrote:
>> > > > On Tue, Apr 19, 2016 at 02:31:13PM -0300, Ezequiel Garcia wrote:
>> > > > > Currently, our implementation of drm_connector_funcs.detect is
>> > > > > based on getting a valid EDID.
>> > > > >
>> > > > > This requirement makes the driver fail to detect connected
>> > > > > connectors in case of EDID corruption, which in turn prevents
>> > > > > from falling back to modes provided by builtin or user-provided
>> > > > > EDIDs.
>> > > >
>> > > > Imo, this should be fixed in the probe helpers. Something like the below
>> > > > might make sense:
>> > > >
>> > > >
>> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> > > > index e714b5a7955f..d3b9dc7535da 100644
>> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
>> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
>> > > > @@ -214,7 +214,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>> > > > else
>> > > > connector->status = connector_status_disconnected;
>> > > > if (connector->funcs->force)
>> > > > - connector->funcs->force(connector);
>> > > > + connector->funcs->force(connector);
>> > > > + } else if (connector->override_edid){
>> > > > + connector->status = connector_status_connected;
>> > > > + connector->funcs->force(connector);
>> > > > } else {
>> > > > connector->status = connector->funcs->detect(connector, true);
>> > > > }
>> > > >
>> > > >
>> > > > It should do what you want it to do, still allow us to override force
>> > > > state manually and also fix things up for every, not just i915-hdmi. Also,
>> > > > much smaller patch.
>> > > >
>> > >
>> > > The patch you are proposing doesn't seem to be related
>> > > to the issue I want to fix, so maybe my explanation is still
>> > > unclear. After re-reading my commit log, I came to realize
>> > > I'm still not explaining the issue properly.
>> > >
>> > > Let me try again :-)
>> > >
>> > > A user can connect any kind of HDMI monitor to the box, and
>> > > the kernel should be able to output some video, even when the
>> > > HDMI monitor is a piece of crap and sends completely broken EDID.
>> > >
>> > > Currently, the i915 driver uses the return value of intel_hdmi_set_edid()
>> > > to set the connector status. IOW, the connector status is set to "connected"
>> > > *only* if the EDID is correct, and is left as "disconnected" if the EDID
>> > > is corrupt.
>> > >
>> > > However, the connector status can be detected by just probing
>> > > the I2C bus (drm_probe_ddc).
>> > >
>> > > The DRM probe helper relies on the .detect callback to decide if
>> > > the modes' fallbacks, EDID provided modes, etc are going to be set.
>> > >
>> > > It only set those modes if the .detect callback handler returns
>> > > a "connected" status and does nothing if .detect returns
>> > > "disconnected".
>> > >
>> > > If the connector is reported as "disconnected" it will skip it and
>> > > the user won't be able to use it (except if the state is forced
>> > > with a parameter).
>> > >
>> > > I am currently shipping intel boxes without monitors, and the
>> > > user can connect its own monitor. I can't know before hand
>> > > what device is going to be plugged (neither on which output
>> > > connector, HDMI, DVI, etc)... so I am not able to force any state.
>> > >
>> > > The patch I am proposing makes the kernel work without any
>> > > user intervention, in the face of corrupted EDID coming out of
>> > > a monitor. This works because the .detect logic after my patch
>> > > just checks if a I2C device is present in the bus to mark the
>> > > connector as "connected" and does not use the EDID parsing for that.
>> > >
>> > > In fact, the EDID parsing is moved to .get_modes() since they're
>> > > not really used before. This at the very least, is consistent with
>> > > how other drivers work (I'm not inventing anything).
>> > >
>> > > Maybe the following commit log is better. How does it look now?
>> >
>> > But in that case the only thing you get is the 1024x756 fallback mode.
>> > You're users are happy with that?
>>
>> Well, users are happy when things Just Work :-)
>>
>> > I thought your use-case was that you
>> > need to overwrite the edid anyway, and that doing the edid override alone
>> > doesn't work. Hence my patch to make stuff work directly with just the
>> > edid override.
>> >
>>
>> I don't think the edid override does what you think it does. If you take
>> a look at the sources, you'll find it's only a debugfs interface to
>> inject EDID. I'm sure it's probably useful for debugging and development,
>> but it won't help at all in this case.
>
> We can lift that to a sysfs interface easily if there's demand. And this
> is the same injection thing as the firmware loader.
No, it's not. The firmware loader has some very useful builtin modes.
And there's no demand for this: the sysfs interface is already there,
assuming drivers detect the connector state properly. No need to force
anything.
> I'm just not convinced
> that the 1024x768 fallback you get without an injected edid is useful for
> anyone,
Well, it was useful for one of my users in the field. So, this *is* useful.
> and on top of that you can force the connector state easily.
>
No, I can't force the connector state easily. I have to add a kernel
parameter, and that requires a kernel reboot or a driver re-probe,
so it's not something easy.
But moreover, let's suppose my box has four connectors: two HDMIs,
and two DPs. Which one is the user using? I have no idea about which
connector is being connected, if the i915 driver refuses to detect the device.
(Well, I guess I can probe the I2C bus... which is what the driver should do :-)
> So not yet convinced.
I'm having a very hard time understanding why you want to have
a driver that doesn't work.
If you can't see why EDID builtins and fallback noedid modes are useful
for me, I wonder if I can at least convince you of having the .detect hook
doing what the rest of the DRM drivers do: limit itself to connector
state detection.
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
