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

[Daniel Vetter]

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 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.

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch