lkml.org 
[lkml]   [2018]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer
    From
    Date
    On 2018-05-14 18:28, Daniel Vetter wrote:
    > On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
    >> On 2018-05-10 10:10, Andrzej Hajda wrote:
    >>> On 04.05.2018 15:52, Peter Rosin wrote:
    >>>> If the bridge supplier is unbound, this will bring the bridge consumer
    >>>> down along with the bridge. Thus, there will no longer linger any
    >>>> dangling pointers from the bridge consumer (the drm_device) to some
    >>>> non-existent bridge supplier.
    >>>>
    >>>> Signed-off-by: Peter Rosin <peda@axentia.se>
    >>>> ---
    >>>> drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
    >>>> include/drm/drm_bridge.h | 2 ++
    >>>> 2 files changed, 20 insertions(+)
    >>>>
    >>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
    >>>> index 78d186b6831b..0259f0a3ff27 100644
    >>>> --- a/drivers/gpu/drm/drm_bridge.c
    >>>> +++ b/drivers/gpu/drm/drm_bridge.c
    >>>> @@ -26,6 +26,7 @@
    >>>> #include <linux/mutex.h>
    >>>>
    >>>> #include <drm/drm_bridge.h>
    >>>> +#include <drm/drm_device.h>
    >>>> #include <drm/drm_encoder.h>
    >>>>
    >>>> #include "drm_crtc_internal.h"
    >>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
    >>>> if (bridge->dev)
    >>>> return -EBUSY;
    >>>>
    >>>> + if (encoder->dev->dev != bridge->odev) {
    >>>
    >>> I wonder why device_link_add does not handle this case (self dependency)
    >>> silently as noop, as it seems to be a correct behavior.
    >>
    >> It's kind-of a silly corner-case though, so perfectly understandable
    >> that it isn't handled.
    >>
    >>>> + bridge->link = device_link_add(encoder->dev->dev,
    >>>> + bridge->odev, 0);
    >>>> + if (!bridge->link) {
    >>>> + dev_err(bridge->odev, "failed to link bridge to %s\n",
    >>>> + dev_name(encoder->dev->dev));
    >>>> + return -EINVAL;
    >>>> + }
    >>>> + }
    >>>> +
    >>>> bridge->dev = encoder->dev;
    >>>> bridge->encoder = encoder;
    >>>>
    >>>> if (bridge->funcs->attach) {
    >>>> ret = bridge->funcs->attach(bridge);
    >>>> if (ret < 0) {
    >>>> + if (bridge->link)
    >>>> + device_link_del(bridge->link);
    >>>> + bridge->link = NULL;
    >>>> bridge->dev = NULL;
    >>>> bridge->encoder = NULL;
    >>>> return ret;
    >>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
    >>>> if (bridge->funcs->detach)
    >>>> bridge->funcs->detach(bridge);
    >>>>
    >>>> + if (bridge->link)
    >>>> + device_link_del(bridge->link);
    >>>> + bridge->link = NULL;
    >>>> +
    >>>> bridge->dev = NULL;
    >>>> }
    >>>>
    >>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
    >>>> index b656e505d11e..804189c63a4c 100644
    >>>> --- a/include/drm/drm_bridge.h
    >>>> +++ b/include/drm/drm_bridge.h
    >>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
    >>>> * @list: to keep track of all added bridges
    >>>> * @timings: the timing specification for the bridge, if any (may
    >>>> * be NULL)
    >>>> + * @link: drm consumer <-> bridge supplier
    >>>
    >>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
    >>> to the bridge" would be better.
    >>
    >> I meant "<->" to indicate that the link is bidirectional, not that the
    >> relationship is in any way symmetric. I wasn't aware of any implication
    >> of a symmetric relationship when using "<->", do you have a reference?
    >> But I guess the different arrow notations in math are somewhat overloaded
    >> and that someone at some point must have used "<->" to indicate a
    >> symmetric relationship...
    >
    > Yeah I agree with Andrzej here, for me <-> implies a symmetric
    > relationship. Spelling it out like Andrzej suggested sounds like the
    > better idea.
    > -Daniel

    Ok, I guess that means I have to do a v3 after all. Or can this
    trivial documentation update be done by the committer? I hate to
    spam everyone with another volley...

    Or perhaps I should squash patches 2-23 that are all rather similar
    and mechanic? I separated them to allow for easier review from
    individual driver maintainers, but that didn't seem to happen
    anyway...

    Cheers,
    Peter

    >
    >>
    >>> Anyway:
    >>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
    >>
    >> Thanks!
    >>
    >> Cheers,
    >> Peter
    >>
    >>>  --
    >>> Regards
    >>> Andrzej
    >>>
    >>>> * @funcs: control functions
    >>>> * @driver_private: pointer to the bridge driver's internal context
    >>>> */
    >>>> @@ -271,6 +272,7 @@ struct drm_bridge {
    >>>> struct drm_bridge *next;
    >>>> struct list_head list;
    >>>> const struct drm_bridge_timings *timings;
    >>>> + struct device_link *link;
    >>>>
    >>>> const struct drm_bridge_funcs *funcs;
    >>>> void *driver_private;
    >>>
    >>>
    >>
    >

    \
     
     \ /
      Last update: 2018-05-14 22:42    [W:2.458 / U:0.464 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site