On Tue, Jul 15, 2014 at 5:44 PM, Alex Deucher <alexdeucher@xxxxxxxxx> wrote: > On Tue, Jul 15, 2014 at 11:18 AM, Daniel Vetter <daniel@xxxxxxxx> wrote: >> On Tue, Jul 15, 2014 at 11:08:11AM -0400, Alex Deucher wrote: >>> We keep a cached version of the edid in radeon_connector which >>> we use for determining connectedness and when to enable certain >>> features like hdmi audio, etc. When the user uses the firmware >>> interface to override the driver with some other edid the driver's >>> copy is never updated. The fetch function will check if there >>> is a user supplied edid and update the driver's copy if there >>> is. >>> >>> bug: >>> https://bugs.freedesktop.org/show_bug.cgi?id=80691 >>> >>> Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx> >> >> [snip] >> >>> +struct edid *radeon_connector_edid(struct drm_connector *connector) >>> +{ >>> + struct radeon_connector *radeon_connector = to_radeon_connector(connector); >>> + struct drm_property_blob *edid_blob = connector->edid_blob_ptr; >>> + >>> + if (radeon_connector->edid) { >>> + return radeon_connector->edid; >>> + } else if (edid_blob) { >>> + struct edid *edid = kmemdup(edid_blob->data, edid_blob->length, GFP_KERNEL); >>> + if (edid) >>> + radeon_connector->edid = edid; >>> + } >>> + return radeon_connector->edid; >>> +} >> >> We have similar issues on intel now that we use the debugfs interface to >> force certain edids (for validating e.g. 4k or 3d) - our code doesn't see >> the forced edid. Should we have a helper somewhere or just change >> drm_get_edid to dtrt here? >> >> Adding Thomas who's working on this. > > I think the best solution would be to make drm_load_edid_firmware() > just return the raw user supplied edid, then let the drivers handle it > internally. That way the drivers could decide how they want to handle > it in their detect() or get_modes() callbacks. The problem is that if > drm_load_edid_firmware() succeeds, the driver's get_modes() callback > never gets called. A less invasive alternative would be to add a a > get_modes_firmware() callback, e.g., > > diff --git a/drivers/gpu/drm/drm_probe_helper.c > b/drivers/gpu/drm/drm_probe_helper.c > index d22676b..ceb246f 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -127,7 +127,10 @@ static int > drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect > } > > #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE > - count = drm_load_edid_firmware(connector); > + if (connector_funcs->get_modes_firmware) > + count = (*connector_funcs->get_modes_firmware)(connector); > + else > + count = drm_load_edid_firmware(connector); > if (count == 0) > #endif > count = (*connector_funcs->get_modes)(connector); > > and the driver implementation could mostly just wrap > drm_load_edid_firmware, e.g., > > +static int radeon_get_modes_firmware(struct drm_connector *connector) > +{ > + struct radeon_connector *radeon_connector = > to_radeon_connector(connector); > + struct drm_property_blob *edid_blob; > + int ret; > + > + ret = drm_load_edid_firmware(connector); > + edid_blob = connector->edid_blob_ptr; > + /* update the driver's copy of the */ > + if (edid_blob) { > + struct edid *edid = kmemdup(edid_blob->data, > edid_blob->length, GFP_KERNEL); > + if (edid) > + radeon_connector->edid = edid; > + } > + > + return ret; > +} > > The problem is that wouldn't give the driver access to the user > provided edid at detect() time. Yeah, we also do a bunch of things in ->detect, so ->detect not getting called for forced edids is a bit annoying. The other thing is that edid overriding through debugfs at runtime is done differently again, and for those the driver's ->detect actually gets called. Well, if the connector state doesn't get forced. One idea I've had which is a bit of work is to move all these detection stuff outside of ->detect and into encoder->mode_fixup functions (compute_config in i915). If we then add a function to grab the cached/firmware/overridden edid and use it there it should all work. And at least on i915 you need to do a full modeset to e.g. update the audio status anyway. ->detect would then really only be for probing and generating the mode list while figuring out the actual hw config. And updating driver private stuff in foo_connector would be done only in ->mode_fixup. I don't see a good way to make things work as-is, at least if we want to run ->detect always (since a lot of driver have important code in there) and make it work with the firmware/debugfs edid forcing and the connector status forcing (through cmdline or again debugfs). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
