| Message ID | CADnq5_O4O4FCMnx_sZrmfTLuQWaPg+ct_vJcytQH-a7sRma-yw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 15, 2014 at 5:44 PM, Alex Deucher <alexdeucher@gmail.com> wrote: > On Tue, Jul 15, 2014 at 11:18 AM, Daniel Vetter <daniel@ffwll.ch> 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@amd.com> >> >> [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
On Tue, Jul 15, 2014 at 12:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Jul 15, 2014 at 5:44 PM, Alex Deucher <alexdeucher@gmail.com> wrote: >> On Tue, Jul 15, 2014 at 11:18 AM, Daniel Vetter <daniel@ffwll.ch> 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@amd.com> >>> >>> [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
On Wed, Jul 16, 2014 at 03:51:37PM -0400, Alex Deucher wrote: > On Tue, Jul 15, 2014 at 12:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Jul 15, 2014 at 5:44 PM, Alex Deucher <alexdeucher@gmail.com> wrote: > >> On Tue, Jul 15, 2014 at 11:18 AM, Daniel Vetter <daniel@ffwll.ch> 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@amd.com> > >>> > >>> [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. So you'll create a new drm_connector_get_cooked_edid or similar which drivers could use for this purpose? Obviously we can't fix all the drivers to dtrt with this approach, but documenting the best practices and adding the helper to drm_probe_helper would be great. -Daniel
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