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

[Alex Deucher]

On Tue, Jul 15, 2014 at 12:51 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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.
>

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