lkml.org 
[lkml]   [2022]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
    Quoting Douglas Anderson (2022-04-08 19:36:23)
    > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
    > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
    > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
    > DP AUX bus properly we really need two "struct device"s. One "struct
    > device" is in charge of providing the DP AUX bus and the other is
    > where we'll try to get a reference to the newly probed endpoint
    > devices.
    >
    > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
    > is already broken up into several "struct devices" anyway because it
    > also provides a PWM and some GPIOs. Adding one more wasn't that
    > difficult / ugly.
    >
    > When I tried to do the same solution in parade-ps8640, it felt like I
    > was copying too much boilerplate code. I made the realization that I
    > didn't _really_ need a separate "driver" for each person that wanted
    > to do the same thing. By putting all the "driver" related code in a
    > common place then we could save a bit of hassle. This change
    > effectively adds a new "ep_client" driver that can be used by
    > anyone. The devices instantiated by this driver will just call through
    > to the probe/remove/shutdown calls provided.
    >
    > At the moment, the "ep_client" driver is backed by the Linux auxiliary
    > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
    > want to expose this to clients, though, so as far as clients are
    > concerned they get a vanilla "struct device".
    >
    > Signed-off-by: Douglas Anderson <dianders@chromium.org>
    > ---

    Is it correct that the struct dp_aux_ep_client is largely equivalent to
    a struct dp_aux_ep_device? What's the difference? I think it is a fully
    probed dp_aux_ep_device? Or a way to tell the driver that calls
    of_dp_aux_populate_ep_devices() that the endpoint has probed now?

    I read the commit text but it didn't click until I read the next patch
    that this is solving a probe ordering/loop problem. The driver that
    creates the 'struct drm_dp_aux' and populates devices on the DP aux bus
    with of_dp_aux_populate_ep_devices() wants to be sure that the
    drm_bridge made by the 'struct dp_aux_ep_driver' probe routine in
    edp-panel is going to be there before calling drm_of_get_bridge().

    of_dp_aux_populate_ep_devices();
    [No idea if the bridge is registered yet]
    drm_of_get_bridge();

    The solution is to retry the drm_of_get_bridge() until 'struct
    dp_aux_ep_driver' probes and registers the next bridge. It looks like a
    wait_for_completion() on top of drm_of_get_bridge() implemented through
    driver probe and -EPROBE_DEFER; no disrespect!

    Is there another problem here that the driver that creates the 'struct
    drm_dp_aux' and populates devices on the DP aux bus with
    of_dp_aux_populate_ep_devices() wants to use that same 'struct
    drm_dp_aux' directly to poke at some 'struct dp_aux_ep_device' that was
    populated? And the 'struct dp_aux_ep_device' may either not be probed
    and bound to a 'struct dp_aux_ep_driver' or it may not be powered on
    because it went to runtime PM suspend?

    Put another way, I worry that the owner of 'struct drm_dp_aux' wants to
    use some function in include/drm/dp/drm_dp_helper.h that takes the
    'struct drm_dp_aux' directly without considering the device model state
    of the endpoint device (the 'struct dp_aux_ep_device'). That would be a
    similar problem as waiting for the endpoint to be powered on and probed.
    Solving that through a sub-driver feels awkward.

    What if we had some function like drm_dp_get_aux_ep() that took a
    'struct drm_dp_aux' and looked for the endpoint device (there can only
    be one?), waited for it to be probed, and then powered it up via some
    pm_runtime_get_sync()? My understanding is that with edp-panel we have
    two power "domains" of importance, the panel power domain and the DP/eDP
    power domain. Access to the AUX bus via 'struct drm_dp_aux' needs both
    power domains to be powered on. If the 'struct dp_aux_ep_device' is in
    hand, then both power domains can be powered on by making sure the power
    state of the 'struct dp_aux_ep_device::dev' is enabled. If only the
    'struct drm_dp_aux' is in hand then we'll need to do something more like
    find the child device and power it on.

    Could the 'struct drm_dp_aux' functions in drm_dp_helper.h do this
    automatically somehow? Maybe we can drop a variable in 'struct
    drm_dp_aux' when of_dp_aux_populate_ep_devices() is called that the
    other side may not be powered up. Then if something tries to transfer on
    that aux channel it powers on all children of 'struct drm_dp_aux::dev'
    that are on the 'dp_aux_bus_type' or bails out if those devices haven't
    probed yet or can't be powered on.

    > diff --git a/include/drm/dp/drm_dp_aux_bus.h b/include/drm/dp/drm_dp_aux_bus.h
    > index 4f19b20b1dd6..ecf68b6873bd 100644
    > --- a/include/drm/dp/drm_dp_aux_bus.h
    > +++ b/include/drm/dp/drm_dp_aux_bus.h
    > @@ -54,4 +54,62 @@ int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *aux_ep_drv,
    > struct module *owner);
    > void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver *aux_ep_drv);
    >
    > +/**
    > + * struct dp_aux_ep_device - Helper for clients of DP AUX EP devices

    dp_aux_ep_client?

    > + *
    > + * The DP AUX bus can be a bit tricky to use properly. Usually, the way
    > + * things work is that:
    > + * - The DP controller driver provides the DP AUX bus and would like to probe
    > + * the endpoints on the DP AUX bus (AKA the panel) as part of its probe
    > + * routine.
    > + * - The DP controller driver would also like to acquire a reference to the
    > + * DP AUX endpoints (AKA the panel) as part of its probe.
    > + *
    > + * The problem is that the DP AUX endpoints aren't guaranteed to complete their
    > + * probe right away. They could be probing asynchronously or they simply might
    > + * fail to acquire some resource and return -EPROBE_DEFER.
    > + *
    > + * The best way to solve this is to break the DP controller's probe into
    > + * two parts. The first part will create the DP AUX bus. The second part will
    > + * acquire the reference to the DP AUX endpoint. The first part can complete
    > + * finish with no problems and be "done" even if the second part ends up

    s/finish//

    > + * deferring while waiting for the DP AUX endpoint.
    > + *
    > + * The dp_aux_ep_client structure and associated functions help with managing
    > + * this common case. They will create/add a second "struct device" for you.

    create and add a second

    > + * In the probe for this second "struct device" (known as the "clientdev" here)
    > + * you can acquire references to the AUX DP endpoints and can freely return
    > + * -EPROBE_DEFER if they're not ready yet.
    > + *
    > + * A few notes:
    > + * - The parent of the clientdev is guaranteed to be aux->dev
    > + * - The of_node of the clientdev is guaranteed to be the same as the of_node
    > + * of aux->dev, copied with device_set_of_node_from_dev().
    > + * - If you're doing "devm" type things in the clientdev's probe you should
    > + * use the clientdev. This makes lifetimes be managed properly.
    > + *
    > + * NOTE: there's no requirement to use these helpers if you have solved the
    > + * problem described above in some other way.
    > + */

    \
     
     \ /
      Last update: 2022-04-15 01:55    [W:4.466 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site