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

[Alex Deucher]

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.

Alex