On Tue, Jul 15, 2014 at 12:51 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: > 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. > Same here. My initial version of the patch just moved the edid assignment into the mode_valid() callback since that was the next time the common code called into the driver, but mode_fixup would work as well. Alex > ->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
