[PATCH 2/2] drm/radeon: use a fetch function to get the edid

[Daniel Vetter]

On Tue, Jul 15, 2014 at 5:44 PM, Alex Deucher <alexdeucher at gmail.com> wrote:
> On Tue, Jul 15, 2014 at 11:18 AM, Daniel Vetter <daniel at 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 at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch