[V7,03/12] drm/bridge: Add helper functions for drm_bridge

Commit Message

Ajay Kumar Aug. 27, 2014, 2:29 p.m. UTC

A set of helper functions are defined in this patch to make
bridge driver probe independent of the drm flow.

The bridge devices register themselves on a lookup table
when they get probed by calling "drm_bridge_add".

The parent encoder driver waits till the bridge is available
in the lookup table(by calling "of_drm_find_bridge") and then
continues with its initialization.

The encoder driver should also call "drm_bridge_attach" to pass
on the drm_device, encoder pointers to the bridge object.

drm_bridge_attach inturn calls drm_bridge_init to register itself
with the drm core. Later, it calls "bridge->funcs->attach" so that
bridge can continue with other initializations.

Signed-off-by: Ajay Kumar <[e-mail]>
---
 drivers/gpu/drm/Makefile               |    1 +
 drivers/gpu/drm/bridge/Kconfig         |   15 ++++-
 drivers/gpu/drm/drm_bridge.c           |  102 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_crtc.c             |    4 +-
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |    4 +-
 include/drm/drm_crtc.h                 |   12 +++-
 6 files changed, 131 insertions(+), 7 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_bridge.c

Comments

Daniel Vetter Oct. 27, 2014, 7:01 p.m. UTC | #1

So don't ask why but I accidentally ended up in a branch looking at this
patch and didn't like it. So very quick&grumpy review.

First, please make the patch subject more descriptive: I'd expect a helper
function scaffolding like the various crtc/probe/dp ... helpers we already
have. You instead add code to untangle the probe ordering. Please say so.

More comments below.

On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
> A set of helper functions are defined in this patch to make
> bridge driver probe independent of the drm flow.
> 
> The bridge devices register themselves on a lookup table
> when they get probed by calling "drm_bridge_add".
> 
> The parent encoder driver waits till the bridge is available
> in the lookup table(by calling "of_drm_find_bridge") and then
> continues with its initialization.
> 
> The encoder driver should also call "drm_bridge_attach" to pass
> on the drm_device, encoder pointers to the bridge object.
> 
> drm_bridge_attach inturn calls drm_bridge_init to register itself
> with the drm core. Later, it calls "bridge->funcs->attach" so that
> bridge can continue with other initializations.
> 
> Signed-off-by: Ajay Kumar <[e-mail]>

[snip]

> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>   * @driver_private: pointer to the bridge driver's internal context
>   */
>  struct drm_bridge {
> -	struct drm_device *dev;
> +	struct device *dev;

Please don't rename the ->dev pointer into drm. Because _all_ the other
drm structures still call it ->dev. Also, can't we use struct device_node
here like we do in the of helpers Russell added? See 7e435aad38083

> +	struct drm_device *drm;
> +	struct drm_encoder *encoder;

This breaks bridge->bridge chaining (if we ever get there). It seems
pretty much unused anyway except for an EBUSY check. Can't you use
bridge->dev for that?

>  	struct list_head head;
> +	struct list_head list;

These lists need better names. I know that the "head" is really awful,
especially since it's actually not the head of the list but just an
element.
>  
>  	struct drm_mode_object base;


Aside: I've noticed all this trying to update the kerneldoc for struct
drm_bridge, which just showed that this patch makes inconsistent changes.
Trying to write kerneldoc is a really great way to come up with better
interfaces imo.

Cheers, Daniel

>  
> @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector);
>  /* helper to unplug all connectors from sysfs for device */
>  extern void drm_connector_unplug_all(struct drm_device *dev);
>  
> +extern int drm_bridge_add(struct drm_bridge *bridge);
> +extern void drm_bridge_remove(struct drm_bridge *bridge);
> +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> +extern int drm_bridge_attach(struct drm_bridge *bridge,
> +				struct drm_encoder *encoder);
>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge);
>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>  
> -- 
> 1.7.9.5
>

Sean Paul Oct. 27, 2014, 7:58 p.m. UTC | #2

On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <[e-mail]> wrote:
> So don't ask why but I accidentally ended up in a branch looking at this
> patch and didn't like it. So very quick&grumpy review.
>
> First, please make the patch subject more descriptive: I'd expect a helper
> function scaffolding like the various crtc/probe/dp ... helpers we already
> have. You instead add code to untangle the probe ordering. Please say so.
>
> More comments below.
>
> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
>> A set of helper functions are defined in this patch to make
>> bridge driver probe independent of the drm flow.
>>
>> The bridge devices register themselves on a lookup table
>> when they get probed by calling "drm_bridge_add".
>>
>> The parent encoder driver waits till the bridge is available
>> in the lookup table(by calling "of_drm_find_bridge") and then
>> continues with its initialization.
>>
>> The encoder driver should also call "drm_bridge_attach" to pass
>> on the drm_device, encoder pointers to the bridge object.
>>
>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>> bridge can continue with other initializations.
>>
>> Signed-off-by: Ajay Kumar <[e-mail]>
>
> [snip]
>
>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>>  struct drm_bridge {
>> -     struct drm_device *dev;
>> +     struct device *dev;
>
> Please don't rename the ->dev pointer into drm. Because _all_ the other
> drm structures still call it ->dev. Also, can't we use struct device_node
> here like we do in the of helpers Russell added? See 7e435aad38083
>

I think this is modeled after the naming in drm_panel, FWIW. However,
seems reasonable to keep the device_node instead.

>> +     struct drm_device *drm;
>> +     struct drm_encoder *encoder;
>
> This breaks bridge->bridge chaining (if we ever get there). It seems
> pretty much unused anyway except for an EBUSY check. Can't you use
> bridge->dev for that?
>

Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
and leave it up to the caller to establish the proper chain.

>>       struct list_head head;
>> +     struct list_head list;
>
> These lists need better names. I know that the "head" is really awful,
> especially since it's actually not the head of the list but just an
> element.

I think we can just rip bridge_list out of mode_config if we're going
to keep track of bridges elsewhere. So we can nuke "head" and keep
"list". This also means that bridge->destroy() goes away, but that's
probably Ok if everything converts to the standalone driver model
where we have driver->remove()

Sean

>>
>>       struct drm_mode_object base;
>
>
> Aside: I've noticed all this trying to update the kerneldoc for struct
> drm_bridge, which just showed that this patch makes inconsistent changes.
> Trying to write kerneldoc is a really great way to come up with better
> interfaces imo.
>
> Cheers, Daniel
>
>>
>> @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector);
>>  /* helper to unplug all connectors from sysfs for device */
>>  extern void drm_connector_unplug_all(struct drm_device *dev);
>>
>> +extern int drm_bridge_add(struct drm_bridge *bridge);
>> +extern void drm_bridge_remove(struct drm_bridge *bridge);
>> +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>> +extern int drm_bridge_attach(struct drm_bridge *bridge,
>> +                             struct drm_encoder *encoder);
>>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge);
>>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>>
>> --
>> 1.7.9.5
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch

Sean Paul Oct. 27, 2014, 8:11 p.m. UTC | #3

On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar <[e-mail]> wrote:
> A set of helper functions are defined in this patch to make
> bridge driver probe independent of the drm flow.
>
> The bridge devices register themselves on a lookup table
> when they get probed by calling "drm_bridge_add".
>
> The parent encoder driver waits till the bridge is available
> in the lookup table(by calling "of_drm_find_bridge") and then
> continues with its initialization.
>
> The encoder driver should also call "drm_bridge_attach" to pass
> on the drm_device, encoder pointers to the bridge object.
>
> drm_bridge_attach inturn calls drm_bridge_init to register itself
> with the drm core. Later, it calls "bridge->funcs->attach" so that
> bridge can continue with other initializations.
>
> Signed-off-by: Ajay Kumar <[e-mail]>
> ---
>  drivers/gpu/drm/Makefile               |    1 +
>  drivers/gpu/drm/bridge/Kconfig         |   15 ++++-
>  drivers/gpu/drm/drm_bridge.c           |  102 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc.c             |    4 +-
>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |    4 +-
>  include/drm/drm_crtc.h                 |   12 +++-
>  6 files changed, 131 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_bridge.c
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 4a55d59..bdbfb6f 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -19,6 +19,7 @@ drm-y       :=        drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>  drm-$(CONFIG_PCI) += ati_pcigart.o
> +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o
>  drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>  drm-$(CONFIG_OF) += drm_of.o
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 884923f..5a8e907 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -1,5 +1,16 @@
> -config DRM_PTN3460
> -       tristate "PTN3460 DP/LVDS bridge"
> +config DRM_BRIDGE

I'm not convinced this adds any value, to be honest. In addition to
whether or not it's useful, it seems like you'd need to stub the
drm_bridge_* functions that are declared in drm_crtc.h or break them
out into drm_bridge.h.

Sean

> +       tristate
>         depends on DRM
>         select DRM_KMS_HELPER
> +       help
> +         Bridge registration and lookup framework.
> +
> +menu "bridge chips"
> +       depends on DRM_BRIDGE
> +
> +config DRM_PTN3460
> +       tristate "PTN3460 DP/LVDS bridge"
> +       depends on DRM_BRIDGE
>         ---help---
> +
> +endmenu
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> new file mode 100644
> index 0000000..b2d43fd
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -0,0 +1,102 @@
> +/*
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sub license,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +
> +#include <drm/drm_crtc.h>
> +
> +#include "drm/drmP.h"
> +
> +static DEFINE_MUTEX(bridge_lock);
> +static LIST_HEAD(bridge_list);
> +
> +int drm_bridge_add(struct drm_bridge *bridge)
> +{
> +       mutex_lock(&bridge_lock);
> +       list_add_tail(&bridge->list, &bridge_list);
> +       mutex_unlock(&bridge_lock);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(drm_bridge_add);
> +
> +void drm_bridge_remove(struct drm_bridge *bridge)
> +{
> +       mutex_lock(&bridge_lock);
> +       list_del_init(&bridge->list);
> +       mutex_unlock(&bridge_lock);
> +}
> +EXPORT_SYMBOL(drm_bridge_remove);
> +
> +int drm_bridge_attach(struct drm_bridge *bridge,
> +                               struct drm_encoder *encoder)
> +{
> +       int ret;
> +
> +       if (!bridge || !encoder)
> +               return -EINVAL;
> +
> +       if (bridge->encoder)
> +               return -EBUSY;
> +
> +       encoder->bridge = bridge;
> +       bridge->encoder = encoder;
> +       bridge->drm = encoder->dev;
> +
> +       ret = drm_bridge_init(bridge->drm, bridge);
> +       if (ret) {
> +               DRM_ERROR("Failed to register bridge with drm\n");
> +               return ret;
> +       }
> +
> +       if (bridge->funcs->attach)
> +               return bridge->funcs->attach(bridge);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(drm_bridge_attach);
> +
> +#ifdef CONFIG_OF
> +struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> +{
> +       struct drm_bridge *bridge;
> +
> +       mutex_lock(&bridge_lock);
> +
> +       list_for_each_entry(bridge, &bridge_list, list) {
> +               if (bridge->dev->of_node == np) {
> +                       mutex_unlock(&bridge_lock);
> +                       return bridge;
> +               }
> +       }
> +
> +       mutex_unlock(&bridge_lock);
> +       return NULL;
> +}
> +EXPORT_SYMBOL(of_drm_find_bridge);
> +#endif
> +
> +MODULE_AUTHOR("Ajay Kumar <[e-mail]>");
> +MODULE_DESCRIPTION("DRM bridge infrastructure");
> +MODULE_LICENSE("GPL and additional rights");
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 25f5cfa..2fb22fa 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1039,7 +1039,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge)
>         if (ret)
>                 goto out;
>
> -       bridge->dev = dev;
> +       bridge->drm = dev;
>
>         list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
>         dev->mode_config.num_bridge++;
> @@ -1058,7 +1058,7 @@ EXPORT_SYMBOL(drm_bridge_init);
>   */
>  void drm_bridge_cleanup(struct drm_bridge *bridge)
>  {
> -       struct drm_device *dev = bridge->dev;
> +       struct drm_device *dev = bridge->drm;
>
>         drm_modeset_lock_all(dev);
>         drm_mode_object_put(dev, &bridge->base);
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 0309539..bc9e5ff 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -33,7 +33,7 @@ static void hdmi_bridge_destroy(struct drm_bridge *bridge)
>
>  static void power_on(struct drm_bridge *bridge)
>  {
> -       struct drm_device *dev = bridge->dev;
> +       struct drm_device *dev = bridge->drm;
>         struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>         struct hdmi *hdmi = hdmi_bridge->hdmi;
>         const struct hdmi_platform_config *config = hdmi->config;
> @@ -67,7 +67,7 @@ static void power_on(struct drm_bridge *bridge)
>
>  static void power_off(struct drm_bridge *bridge)
>  {
> -       struct drm_device *dev = bridge->dev;
> +       struct drm_device *dev = bridge->drm;
>         struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>         struct hdmi *hdmi = hdmi_bridge->hdmi;
>         const struct hdmi_platform_config *config = hdmi->config;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f48a436..f6f426f 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -629,6 +629,7 @@ struct drm_plane {
>
>  /**
>   * drm_bridge_funcs - drm_bridge control functions
> + * @attach: Called during drm_bridge_attach
>   * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge
>   * @disable: Called right before encoder prepare, disables the bridge
>   * @post_disable: Called right after encoder prepare, for lockstepped disable
> @@ -638,6 +639,7 @@ struct drm_plane {
>   * @destroy: make object go away
>   */
>  struct drm_bridge_funcs {
> +       int (*attach)(struct drm_bridge *bridge);
>         bool (*mode_fixup)(struct drm_bridge *bridge,
>                            const struct drm_display_mode *mode,
>                            struct drm_display_mode *adjusted_mode);
> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>   * @driver_private: pointer to the bridge driver's internal context
>   */
>  struct drm_bridge {
> -       struct drm_device *dev;
> +       struct device *dev;
> +       struct drm_device *drm;
> +       struct drm_encoder *encoder;
>         struct list_head head;
> +       struct list_head list;
>
>         struct drm_mode_object base;
>
> @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector);
>  /* helper to unplug all connectors from sysfs for device */
>  extern void drm_connector_unplug_all(struct drm_device *dev);
>
> +extern int drm_bridge_add(struct drm_bridge *bridge);
> +extern void drm_bridge_remove(struct drm_bridge *bridge);
> +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> +extern int drm_bridge_attach(struct drm_bridge *bridge,
> +                               struct drm_encoder *encoder);
>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge);
>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Daniel Vetter Oct. 27, 2014, 10:20 p.m. UTC | #4

On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>>   * @driver_private: pointer to the bridge driver's internal context
>>>   */
>>>  struct drm_bridge {
>>> -     struct drm_device *dev;
>>> +     struct device *dev;
>>
>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>> drm structures still call it ->dev. Also, can't we use struct device_node
>> here like we do in the of helpers Russell added? See 7e435aad38083
>>
>
> I think this is modeled after the naming in drm_panel, FWIW. However,
> seems reasonable to keep the device_node instead.

Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
drm_crtc drop the struct device and go directly to a struct
device_node. Since we don't really need the sturct device, the only
thing we care about is the of_node. For added bonus wrap an #ifdef
CONFIG_OF around all the various struct device_node in drm_foo.h.
Should be all fairly simple to pull off with cocci.

Thierry?
-Daniel

Daniel Vetter Oct. 27, 2014, 10:26 p.m. UTC | #5

On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <[e-mail]> wrote:
> On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
>>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>   */
>>>>  struct drm_bridge {
>>>> -     struct drm_device *dev;
>>>> +     struct device *dev;
>>>
>>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>>> drm structures still call it ->dev. Also, can't we use struct device_node
>>> here like we do in the of helpers Russell added? See 7e435aad38083
>>>
>>
>> I think this is modeled after the naming in drm_panel, FWIW. However,
>> seems reasonable to keep the device_node instead.
>
> Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> drm_crtc drop the struct device and go directly to a struct
> device_node. Since we don't really need the sturct device, the only
> thing we care about is the of_node. For added bonus wrap an #ifdef
> CONFIG_OF around all the various struct device_node in drm_foo.h.
> Should be all fairly simple to pull off with cocci.
>
> Thierry?

Looking at the of_drm_find_panel function I actually wonder how that
works - the drm_panel doesn't really need to stick around afaics.
After all panel_list is global so some other driver can unload.
Russell's of support for possible_crtcs code works differently since
it only looks at per-drm_device lists.

This bridge code here though suffers from the same. So to me this
looks rather fishy.

It doesn't help that we have drm_of.[hc] around but not all the of
code is in there. Adding Russell too.
-Daniel

Ajay kumar Oct. 28, 2014, 9:21 a.m. UTC | #6

Hi Daniel and Sean,

Thanks for the comments!

On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <[e-mail]> wrote:
> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <[e-mail]> wrote:
>> So don't ask why but I accidentally ended up in a branch looking at this
>> patch and didn't like it. So very quick&grumpy review.
>>
>> First, please make the patch subject more descriptive: I'd expect a helper
>> function scaffolding like the various crtc/probe/dp ... helpers we already
>> have. You instead add code to untangle the probe ordering. Please say so.
Sure. I will reword it properly.

>> More comments below.
>>
>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
>>> A set of helper functions are defined in this patch to make
>>> bridge driver probe independent of the drm flow.
>>>
>>> The bridge devices register themselves on a lookup table
>>> when they get probed by calling "drm_bridge_add".
>>>
>>> The parent encoder driver waits till the bridge is available
>>> in the lookup table(by calling "of_drm_find_bridge") and then
>>> continues with its initialization.
>>>
>>> The encoder driver should also call "drm_bridge_attach" to pass
>>> on the drm_device, encoder pointers to the bridge object.
>>>
>>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>>> bridge can continue with other initializations.
>>>
>>> Signed-off-by: Ajay Kumar <[e-mail]>
>>
>> [snip]
>>
>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>>   * @driver_private: pointer to the bridge driver's internal context
>>>   */
>>>  struct drm_bridge {
>>> -     struct drm_device *dev;
>>> +     struct device *dev;
>>
>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>> drm structures still call it ->dev. Also, can't we use struct device_node
>> here like we do in the of helpers Russell added? See 7e435aad38083
>>
>
> I think this is modeled after the naming in drm_panel,
Right, The entire rework is based on how drm_panel framework is structured.

> FWIW. However,
> seems reasonable to keep the device_node instead.
Yes, its visible that just device_node would be sufficient.
This will save us from renaming drm_device as well.

>>> +     struct drm_device *drm;
>>> +     struct drm_encoder *encoder;
>>
>> This breaks bridge->bridge chaining (if we ever get there). It seems
>> pretty much unused anyway except for an EBUSY check. Can't you use
>> bridge->dev for that?
>>
>
> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
> and leave it up to the caller to establish the proper chain.
Ok. I will use drm_device pointer directly instead of passing encoder pointer.

>>>       struct list_head head;
>>> +     struct list_head list;
>>
>> These lists need better names. I know that the "head" is really awful,
>> especially since it's actually not the head of the list but just an
>> element.
>
> I think we can just rip bridge_list out of mode_config if we're going
> to keep track of bridges elsewhere. So we can nuke "head" and keep
> "list". This also means that bridge->destroy() goes away, but that's
> probably Ok if everything converts to the standalone driver model
> where we have driver->remove()
>
> Sean
Great! Thierry actually mentioned about this once, and we have the
confirmation now.

>>>
>>>       struct drm_mode_object base;
>>
>>
>> Aside: I've noticed all this trying to update the kerneldoc for struct
>> drm_bridge, which just showed that this patch makes inconsistent changes.
>> Trying to write kerneldoc is a really great way to come up with better
>> interfaces imo.
>>
>> Cheers, Daniel
I din't get this actually. You want me to create Doc../drm_bridge.txt
or something similar?

Ajay

Ajay kumar Oct. 28, 2014, 9:22 a.m. UTC | #7

On Tue, Oct 28, 2014 at 1:41 AM, Sean Paul <[e-mail]> wrote:
> On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar <[e-mail]> wrote:
>> A set of helper functions are defined in this patch to make
>> bridge driver probe independent of the drm flow.
>>
>> The bridge devices register themselves on a lookup table
>> when they get probed by calling "drm_bridge_add".
>>
>> The parent encoder driver waits till the bridge is available
>> in the lookup table(by calling "of_drm_find_bridge") and then
>> continues with its initialization.
>>
>> The encoder driver should also call "drm_bridge_attach" to pass
>> on the drm_device, encoder pointers to the bridge object.
>>
>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>> bridge can continue with other initializations.
>>
>> Signed-off-by: Ajay Kumar <[e-mail]>
>> ---
>>  drivers/gpu/drm/Makefile               |    1 +
>>  drivers/gpu/drm/bridge/Kconfig         |   15 ++++-
>>  drivers/gpu/drm/drm_bridge.c           |  102 ++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_crtc.c             |    4 +-
>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |    4 +-
>>  include/drm/drm_crtc.h                 |   12 +++-
>>  6 files changed, 131 insertions(+), 7 deletions(-)
>>  create mode 100644 drivers/gpu/drm/drm_bridge.c
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 4a55d59..bdbfb6f 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -19,6 +19,7 @@ drm-y       :=        drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
>>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>>  drm-$(CONFIG_PCI) += ati_pcigart.o
>> +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o
>>  drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>>  drm-$(CONFIG_OF) += drm_of.o
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index 884923f..5a8e907 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -1,5 +1,16 @@
>> -config DRM_PTN3460
>> -       tristate "PTN3460 DP/LVDS bridge"
>> +config DRM_BRIDGE
>
> I'm not convinced this adds any value, to be honest. In addition to
> whether or not it's useful, it seems like you'd need to stub the
> drm_bridge_* functions that are declared in drm_crtc.h or break them
> out into drm_bridge.h.
>
> Sean
>
>> +       tristate
>>         depends on DRM
>>         select DRM_KMS_HELPER
>> +       help
>> +         Bridge registration and lookup framework.
>> +
>> +menu "bridge chips"
>> +       depends on DRM_BRIDGE
>> +
>> +config DRM_PTN3460
>> +       tristate "PTN3460 DP/LVDS bridge"
>> +       depends on DRM_BRIDGE
>>         ---help---
>> +
>> +endmenu
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> new file mode 100644
>> index 0000000..b2d43fd
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -0,0 +1,102 @@
>> +/*
>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sub license,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the
>> + * next paragraph) shall be included in all copies or substantial portions
>> + * of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +
>> +#include <drm/drm_crtc.h>
>> +
>> +#include "drm/drmP.h"
>> +
>> +static DEFINE_MUTEX(bridge_lock);
>> +static LIST_HEAD(bridge_list);
>> +
>> +int drm_bridge_add(struct drm_bridge *bridge)
>> +{
>> +       mutex_lock(&bridge_lock);
>> +       list_add_tail(&bridge->list, &bridge_list);
>> +       mutex_unlock(&bridge_lock);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(drm_bridge_add);
>> +
>> +void drm_bridge_remove(struct drm_bridge *bridge)
>> +{
>> +       mutex_lock(&bridge_lock);
>> +       list_del_init(&bridge->list);
>> +       mutex_unlock(&bridge_lock);
>> +}
>> +EXPORT_SYMBOL(drm_bridge_remove);
>> +
>> +int drm_bridge_attach(struct drm_bridge *bridge,
>> +                               struct drm_encoder *encoder)
>> +{
>> +       int ret;
>> +
>> +       if (!bridge || !encoder)
>> +               return -EINVAL;
>> +
>> +       if (bridge->encoder)
>> +               return -EBUSY;
>> +
>> +       encoder->bridge = bridge;
>> +       bridge->encoder = encoder;
>> +       bridge->drm = encoder->dev;
>> +
>> +       ret = drm_bridge_init(bridge->drm, bridge);
>> +       if (ret) {
>> +               DRM_ERROR("Failed to register bridge with drm\n");
>> +               return ret;
>> +       }
>> +
>> +       if (bridge->funcs->attach)
>> +               return bridge->funcs->attach(bridge);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(drm_bridge_attach);
>> +
>> +#ifdef CONFIG_OF
>> +struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>> +{
>> +       struct drm_bridge *bridge;
>> +
>> +       mutex_lock(&bridge_lock);
>> +
>> +       list_for_each_entry(bridge, &bridge_list, list) {
>> +               if (bridge->dev->of_node == np) {
>> +                       mutex_unlock(&bridge_lock);
>> +                       return bridge;
>> +               }
>> +       }
>> +
>> +       mutex_unlock(&bridge_lock);
>> +       return NULL;
>> +}
>> +EXPORT_SYMBOL(of_drm_find_bridge);
>> +#endif
>> +
>> +MODULE_AUTHOR("Ajay Kumar <[e-mail]>");
>> +MODULE_DESCRIPTION("DRM bridge infrastructure");
>> +MODULE_LICENSE("GPL and additional rights");
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 25f5cfa..2fb22fa 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -1039,7 +1039,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge)
>>         if (ret)
>>                 goto out;
>>
>> -       bridge->dev = dev;
>> +       bridge->drm = dev;
>>
>>         list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
>>         dev->mode_config.num_bridge++;
>> @@ -1058,7 +1058,7 @@ EXPORT_SYMBOL(drm_bridge_init);
>>   */
>>  void drm_bridge_cleanup(struct drm_bridge *bridge)
>>  {
>> -       struct drm_device *dev = bridge->dev;
>> +       struct drm_device *dev = bridge->drm;
>>
>>         drm_modeset_lock_all(dev);
>>         drm_mode_object_put(dev, &bridge->base);
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>> index 0309539..bc9e5ff 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>> @@ -33,7 +33,7 @@ static void hdmi_bridge_destroy(struct drm_bridge *bridge)
>>
>>  static void power_on(struct drm_bridge *bridge)
>>  {
>> -       struct drm_device *dev = bridge->dev;
>> +       struct drm_device *dev = bridge->drm;
>>         struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>>         struct hdmi *hdmi = hdmi_bridge->hdmi;
>>         const struct hdmi_platform_config *config = hdmi->config;
>> @@ -67,7 +67,7 @@ static void power_on(struct drm_bridge *bridge)
>>
>>  static void power_off(struct drm_bridge *bridge)
>>  {
>> -       struct drm_device *dev = bridge->dev;
>> +       struct drm_device *dev = bridge->drm;
>>         struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>>         struct hdmi *hdmi = hdmi_bridge->hdmi;
>>         const struct hdmi_platform_config *config = hdmi->config;
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index f48a436..f6f426f 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -629,6 +629,7 @@ struct drm_plane {
>>
>>  /**
>>   * drm_bridge_funcs - drm_bridge control functions
>> + * @attach: Called during drm_bridge_attach
>>   * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge
>>   * @disable: Called right before encoder prepare, disables the bridge
>>   * @post_disable: Called right after encoder prepare, for lockstepped disable
>> @@ -638,6 +639,7 @@ struct drm_plane {
>>   * @destroy: make object go away
>>   */
>>  struct drm_bridge_funcs {
>> +       int (*attach)(struct drm_bridge *bridge);
>>         bool (*mode_fixup)(struct drm_bridge *bridge,
>>                            const struct drm_display_mode *mode,
>>                            struct drm_display_mode *adjusted_mode);
>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>>  struct drm_bridge {
>> -       struct drm_device *dev;
>> +       struct device *dev;
>> +       struct drm_device *drm;
>> +       struct drm_encoder *encoder;
>>         struct list_head head;
>> +       struct list_head list;
>>
>>         struct drm_mode_object base;
>>
>> @@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector);
>>  /* helper to unplug all connectors from sysfs for device */
>>  extern void drm_connector_unplug_all(struct drm_device *dev);
>>
>> +extern int drm_bridge_add(struct drm_bridge *bridge);
>> +extern void drm_bridge_remove(struct drm_bridge *bridge);
>> +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>> +extern int drm_bridge_attach(struct drm_bridge *bridge,
>> +                               struct drm_encoder *encoder);
>>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge);
>>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>>
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ajay kumar Oct. 28, 2014, 9:26 a.m. UTC | #8

On Tue, Oct 28, 2014 at 1:41 AM, Sean Paul <[e-mail]> wrote:
> On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar <[e-mail]> wrote:
>> A set of helper functions are defined in this patch to make
>> bridge driver probe independent of the drm flow.
>>
>> The bridge devices register themselves on a lookup table
>> when they get probed by calling "drm_bridge_add".
>>
>> The parent encoder driver waits till the bridge is available
>> in the lookup table(by calling "of_drm_find_bridge") and then
>> continues with its initialization.
>>
>> The encoder driver should also call "drm_bridge_attach" to pass
>> on the drm_device, encoder pointers to the bridge object.
>>
>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>> bridge can continue with other initializations.
>>
>> Signed-off-by: Ajay Kumar <[e-mail]>
>> ---
>>  drivers/gpu/drm/Makefile               |    1 +
>>  drivers/gpu/drm/bridge/Kconfig         |   15 ++++-
>>  drivers/gpu/drm/drm_bridge.c           |  102 ++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_crtc.c             |    4 +-
>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |    4 +-
>>  include/drm/drm_crtc.h                 |   12 +++-
>>  6 files changed, 131 insertions(+), 7 deletions(-)
>>  create mode 100644 drivers/gpu/drm/drm_bridge.c
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 4a55d59..bdbfb6f 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -19,6 +19,7 @@ drm-y       :=        drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
>>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>>  drm-$(CONFIG_PCI) += ati_pcigart.o
>> +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o
>>  drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>>  drm-$(CONFIG_OF) += drm_of.o
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index 884923f..5a8e907 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -1,5 +1,16 @@
>> -config DRM_PTN3460
>> -       tristate "PTN3460 DP/LVDS bridge"
>> +config DRM_BRIDGE
>
> I'm not convinced this adds any value, to be honest.
Hmm, then how to compile drm_bridge.c?

> In addition to
> whether or not it's useful, it seems like you'd need to stub the
> drm_bridge_* functions that are declared in drm_crtc.h or break them
> out into drm_bridge.h.
Well, Thierry mentioned about this long back. Again, we have the
confirmation now!

Ajay
[snip]

Daniel Vetter Oct. 28, 2014, 10:01 a.m. UTC | #9

On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar <[e-mail]> wrote:
> Hi Daniel and Sean,
>
> Thanks for the comments!
>
> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <[e-mail]> wrote:
>> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <[e-mail]> wrote:
>>> So don't ask why but I accidentally ended up in a branch looking at this
>>> patch and didn't like it. So very quick&grumpy review.
>>>
>>> First, please make the patch subject more descriptive: I'd expect a helper
>>> function scaffolding like the various crtc/probe/dp ... helpers we already
>>> have. You instead add code to untangle the probe ordering. Please say so.
> Sure. I will reword it properly.
>
>>> More comments below.
>>>
>>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
>>>> A set of helper functions are defined in this patch to make
>>>> bridge driver probe independent of the drm flow.
>>>>
>>>> The bridge devices register themselves on a lookup table
>>>> when they get probed by calling "drm_bridge_add".
>>>>
>>>> The parent encoder driver waits till the bridge is available
>>>> in the lookup table(by calling "of_drm_find_bridge") and then
>>>> continues with its initialization.
>>>>
>>>> The encoder driver should also call "drm_bridge_attach" to pass
>>>> on the drm_device, encoder pointers to the bridge object.
>>>>
>>>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>>>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>>>> bridge can continue with other initializations.
>>>>
>>>> Signed-off-by: Ajay Kumar <[e-mail]>
>>>
>>> [snip]
>>>
>>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>   */
>>>>  struct drm_bridge {
>>>> -     struct drm_device *dev;
>>>> +     struct device *dev;
>>>
>>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>>> drm structures still call it ->dev. Also, can't we use struct device_node
>>> here like we do in the of helpers Russell added? See 7e435aad38083
>>>
>>
>> I think this is modeled after the naming in drm_panel,
> Right, The entire rework is based on how drm_panel framework is structured.
>
>> FWIW. However,
>> seems reasonable to keep the device_node instead.
> Yes, its visible that just device_node would be sufficient.
> This will save us from renaming drm_device as well.
>
>>>> +     struct drm_device *drm;
>>>> +     struct drm_encoder *encoder;
>>>
>>> This breaks bridge->bridge chaining (if we ever get there). It seems
>>> pretty much unused anyway except for an EBUSY check. Can't you use
>>> bridge->dev for that?
>>>
>>
>> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
>> and leave it up to the caller to establish the proper chain.
> Ok. I will use drm_device pointer directly instead of passing encoder pointer.

Hm, if you do this can you pls also update drm_panel accordingly? It
shouldn't be a lot of fuzz and would make things around drm+dt more
consistent.

>
>>>>       struct list_head head;
>>>> +     struct list_head list;
>>>
>>> These lists need better names. I know that the "head" is really awful,
>>> especially since it's actually not the head of the list but just an
>>> element.
>>
>> I think we can just rip bridge_list out of mode_config if we're going
>> to keep track of bridges elsewhere. So we can nuke "head" and keep
>> "list". This also means that bridge->destroy() goes away, but that's
>> probably Ok if everything converts to the standalone driver model
>> where we have driver->remove()
>>
>> Sean
> Great! Thierry actually mentioned about this once, and we have the
> confirmation now.
>
>>>>
>>>>       struct drm_mode_object base;
>>>
>>>
>>> Aside: I've noticed all this trying to update the kerneldoc for struct
>>> drm_bridge, which just showed that this patch makes inconsistent changes.
>>> Trying to write kerneldoc is a really great way to come up with better
>>> interfaces imo.
>>>
>>> Cheers, Daniel
> I din't get this actually. You want me to create Doc../drm_bridge.txt
> or something similar?

If you want to document drm_bridge then I recomment to sprinkle proper
kerneldoc over drm_bridge.c and pull it all into the drm DocBook
template. That way all the drm documentation is in one place. I've
done that for drm_crtc.h in an unrelated patch series (but based upon
a branch with your patch here included) and there's struct drm_bridge*
in there. Hence why I've noticed.

If you do kerneldoc/DocBook integration for drm_bridge it's probably
best to also do it for drm_panel and use the opportunity to
review/rework the interfaces a bit for consistency. E.g. move dt
related stuff into drm_of.c and have that in a separate section in the
docs.
-Daniel

Ajay kumar Oct. 28, 2014, 12:28 p.m. UTC | #10

On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <[e-mail]> wrote:
> On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar <[e-mail]> wrote:
>> Hi Daniel and Sean,
>>
>> Thanks for the comments!
>>
>> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <[e-mail]> wrote:
>>> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <[e-mail]> wrote:
>>>> So don't ask why but I accidentally ended up in a branch looking at this
>>>> patch and didn't like it. So very quick&grumpy review.
>>>>
>>>> First, please make the patch subject more descriptive: I'd expect a helper
>>>> function scaffolding like the various crtc/probe/dp ... helpers we already
>>>> have. You instead add code to untangle the probe ordering. Please say so.
>> Sure. I will reword it properly.
>>
>>>> More comments below.
>>>>
>>>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
>>>>> A set of helper functions are defined in this patch to make
>>>>> bridge driver probe independent of the drm flow.
>>>>>
>>>>> The bridge devices register themselves on a lookup table
>>>>> when they get probed by calling "drm_bridge_add".
>>>>>
>>>>> The parent encoder driver waits till the bridge is available
>>>>> in the lookup table(by calling "of_drm_find_bridge") and then
>>>>> continues with its initialization.
>>>>>
>>>>> The encoder driver should also call "drm_bridge_attach" to pass
>>>>> on the drm_device, encoder pointers to the bridge object.
>>>>>
>>>>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>>>>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>>>>> bridge can continue with other initializations.
>>>>>
>>>>> Signed-off-by: Ajay Kumar <[e-mail]>
>>>>
>>>> [snip]
>>>>
>>>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>>   */
>>>>>  struct drm_bridge {
>>>>> -     struct drm_device *dev;
>>>>> +     struct device *dev;
>>>>
>>>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>>>> drm structures still call it ->dev. Also, can't we use struct device_node
>>>> here like we do in the of helpers Russell added? See 7e435aad38083
>>>>
>>>
>>> I think this is modeled after the naming in drm_panel,
>> Right, The entire rework is based on how drm_panel framework is structured.
>>
>>> FWIW. However,
>>> seems reasonable to keep the device_node instead.
>> Yes, its visible that just device_node would be sufficient.
>> This will save us from renaming drm_device as well.
>>
>>>>> +     struct drm_device *drm;
>>>>> +     struct drm_encoder *encoder;
>>>>
>>>> This breaks bridge->bridge chaining (if we ever get there). It seems
>>>> pretty much unused anyway except for an EBUSY check. Can't you use
>>>> bridge->dev for that?
>>>>
>>>
>>> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
>>> and leave it up to the caller to establish the proper chain.
>> Ok. I will use drm_device pointer directly instead of passing encoder pointer.
>
> Hm, if you do this can you pls also update drm_panel accordingly? It
> shouldn't be a lot of fuzz and would make things around drm+dt more
> consistent.
Are you talking about using struct device_node instead of struct device?
I guess you have misplaced the comment under the wrong section!

>>
>>>>>       struct list_head head;
>>>>> +     struct list_head list;
>>>>
>>>> These lists need better names. I know that the "head" is really awful,
>>>> especially since it's actually not the head of the list but just an
>>>> element.
>>>
>>> I think we can just rip bridge_list out of mode_config if we're going
>>> to keep track of bridges elsewhere. So we can nuke "head" and keep
>>> "list". This also means that bridge->destroy() goes away, but that's
>>> probably Ok if everything converts to the standalone driver model
>>> where we have driver->remove()
>>>
>>> Sean
>> Great! Thierry actually mentioned about this once, and we have the
>> confirmation now.
>>
>>>>>
>>>>>       struct drm_mode_object base;
>>>>
>>>>
>>>> Aside: I've noticed all this trying to update the kerneldoc for struct
>>>> drm_bridge, which just showed that this patch makes inconsistent changes.
>>>> Trying to write kerneldoc is a really great way to come up with better
>>>> interfaces imo.
>>>>
>>>> Cheers, Daniel
>> I din't get this actually. You want me to create Doc../drm_bridge.txt
>> or something similar?
>
> If you want to document drm_bridge then I recomment to sprinkle proper
> kerneldoc over drm_bridge.c and pull it all into the drm DocBook
> template. That way all the drm documentation is in one place. I've
> done that for drm_crtc.h in an unrelated patch series (but based upon
> a branch with your patch here included) and there's struct drm_bridge*
> in there. Hence why I've noticed.
Can you send a link for that?
And, is there any problem if the doc comes later?

Ajay

Daniel Vetter Oct. 28, 2014, 2:19 p.m. UTC | #11

On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <[e-mail]> wrote:
> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <[e-mail]> wrote:
>> On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar <[e-mail]> wrote:
>>> Hi Daniel and Sean,
>>>
>>> Thanks for the comments!
>>>
>>> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <[e-mail]> wrote:
>>>> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <[e-mail]> wrote:
>>>>> So don't ask why but I accidentally ended up in a branch looking at this
>>>>> patch and didn't like it. So very quick&grumpy review.
>>>>>
>>>>> First, please make the patch subject more descriptive: I'd expect a helper
>>>>> function scaffolding like the various crtc/probe/dp ... helpers we already
>>>>> have. You instead add code to untangle the probe ordering. Please say so.
>>> Sure. I will reword it properly.
>>>
>>>>> More comments below.
>>>>>
>>>>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
>>>>>> A set of helper functions are defined in this patch to make
>>>>>> bridge driver probe independent of the drm flow.
>>>>>>
>>>>>> The bridge devices register themselves on a lookup table
>>>>>> when they get probed by calling "drm_bridge_add".
>>>>>>
>>>>>> The parent encoder driver waits till the bridge is available
>>>>>> in the lookup table(by calling "of_drm_find_bridge") and then
>>>>>> continues with its initialization.
>>>>>>
>>>>>> The encoder driver should also call "drm_bridge_attach" to pass
>>>>>> on the drm_device, encoder pointers to the bridge object.
>>>>>>
>>>>>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>>>>>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>>>>>> bridge can continue with other initializations.
>>>>>>
>>>>>> Signed-off-by: Ajay Kumar <[e-mail]>
>>>>>
>>>>> [snip]
>>>>>
>>>>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>>>   */
>>>>>>  struct drm_bridge {
>>>>>> -     struct drm_device *dev;
>>>>>> +     struct device *dev;
>>>>>
>>>>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>>>>> drm structures still call it ->dev. Also, can't we use struct device_node
>>>>> here like we do in the of helpers Russell added? See 7e435aad38083
>>>>>
>>>>
>>>> I think this is modeled after the naming in drm_panel,
>>> Right, The entire rework is based on how drm_panel framework is structured.
>>>
>>>> FWIW. However,
>>>> seems reasonable to keep the device_node instead.
>>> Yes, its visible that just device_node would be sufficient.
>>> This will save us from renaming drm_device as well.
>>>
>>>>>> +     struct drm_device *drm;
>>>>>> +     struct drm_encoder *encoder;
>>>>>
>>>>> This breaks bridge->bridge chaining (if we ever get there). It seems
>>>>> pretty much unused anyway except for an EBUSY check. Can't you use
>>>>> bridge->dev for that?
>>>>>
>>>>
>>>> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
>>>> and leave it up to the caller to establish the proper chain.
>>> Ok. I will use drm_device pointer directly instead of passing encoder pointer.
>>
>> Hm, if you do this can you pls also update drm_panel accordingly? It
>> shouldn't be a lot of fuzz and would make things around drm+dt more
>> consistent.
> Are you talking about using struct device_node instead of struct device?
> I guess you have misplaced the comment under the wrong section!

Yeah, that should have been one up ;-)

>>>>>>       struct list_head head;
>>>>>> +     struct list_head list;
>>>>>
>>>>> These lists need better names. I know that the "head" is really awful,
>>>>> especially since it's actually not the head of the list but just an
>>>>> element.
>>>>
>>>> I think we can just rip bridge_list out of mode_config if we're going
>>>> to keep track of bridges elsewhere. So we can nuke "head" and keep
>>>> "list". This also means that bridge->destroy() goes away, but that's
>>>> probably Ok if everything converts to the standalone driver model
>>>> where we have driver->remove()
>>>>
>>>> Sean
>>> Great! Thierry actually mentioned about this once, and we have the
>>> confirmation now.
>>>
>>>>>>
>>>>>>       struct drm_mode_object base;
>>>>>
>>>>>
>>>>> Aside: I've noticed all this trying to update the kerneldoc for struct
>>>>> drm_bridge, which just showed that this patch makes inconsistent changes.
>>>>> Trying to write kerneldoc is a really great way to come up with better
>>>>> interfaces imo.
>>>>>
>>>>> Cheers, Daniel
>>> I din't get this actually. You want me to create Doc../drm_bridge.txt
>>> or something similar?
>>
>> If you want to document drm_bridge then I recomment to sprinkle proper
>> kerneldoc over drm_bridge.c and pull it all into the drm DocBook
>> template. That way all the drm documentation is in one place. I've
>> done that for drm_crtc.h in an unrelated patch series (but based upon
>> a branch with your patch here included) and there's struct drm_bridge*
>> in there. Hence why I've noticed.
> Can you send a link for that?
> And, is there any problem if the doc comes later?

Since quite a while we've asked for the kerneldoc polish as part of
each drm core patch series. It's just that drm_bridge/panel kinda have
flown under the radar of the people usually asking for docs ;-)
-Daniel

Sean Paul Oct. 28, 2014, 2:28 p.m. UTC | #12

On Tue, Oct 28, 2014 at 10:19 AM, Daniel Vetter <[e-mail]> wrote:
> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <[e-mail]> wrote:
>> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <[e-mail]> wrote:
>>> On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar <[e-mail]> wrote:
>>>> Hi Daniel and Sean,
>>>>
>>>> Thanks for the comments!
>>>>
>>>> On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul <[e-mail]> wrote:
>>>>> On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter <[e-mail]> wrote:
>>>>>> So don't ask why but I accidentally ended up in a branch looking at this
>>>>>> patch and didn't like it. So very quick&grumpy review.
>>>>>>
>>>>>> First, please make the patch subject more descriptive: I'd expect a helper
>>>>>> function scaffolding like the various crtc/probe/dp ... helpers we already
>>>>>> have. You instead add code to untangle the probe ordering. Please say so.
>>>> Sure. I will reword it properly.
>>>>
>>>>>> More comments below.
>>>>>>
>>>>>> On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:
>>>>>>> A set of helper functions are defined in this patch to make
>>>>>>> bridge driver probe independent of the drm flow.
>>>>>>>
>>>>>>> The bridge devices register themselves on a lookup table
>>>>>>> when they get probed by calling "drm_bridge_add".
>>>>>>>
>>>>>>> The parent encoder driver waits till the bridge is available
>>>>>>> in the lookup table(by calling "of_drm_find_bridge") and then
>>>>>>> continues with its initialization.
>>>>>>>
>>>>>>> The encoder driver should also call "drm_bridge_attach" to pass
>>>>>>> on the drm_device, encoder pointers to the bridge object.
>>>>>>>
>>>>>>> drm_bridge_attach inturn calls drm_bridge_init to register itself
>>>>>>> with the drm core. Later, it calls "bridge->funcs->attach" so that
>>>>>>> bridge can continue with other initializations.
>>>>>>>
>>>>>>> Signed-off-by: Ajay Kumar <[e-mail]>
>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>>>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>>>>   */
>>>>>>>  struct drm_bridge {
>>>>>>> -     struct drm_device *dev;
>>>>>>> +     struct device *dev;
>>>>>>
>>>>>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>>>>>> drm structures still call it ->dev. Also, can't we use struct device_node
>>>>>> here like we do in the of helpers Russell added? See 7e435aad38083
>>>>>>
>>>>>
>>>>> I think this is modeled after the naming in drm_panel,
>>>> Right, The entire rework is based on how drm_panel framework is structured.
>>>>
>>>>> FWIW. However,
>>>>> seems reasonable to keep the device_node instead.
>>>> Yes, its visible that just device_node would be sufficient.
>>>> This will save us from renaming drm_device as well.
>>>>
>>>>>>> +     struct drm_device *drm;
>>>>>>> +     struct drm_encoder *encoder;
>>>>>>
>>>>>> This breaks bridge->bridge chaining (if we ever get there). It seems
>>>>>> pretty much unused anyway except for an EBUSY check. Can't you use
>>>>>> bridge->dev for that?
>>>>>>
>>>>>
>>>>> Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach
>>>>> and leave it up to the caller to establish the proper chain.
>>>> Ok. I will use drm_device pointer directly instead of passing encoder pointer.
>>>
>>> Hm, if you do this can you pls also update drm_panel accordingly? It
>>> shouldn't be a lot of fuzz and would make things around drm+dt more
>>> consistent.
>> Are you talking about using struct device_node instead of struct device?
>> I guess you have misplaced the comment under the wrong section!
>
> Yeah, that should have been one up ;-)
>
>>>>>>>       struct list_head head;
>>>>>>> +     struct list_head list;
>>>>>>
>>>>>> These lists need better names. I know that the "head" is really awful,
>>>>>> especially since it's actually not the head of the list but just an
>>>>>> element.
>>>>>
>>>>> I think we can just rip bridge_list out of mode_config if we're going
>>>>> to keep track of bridges elsewhere. So we can nuke "head" and keep
>>>>> "list". This also means that bridge->destroy() goes away, but that's
>>>>> probably Ok if everything converts to the standalone driver model
>>>>> where we have driver->remove()
>>>>>
>>>>> Sean
>>>> Great! Thierry actually mentioned about this once, and we have the
>>>> confirmation now.
>>>>
>>>>>>>
>>>>>>>       struct drm_mode_object base;
>>>>>>
>>>>>>
>>>>>> Aside: I've noticed all this trying to update the kerneldoc for struct
>>>>>> drm_bridge, which just showed that this patch makes inconsistent changes.
>>>>>> Trying to write kerneldoc is a really great way to come up with better
>>>>>> interfaces imo.
>>>>>>
>>>>>> Cheers, Daniel
>>>> I din't get this actually. You want me to create Doc../drm_bridge.txt
>>>> or something similar?
>>>
>>> If you want to document drm_bridge then I recomment to sprinkle proper
>>> kerneldoc over drm_bridge.c and pull it all into the drm DocBook
>>> template. That way all the drm documentation is in one place. I've
>>> done that for drm_crtc.h in an unrelated patch series (but based upon
>>> a branch with your patch here included) and there's struct drm_bridge*
>>> in there. Hence why I've noticed.
>> Can you send a link for that?
>> And, is there any problem if the doc comes later?
>
> Since quite a while we've asked for the kerneldoc polish as part of
> each drm core patch series. It's just that drm_bridge/panel kinda have
> flown under the radar of the people usually asking for docs ;-)

Heh, sorry about that.

Sean



> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch

Thierry Reding Oct. 28, 2014, 2:29 p.m. UTC | #13

On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
> >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> >>>   * @driver_private: pointer to the bridge driver's internal context
> >>>   */
> >>>  struct drm_bridge {
> >>> -     struct drm_device *dev;
> >>> +     struct device *dev;
> >>
> >> Please don't rename the ->dev pointer into drm. Because _all_ the other
> >> drm structures still call it ->dev. Also, can't we use struct device_node
> >> here like we do in the of helpers Russell added? See 7e435aad38083
> >>
> >
> > I think this is modeled after the naming in drm_panel, FWIW. However,
> > seems reasonable to keep the device_node instead.
> 
> Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> drm_crtc drop the struct device and go directly to a struct
> device_node. Since we don't really need the sturct device, the only
> thing we care about is the of_node. For added bonus wrap an #ifdef
> CONFIG_OF around all the various struct device_node in drm_foo.h.
> Should be all fairly simple to pull off with cocci.
> 
> Thierry?

The struct device * is in DRM panel because there's nothing device tree
specific about the concept. Having a struct device_node * instead would
indicate that it can only be used with a device tree, whereas the
framework doesn't care the tiniest bit what type of device we have.

While the trend clearly is to use more device tree, I don't think we
should make it impossible for anybody else to use these frameworks.

There are other advantages to keeping a struct device *, like having
access to the proper device and its name. Also you get access to the
device_node * via dev->of_node anyway. I don't see any advantage in
switching to just a struct device_node *, only disadvantages.

Thierry

Thierry Reding Oct. 28, 2014, 2:35 p.m. UTC | #14

On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <[e-mail]> wrote:
> > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
> >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> >>>>   * @driver_private: pointer to the bridge driver's internal context
> >>>>   */
> >>>>  struct drm_bridge {
> >>>> -     struct drm_device *dev;
> >>>> +     struct device *dev;
> >>>
> >>> Please don't rename the ->dev pointer into drm. Because _all_ the other
> >>> drm structures still call it ->dev. Also, can't we use struct device_node
> >>> here like we do in the of helpers Russell added? See 7e435aad38083
> >>>
> >>
> >> I think this is modeled after the naming in drm_panel, FWIW. However,
> >> seems reasonable to keep the device_node instead.
> >
> > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > drm_crtc drop the struct device and go directly to a struct
> > device_node. Since we don't really need the sturct device, the only
> > thing we care about is the of_node. For added bonus wrap an #ifdef
> > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > Should be all fairly simple to pull off with cocci.
> >
> > Thierry?
> 
> Looking at the of_drm_find_panel function I actually wonder how that
> works - the drm_panel doesn't really need to stick around afaics.
> After all panel_list is global so some other driver can unload.
> Russell's of support for possible_crtcs code works differently since
> it only looks at per-drm_device lists.

I don't understand. Panels are global resources that get registered by
some driver that isn't tied to the DRM device until attached. It can't
be in a per-DRM device list, because it's external to the device.

And yes, they can go away when a driver is unloaded, though it's not the
typical use-case.

> This bridge code here though suffers from the same. So to me this
> looks rather fishy.

Well, this version of the DRM bridge support was written to be close to
DRM panel, so it isn't really surprising that it's similar =), but like
I said, I don't really understand what you think is wrong with it.

> It doesn't help that we have drm_of.[hc] around but not all the of
> code is in there. Adding Russell too.

DRM panel and DRM bridge aren't just OF helpers. They can be used with
whatever type of device you want.

Thierry

Thierry Reding Oct. 28, 2014, 2:41 p.m. UTC | #15

On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <[e-mail]> wrote:
> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <[e-mail]> wrote:
[...]
> >> Hm, if you do this can you pls also update drm_panel accordingly? It
> >> shouldn't be a lot of fuzz and would make things around drm+dt more
> >> consistent.
> > Are you talking about using struct device_node instead of struct device?
> > I guess you have misplaced the comment under the wrong section!
> 
> Yeah, that should have been one up ;-)

Like I said earlier, I don't think dropping struct device * in favour of
struct device_node * is a good idea.

> >> If you want to document drm_bridge then I recomment to sprinkle proper
> >> kerneldoc over drm_bridge.c and pull it all into the drm DocBook
> >> template. That way all the drm documentation is in one place. I've
> >> done that for drm_crtc.h in an unrelated patch series (but based upon
> >> a branch with your patch here included) and there's struct drm_bridge*
> >> in there. Hence why I've noticed.
> > Can you send a link for that?
> > And, is there any problem if the doc comes later?
> 
> Since quite a while we've asked for the kerneldoc polish as part of
> each drm core patch series. It's just that drm_bridge/panel kinda have
> flown under the radar of the people usually asking for docs ;-)

FWIW, there's some kerneldoc in include/drm/drm_panel.h but I guess I
could write up something more complete and integrate it into DocBook.

Thierry

Ajay kumar Oct. 28, 2014, 2:46 p.m. UTC | #16

On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
<[e-mail]> wrote:
> On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
>> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <[e-mail]> wrote:
>> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <[e-mail]> wrote:
> [...]
>> >> Hm, if you do this can you pls also update drm_panel accordingly? It
>> >> shouldn't be a lot of fuzz and would make things around drm+dt more
>> >> consistent.
>> > Are you talking about using struct device_node instead of struct device?
>> > I guess you have misplaced the comment under the wrong section!
>>
>> Yeah, that should have been one up ;-)
>
> Like I said earlier, I don't think dropping struct device * in favour of
> struct device_node * is a good idea.
I am not sure about drm_panel.
But, I am not really doing anything with the struct device pointer in
case of bridge.
So, just wondering if it is really needed?

Ajay

Thierry Reding Oct. 28, 2014, 3:05 p.m. UTC | #17

On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote:
> On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
> <[e-mail]> wrote:
> > On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
> >> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <[e-mail]> wrote:
> >> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <[e-mail]> wrote:
> > [...]
> >> >> Hm, if you do this can you pls also update drm_panel accordingly? It
> >> >> shouldn't be a lot of fuzz and would make things around drm+dt more
> >> >> consistent.
> >> > Are you talking about using struct device_node instead of struct device?
> >> > I guess you have misplaced the comment under the wrong section!
> >>
> >> Yeah, that should have been one up ;-)
> >
> > Like I said earlier, I don't think dropping struct device * in favour of
> > struct device_node * is a good idea.
> I am not sure about drm_panel.
> But, I am not really doing anything with the struct device pointer in
> case of bridge.
> So, just wondering if it is really needed?

I think it's useful to have it just to send the right message. DRM panel
and DRM bridge aren't specific to device tree. They are completely
generic and can work with any type of device, whether it was
instantiated from the device tree or some other infrastructure. Dropping
struct device * will make it effectively useless on anything but DT. I
don't think we should strive for that, even if only DT-enabled platforms
currently use them.

Thierry

Daniel Vetter Oct. 29, 2014, 7:43 a.m. UTC | #18

On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
> On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <[e-mail]> wrote:
> > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
> > >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > >>>>   * @driver_private: pointer to the bridge driver's internal context
> > >>>>   */
> > >>>>  struct drm_bridge {
> > >>>> -     struct drm_device *dev;
> > >>>> +     struct device *dev;
> > >>>
> > >>> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > >>> drm structures still call it ->dev. Also, can't we use struct device_node
> > >>> here like we do in the of helpers Russell added? See 7e435aad38083
> > >>>
> > >>
> > >> I think this is modeled after the naming in drm_panel, FWIW. However,
> > >> seems reasonable to keep the device_node instead.
> > >
> > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > drm_crtc drop the struct device and go directly to a struct
> > > device_node. Since we don't really need the sturct device, the only
> > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > Should be all fairly simple to pull off with cocci.
> > >
> > > Thierry?
> > 
> > Looking at the of_drm_find_panel function I actually wonder how that
> > works - the drm_panel doesn't really need to stick around afaics.
> > After all panel_list is global so some other driver can unload.
> > Russell's of support for possible_crtcs code works differently since
> > it only looks at per-drm_device lists.
> 
> I don't understand. Panels are global resources that get registered by
> some driver that isn't tied to the DRM device until attached. It can't
> be in a per-DRM device list, because it's external to the device.
> 
> And yes, they can go away when a driver is unloaded, though it's not the
> typical use-case.
> 
> > This bridge code here though suffers from the same. So to me this
> > looks rather fishy.
> 
> Well, this version of the DRM bridge support was written to be close to
> DRM panel, so it isn't really surprising that it's similar =), but like
> I said, I don't really understand what you think is wrong with it.

You have a mutex to protect the global list of bridges/panels. But if you
do a lookup you don't grab a reference count on the panel, so the moment
you drop the list mutex the panel/bridge can go away.

Yes usually you don't unload a driver on a soc but soc isn't everything
and designing new stuff to not be hotunplug save isn't great either.
-Daniel

Daniel Vetter Oct. 29, 2014, 7:51 a.m. UTC | #19

On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
> > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > >>>   * @driver_private: pointer to the bridge driver's internal context
> > >>>   */
> > >>>  struct drm_bridge {
> > >>> -     struct drm_device *dev;
> > >>> +     struct device *dev;
> > >>
> > >> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > >> drm structures still call it ->dev. Also, can't we use struct device_node
> > >> here like we do in the of helpers Russell added? See 7e435aad38083
> > >>
> > >
> > > I think this is modeled after the naming in drm_panel, FWIW. However,
> > > seems reasonable to keep the device_node instead.
> > 
> > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > drm_crtc drop the struct device and go directly to a struct
> > device_node. Since we don't really need the sturct device, the only
> > thing we care about is the of_node. For added bonus wrap an #ifdef
> > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > Should be all fairly simple to pull off with cocci.
> > 
> > Thierry?
> 
> The struct device * is in DRM panel because there's nothing device tree
> specific about the concept. Having a struct device_node * instead would
> indicate that it can only be used with a device tree, whereas the
> framework doesn't care the tiniest bit what type of device we have.
> 
> While the trend clearly is to use more device tree, I don't think we
> should make it impossible for anybody else to use these frameworks.
> 
> There are other advantages to keeping a struct device *, like having
> access to the proper device and its name. Also you get access to the
> device_node * via dev->of_node anyway. I don't see any advantage in
> switching to just a struct device_node *, only disadvantages.

Well the idea is to make the lookup key specific, and conditional on
#CONFIG_OF. If there's going to be another neat way to enumerate platform
devices then I think we should add that, too. Or maybe we should have a
void *platform_data or so.

The reason I really don't want a struct device * in core drm structures is
that two releases down the road people will have found tons of really
great ways to abuse them and re-create a midlayer. DRM core really should
only care about the sw objects and not be hw specific at all. Heck there's
not even an requirement to have any piece of actual hw, you could write a
completely fake drm driver (for e.g. testing like the new v4l driver).

Tbh I wonder a bit why we even have this registery embedded into the core
drm objects. Essentially the only thing you're doing is a list that maps
some platform specific key onto some subsystem specific driver structure
or fails the lookup. So instead of putting all these low-level details
into drm core structures can't we just have a generic hashtable/list for
this, plus some static inline helpers that cast the void * you get into
the one you want?

I also get the feeling that this really should be in the driver core (like
the component helpers), and that we should think a lot harder about
lifetimes and refcounting (see my other reply on that).
-Daniel

Daniel Vetter Oct. 29, 2014, 7:58 a.m. UTC | #20

On Tue, Oct 28, 2014 at 04:05:34PM +0100, Thierry Reding wrote:
> On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote:
> > On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
> > <[e-mail]> wrote:
> > > On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
> > >> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <[e-mail]> wrote:
> > >> > On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <[e-mail]> wrote:
> > > [...]
> > >> >> Hm, if you do this can you pls also update drm_panel accordingly? It
> > >> >> shouldn't be a lot of fuzz and would make things around drm+dt more
> > >> >> consistent.
> > >> > Are you talking about using struct device_node instead of struct device?
> > >> > I guess you have misplaced the comment under the wrong section!
> > >>
> > >> Yeah, that should have been one up ;-)
> > >
> > > Like I said earlier, I don't think dropping struct device * in favour of
> > > struct device_node * is a good idea.
> > I am not sure about drm_panel.
> > But, I am not really doing anything with the struct device pointer in
> > case of bridge.
> > So, just wondering if it is really needed?
> 
> I think it's useful to have it just to send the right message. DRM panel
> and DRM bridge aren't specific to device tree. They are completely
> generic and can work with any type of device, whether it was
> instantiated from the device tree or some other infrastructure. Dropping
> struct device * will make it effectively useless on anything but DT. I
> don't think we should strive for that, even if only DT-enabled platforms
> currently use them.

See my other reply, but I now think we should put neither into drm
structures. This "find me the driver structures for this device" problem
looks really generic, and it has nothing to do with the drm structures and
concepts like bridges/panels at all. It shouldn't be in there at all.

Adding it looks very much like reintroducing the drm midlayer that we just
finally made obsolete, just not at the top-level (struct drm_device) but
at a bunch of leaf nodes. I expect all the same issues though. And I'm
definitely not looking to de-midlayer more stuff that we're just adding.

Imo this should be solved as a separate helper thing, maybe in the driver
core akin to the component helpers from Russell.
-Daniel

Thierry Reding Oct. 29, 2014, 8:38 a.m. UTC | #21

On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
> > On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> > > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <[e-mail]> wrote:
> > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
> > > >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > >>>>   * @driver_private: pointer to the bridge driver's internal context
> > > >>>>   */
> > > >>>>  struct drm_bridge {
> > > >>>> -     struct drm_device *dev;
> > > >>>> +     struct device *dev;
> > > >>>
> > > >>> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > > >>> drm structures still call it ->dev. Also, can't we use struct device_node
> > > >>> here like we do in the of helpers Russell added? See 7e435aad38083
> > > >>>
> > > >>
> > > >> I think this is modeled after the naming in drm_panel, FWIW. However,
> > > >> seems reasonable to keep the device_node instead.
> > > >
> > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > drm_crtc drop the struct device and go directly to a struct
> > > > device_node. Since we don't really need the sturct device, the only
> > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > Should be all fairly simple to pull off with cocci.
> > > >
> > > > Thierry?
> > > 
> > > Looking at the of_drm_find_panel function I actually wonder how that
> > > works - the drm_panel doesn't really need to stick around afaics.
> > > After all panel_list is global so some other driver can unload.
> > > Russell's of support for possible_crtcs code works differently since
> > > it only looks at per-drm_device lists.
> > 
> > I don't understand. Panels are global resources that get registered by
> > some driver that isn't tied to the DRM device until attached. It can't
> > be in a per-DRM device list, because it's external to the device.
> > 
> > And yes, they can go away when a driver is unloaded, though it's not the
> > typical use-case.
> > 
> > > This bridge code here though suffers from the same. So to me this
> > > looks rather fishy.
> > 
> > Well, this version of the DRM bridge support was written to be close to
> > DRM panel, so it isn't really surprising that it's similar =), but like
> > I said, I don't really understand what you think is wrong with it.
> 
> You have a mutex to protect the global list of bridges/panels. But if you
> do a lookup you don't grab a reference count on the panel, so the moment
> you drop the list mutex the panel/bridge can go away.
> 
> Yes usually you don't unload a driver on a soc but soc isn't everything
> and designing new stuff to not be hotunplug save isn't great either.

Yeah, I certainly agree that adding proper reference counting would be a
good thing. I think perhaps we could just take a reference on the struct
device * to prevent it from disappearing.

Although perhaps I misunderstand what you mean by "go away".

Thierry

Daniel Vetter Oct. 29, 2014, 8:57 a.m. UTC | #22

On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote:
> On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
> > On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
> > > On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> > > > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <[e-mail]> wrote:
> > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
> > > > >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > > >>>>   * @driver_private: pointer to the bridge driver's internal context
> > > > >>>>   */
> > > > >>>>  struct drm_bridge {
> > > > >>>> -     struct drm_device *dev;
> > > > >>>> +     struct device *dev;
> > > > >>>
> > > > >>> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > > > >>> drm structures still call it ->dev. Also, can't we use struct device_node
> > > > >>> here like we do in the of helpers Russell added? See 7e435aad38083
> > > > >>>
> > > > >>
> > > > >> I think this is modeled after the naming in drm_panel, FWIW. However,
> > > > >> seems reasonable to keep the device_node instead.
> > > > >
> > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > > drm_crtc drop the struct device and go directly to a struct
> > > > > device_node. Since we don't really need the sturct device, the only
> > > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > > Should be all fairly simple to pull off with cocci.
> > > > >
> > > > > Thierry?
> > > > 
> > > > Looking at the of_drm_find_panel function I actually wonder how that
> > > > works - the drm_panel doesn't really need to stick around afaics.
> > > > After all panel_list is global so some other driver can unload.
> > > > Russell's of support for possible_crtcs code works differently since
> > > > it only looks at per-drm_device lists.
> > > 
> > > I don't understand. Panels are global resources that get registered by
> > > some driver that isn't tied to the DRM device until attached. It can't
> > > be in a per-DRM device list, because it's external to the device.
> > > 
> > > And yes, they can go away when a driver is unloaded, though it's not the
> > > typical use-case.
> > > 
> > > > This bridge code here though suffers from the same. So to me this
> > > > looks rather fishy.
> > > 
> > > Well, this version of the DRM bridge support was written to be close to
> > > DRM panel, so it isn't really surprising that it's similar =), but like
> > > I said, I don't really understand what you think is wrong with it.
> > 
> > You have a mutex to protect the global list of bridges/panels. But if you
> > do a lookup you don't grab a reference count on the panel, so the moment
> > you drop the list mutex the panel/bridge can go away.
> > 
> > Yes usually you don't unload a driver on a soc but soc isn't everything
> > and designing new stuff to not be hotunplug save isn't great either.
> 
> Yeah, I certainly agree that adding proper reference counting would be a
> good thing. I think perhaps we could just take a reference on the struct
> device * to prevent it from disappearing.
> 
> Although perhaps I misunderstand what you mean by "go away".

I meant the drm_panel/bridge could go away any second, since nothing
prevents a module unload of the panel/bridge driver. So in theory you
could get the unregister call right after you've done the lookup. Which
means the bridge/panel pointer you've just returned is freed memory.

I think we nee try_get_module for the code and kref on the actual data
structures. That makes the entire thing a bit non-trivial, which is why I
think it would be better as some generic helper. Which then gets embedded
or instantiated for specific cases, like dt&drm_panel or dt&drm_bridge. Or
maybe even acpi&drm_bridge, who knows ;-)
-Daniel

Andrzej Hajda Oct. 29, 2014, 9:09 a.m. UTC | #23

On 10/29/2014 08:58 AM, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 04:05:34PM +0100, Thierry Reding wrote:
>> On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote:
>>> On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
>>> <[e-mail]> wrote:
>>>> On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
>>>>> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <[e-mail]> wrote:
>>>>>> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <[e-mail]> wrote:
>>>> [...]
>>>>>>> Hm, if you do this can you pls also update drm_panel accordingly? It
>>>>>>> shouldn't be a lot of fuzz and would make things around drm+dt more
>>>>>>> consistent.
>>>>>> Are you talking about using struct device_node instead of struct device?
>>>>>> I guess you have misplaced the comment under the wrong section!
>>>>>
>>>>> Yeah, that should have been one up ;-)
>>>>
>>>> Like I said earlier, I don't think dropping struct device * in favour of
>>>> struct device_node * is a good idea.
>>> I am not sure about drm_panel.
>>> But, I am not really doing anything with the struct device pointer in
>>> case of bridge.
>>> So, just wondering if it is really needed?
>>
>> I think it's useful to have it just to send the right message. DRM panel
>> and DRM bridge aren't specific to device tree. They are completely
>> generic and can work with any type of device, whether it was
>> instantiated from the device tree or some other infrastructure. Dropping
>> struct device * will make it effectively useless on anything but DT. I
>> don't think we should strive for that, even if only DT-enabled platforms
>> currently use them.
> 
> See my other reply, but I now think we should put neither into drm
> structures. This "find me the driver structures for this device" problem
> looks really generic, and it has nothing to do with the drm structures and
> concepts like bridges/panels at all. It shouldn't be in there at all.
> 
> Adding it looks very much like reintroducing the drm midlayer that we just
> finally made obsolete, just not at the top-level (struct drm_device) but
> at a bunch of leaf nodes. I expect all the same issues though. And I'm
> definitely not looking to de-midlayer more stuff that we're just adding.
> 
> Imo this should be solved as a separate helper thing, maybe in the driver
> core akin to the component helpers from Russell.
> -Daniel
> 

As I understand you want something generic to look for panels, bridges,
whatever and, like components, it should allow to safe hot plug/unplug.
I have proposed such thing few months ago [1]. Have you looked at it?

[1]: https://lkml.org/lkml/2014/4/30/345

Regards
Andrzej

Thierry Reding Oct. 29, 2014, 9:14 a.m. UTC | #24

On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote:
> > On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
> > > On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
> > > > On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
> > > > > On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <[e-mail]> wrote:
> > > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
> > > > > >>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > > > >>>>   * @driver_private: pointer to the bridge driver's internal context
> > > > > >>>>   */
> > > > > >>>>  struct drm_bridge {
> > > > > >>>> -     struct drm_device *dev;
> > > > > >>>> +     struct device *dev;
> > > > > >>>
> > > > > >>> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > > > > >>> drm structures still call it ->dev. Also, can't we use struct device_node
> > > > > >>> here like we do in the of helpers Russell added? See 7e435aad38083
> > > > > >>>
> > > > > >>
> > > > > >> I think this is modeled after the naming in drm_panel, FWIW. However,
> > > > > >> seems reasonable to keep the device_node instead.
> > > > > >
> > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > > > drm_crtc drop the struct device and go directly to a struct
> > > > > > device_node. Since we don't really need the sturct device, the only
> > > > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > > > Should be all fairly simple to pull off with cocci.
> > > > > >
> > > > > > Thierry?
> > > > > 
> > > > > Looking at the of_drm_find_panel function I actually wonder how that
> > > > > works - the drm_panel doesn't really need to stick around afaics.
> > > > > After all panel_list is global so some other driver can unload.
> > > > > Russell's of support for possible_crtcs code works differently since
> > > > > it only looks at per-drm_device lists.
> > > > 
> > > > I don't understand. Panels are global resources that get registered by
> > > > some driver that isn't tied to the DRM device until attached. It can't
> > > > be in a per-DRM device list, because it's external to the device.
> > > > 
> > > > And yes, they can go away when a driver is unloaded, though it's not the
> > > > typical use-case.
> > > > 
> > > > > This bridge code here though suffers from the same. So to me this
> > > > > looks rather fishy.
> > > > 
> > > > Well, this version of the DRM bridge support was written to be close to
> > > > DRM panel, so it isn't really surprising that it's similar =), but like
> > > > I said, I don't really understand what you think is wrong with it.
> > > 
> > > You have a mutex to protect the global list of bridges/panels. But if you
> > > do a lookup you don't grab a reference count on the panel, so the moment
> > > you drop the list mutex the panel/bridge can go away.
> > > 
> > > Yes usually you don't unload a driver on a soc but soc isn't everything
> > > and designing new stuff to not be hotunplug save isn't great either.
> > 
> > Yeah, I certainly agree that adding proper reference counting would be a
> > good thing. I think perhaps we could just take a reference on the struct
> > device * to prevent it from disappearing.
> > 
> > Although perhaps I misunderstand what you mean by "go away".
> 
> I meant the drm_panel/bridge could go away any second, since nothing
> prevents a module unload of the panel/bridge driver. So in theory you
> could get the unregister call right after you've done the lookup. Which
> means the bridge/panel pointer you've just returned is freed memory.

Ah yes, I see now.

> I think we nee try_get_module for the code and kref on the actual data
> structures.

Agreed, that should do the trick. We'd probably need some sort of logic
to also make operations return something like -ENODEV when the
underlying device has disappeared. I think David had introduced
something similar for DRM device not so long ago?

> That makes the entire thing a bit non-trivial, which is why I think it
> would be better as some generic helper. Which then gets embedded or
> instantiated for specific cases, like dt&drm_panel or dt&drm_bridge.
> Or maybe even acpi&drm_bridge, who knows ;-)

I worry a little about type safety. How will this generic helper know
what registry to look in for? Or conversely, if all these resources are
added to a single registry how do you know that they're of the correct
type? Failing to ensure this could cause situations where you're asking
for a panel and get a bridge in return because you've wrongly wired it
up in device tree for example.

But perhaps if both the registry and the device parts are turned into
helpers we could still have a single core implementation and then
instantiate that for each type, something roughly like this:

	struct registry {
		struct list_head list;
		struct mutex lock;
	};

	struct registry_record {
		struct list_head list;
		struct module *owner;
		struct kref *ref;

		struct device *dev;
	};

	int registry_add(struct registry *registry, struct registry_record *record)
	{
		...
		try_module_get(record->owner);
		...
	}

	struct registry_record *registry_find_by_of_node(struct registry *registry,
							 struct device_node *np)
	{
		...
		kref_get(...);
		...
	}

That way it should be possible to embed these into other structures,
like so:

	struct drm_panel {
		struct registry_record record;
		...
	};

	static struct registry drm_panels;

	int drm_panel_add(struct drm_panel *panel)
	{
		return registry_add(&drm_panels, &panel->record);
	}

	struct drm_panel *of_drm_panel_find(struct device_node *np)
	{
		struct registry_record *record;

		record = registry_find_by_of_node(&drm_panels, np);

		return container_of(record, struct drm_panel, record);
	}

Is that what you had in mind?

Thierry

Thierry Reding Oct. 29, 2014, 9:16 a.m. UTC | #25

On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
> > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > >>>   * @driver_private: pointer to the bridge driver's internal context
> > > >>>   */
> > > >>>  struct drm_bridge {
> > > >>> -     struct drm_device *dev;
> > > >>> +     struct device *dev;
> > > >>
> > > >> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > > >> drm structures still call it ->dev. Also, can't we use struct device_node
> > > >> here like we do in the of helpers Russell added? See 7e435aad38083
> > > >>
> > > >
> > > > I think this is modeled after the naming in drm_panel, FWIW. However,
> > > > seems reasonable to keep the device_node instead.
> > > 
> > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > drm_crtc drop the struct device and go directly to a struct
> > > device_node. Since we don't really need the sturct device, the only
> > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > Should be all fairly simple to pull off with cocci.
> > > 
> > > Thierry?
> > 
> > The struct device * is in DRM panel because there's nothing device tree
> > specific about the concept. Having a struct device_node * instead would
> > indicate that it can only be used with a device tree, whereas the
> > framework doesn't care the tiniest bit what type of device we have.
> > 
> > While the trend clearly is to use more device tree, I don't think we
> > should make it impossible for anybody else to use these frameworks.
> > 
> > There are other advantages to keeping a struct device *, like having
> > access to the proper device and its name. Also you get access to the
> > device_node * via dev->of_node anyway. I don't see any advantage in
> > switching to just a struct device_node *, only disadvantages.
> 
> Well the idea is to make the lookup key specific, and conditional on
> #CONFIG_OF. If there's going to be another neat way to enumerate platform
> devices then I think we should add that, too. Or maybe we should have a
> void *platform_data or so.
> 
> The reason I really don't want a struct device * in core drm structures is
> that two releases down the road people will have found tons of really
> great ways to abuse them and re-create a midlayer. DRM core really should
> only care about the sw objects and not be hw specific at all. Heck there's
> not even an requirement to have any piece of actual hw, you could write a
> completely fake drm driver (for e.g. testing like the new v4l driver).
> 
> Tbh I wonder a bit why we even have this registery embedded into the core
> drm objects. Essentially the only thing you're doing is a list that maps
> some platform specific key onto some subsystem specific driver structure
> or fails the lookup. So instead of putting all these low-level details
> into drm core structures can't we just have a generic hashtable/list for
> this, plus some static inline helpers that cast the void * you get into
> the one you want?
> 
> I also get the feeling that this really should be in the driver core (like
> the component helpers), and that we should think a lot harder about
> lifetimes and refcounting (see my other reply on that).

Yes, that sounds very useful indeed. Also see my reply to yours. =)

Thierry

Andrzej Hajda Oct. 30, 2014, 10:01 a.m. UTC | #26

On 10/29/2014 10:14 AM, Thierry Reding wrote:
> On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
>> On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote:
>>> On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:
>>>> On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:
>>>>> On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
>>>>>> On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter <[e-mail]> wrote:
>>>>>>> On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
>>>>>>>>>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
>>>>>>>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>>>>>>>   */
>>>>>>>>>>  struct drm_bridge {
>>>>>>>>>> -     struct drm_device *dev;
>>>>>>>>>> +     struct device *dev;
>>>>>>>>>
>>>>>>>>> Please don't rename the ->dev pointer into drm. Because _all_ the other
>>>>>>>>> drm structures still call it ->dev. Also, can't we use struct device_node
>>>>>>>>> here like we do in the of helpers Russell added? See 7e435aad38083
>>>>>>>>>
>>>>>>>>
>>>>>>>> I think this is modeled after the naming in drm_panel, FWIW. However,
>>>>>>>> seems reasonable to keep the device_node instead.
>>>>>>>
>>>>>>> Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
>>>>>>> drm_crtc drop the struct device and go directly to a struct
>>>>>>> device_node. Since we don't really need the sturct device, the only
>>>>>>> thing we care about is the of_node. For added bonus wrap an #ifdef
>>>>>>> CONFIG_OF around all the various struct device_node in drm_foo.h.
>>>>>>> Should be all fairly simple to pull off with cocci.
>>>>>>>
>>>>>>> Thierry?
>>>>>>
>>>>>> Looking at the of_drm_find_panel function I actually wonder how that
>>>>>> works - the drm_panel doesn't really need to stick around afaics.
>>>>>> After all panel_list is global so some other driver can unload.
>>>>>> Russell's of support for possible_crtcs code works differently since
>>>>>> it only looks at per-drm_device lists.
>>>>>
>>>>> I don't understand. Panels are global resources that get registered by
>>>>> some driver that isn't tied to the DRM device until attached. It can't
>>>>> be in a per-DRM device list, because it's external to the device.
>>>>>
>>>>> And yes, they can go away when a driver is unloaded, though it's not the
>>>>> typical use-case.
>>>>>
>>>>>> This bridge code here though suffers from the same. So to me this
>>>>>> looks rather fishy.
>>>>>
>>>>> Well, this version of the DRM bridge support was written to be close to
>>>>> DRM panel, so it isn't really surprising that it's similar =), but like
>>>>> I said, I don't really understand what you think is wrong with it.
>>>>
>>>> You have a mutex to protect the global list of bridges/panels. But if you
>>>> do a lookup you don't grab a reference count on the panel, so the moment
>>>> you drop the list mutex the panel/bridge can go away.
>>>>
>>>> Yes usually you don't unload a driver on a soc but soc isn't everything
>>>> and designing new stuff to not be hotunplug save isn't great either.
>>>
>>> Yeah, I certainly agree that adding proper reference counting would be a
>>> good thing. I think perhaps we could just take a reference on the struct
>>> device * to prevent it from disappearing.
>>>
>>> Although perhaps I misunderstand what you mean by "go away".
>>
>> I meant the drm_panel/bridge could go away any second, since nothing
>> prevents a module unload of the panel/bridge driver. So in theory you
>> could get the unregister call right after you've done the lookup. Which
>> means the bridge/panel pointer you've just returned is freed memory.
> 
> Ah yes, I see now.
> 
>> I think we nee try_get_module for the code and kref on the actual data
>> structures.
> 
> Agreed, that should do the trick. We'd probably need some sort of logic
> to also make operations return something like -ENODEV when the
> underlying device has disappeared. I think David had introduced
> something similar for DRM device not so long ago?

If the underlying device disappears it would be good to receive
notification anyway to trigger DRM HPD event. And if we have the
notification, we can release references to the device smoothly. We do
not need to play tricky games with krefs, zombie data and module
refcounting.
On the other side component framework uses notification callbacks
bind/unbind for master and components to smoothly attach/release
devices, why should it be done differently in this case.

Again, look at interface_tracker [1] it does exactly what you need.

[1]: https://lkml.org/lkml/2014/4/30/345

Regards
Andrzej

Russell King - ARM Linux Oct. 30, 2014, 10:09 a.m. UTC | #27

On Thu, Oct 30, 2014 at 11:01:02AM +0100, Andrzej Hajda wrote:
> On 10/29/2014 10:14 AM, Thierry Reding wrote:
> > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> >> I think we nee try_get_module for the code and kref on the actual data
> >> structures.
> > 
> > Agreed, that should do the trick. We'd probably need some sort of logic
> > to also make operations return something like -ENODEV when the
> > underlying device has disappeared. I think David had introduced
> > something similar for DRM device not so long ago?
> 
> If the underlying device disappears it would be good to receive
> notification anyway to trigger DRM HPD event. And if we have the
> notification, we can release references to the device smoothly. We do
> not need to play tricky games with krefs, zombie data and module
> refcounting.

Any solution which thinks it needs to lock modules into the core is
fundamentally broken.  It totally misses the point.

While you can lock a module into the core using try_get_module(), that
doesn't stop the device itself being unbound from a driver.  Soo many
people have fallen into that trap.  They write their device driver,
along with some kind of framework which they make use try_get_module().
They think its safe.  When you then echo the device name into the
driver's unbind sysfs file, all hell breaks loose and the kernel oopses.

try_get_module is /totally/ useless for ensuring that things stick around.

The reality is that you can't make devices stick around.  Once that
remove callback from the driver layer is called, that's it, the device
_is_ going away whether you like it or not.  You can't stop it.  It's
no good returning -EBUSY, because the remove return code is ignored.

What's more scarey is when you consider that in a real hotplug
situation, when the remove callback is called, the device has
/already/ gone.

So please, stop thinking that try_get_module() has some magic solution.
Any "solution" to device lifetimes using try_get_module() totally misses
the problem, and is just mere obfuscation and actually a bug in itself.

Daniel Vetter Oct. 31, 2014, 3:51 p.m. UTC | #28

On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote:
> On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> > That makes the entire thing a bit non-trivial, which is why I think it
> > would be better as some generic helper. Which then gets embedded or
> > instantiated for specific cases, like dt&drm_panel or dt&drm_bridge.
> > Or maybe even acpi&drm_bridge, who knows ;-)
> 
> I worry a little about type safety. How will this generic helper know
> what registry to look in for? Or conversely, if all these resources are
> added to a single registry how do you know that they're of the correct
> type? Failing to ensure this could cause situations where you're asking
> for a panel and get a bridge in return because you've wrongly wired it
> up in device tree for example.
> 
> But perhaps if both the registry and the device parts are turned into
> helpers we could still have a single core implementation and then
> instantiate that for each type, something roughly like this:
> 
> 	struct registry {
> 		struct list_head list;
> 		struct mutex lock;
> 	};
> 
> 	struct registry_record {
> 		struct list_head list;
> 		struct module *owner;
> 		struct kref *ref;
> 
> 		struct device *dev;
> 	};
> 
> 	int registry_add(struct registry *registry, struct registry_record *record)
> 	{
> 		...
> 		try_module_get(record->owner);
> 		...
> 	}
> 
> 	struct registry_record *registry_find_by_of_node(struct registry *registry,
> 							 struct device_node *np)
> 	{
> 		...
> 		kref_get(...);
> 		...
> 	}
> 
> That way it should be possible to embed these into other structures,
> like so:
> 
> 	struct drm_panel {
> 		struct registry_record record;
> 		...
> 	};
> 
> 	static struct registry drm_panels;
> 
> 	int drm_panel_add(struct drm_panel *panel)
> 	{
> 		return registry_add(&drm_panels, &panel->record);
> 	}
> 
> 	struct drm_panel *of_drm_panel_find(struct device_node *np)
> 	{
> 		struct registry_record *record;
> 
> 		record = registry_find_by_of_node(&drm_panels, np);
> 
> 		return container_of(record, struct drm_panel, record);
> 	}
> 
> Is that what you had in mind?

Yeah I've thought that we should instantiate using macros even, so that we
have per-type registries. So you'd smash the usual set of
DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take
a (name, key, value) tripled. For the example here(of_drm_panel, struct
device_node *, struct drm_panel *) or similar. I might be hand-waving over
a few too many details though ;-)

Cheers, Daniel

Daniel Vetter Oct. 31, 2014, 3:54 p.m. UTC | #29

On Thu, Oct 30, 2014 at 10:09:28AM +0000, Russell King - ARM Linux wrote:
> On Thu, Oct 30, 2014 at 11:01:02AM +0100, Andrzej Hajda wrote:
> > On 10/29/2014 10:14 AM, Thierry Reding wrote:
> > > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> > >> I think we nee try_get_module for the code and kref on the actual data
> > >> structures.
> > > 
> > > Agreed, that should do the trick. We'd probably need some sort of logic
> > > to also make operations return something like -ENODEV when the
> > > underlying device has disappeared. I think David had introduced
> > > something similar for DRM device not so long ago?
> > 
> > If the underlying device disappears it would be good to receive
> > notification anyway to trigger DRM HPD event. And if we have the
> > notification, we can release references to the device smoothly. We do
> > not need to play tricky games with krefs, zombie data and module
> > refcounting.
> 
> Any solution which thinks it needs to lock modules into the core is
> fundamentally broken.  It totally misses the point.
> 
> While you can lock a module into the core using try_get_module(), that
> doesn't stop the device itself being unbound from a driver.  Soo many
> people have fallen into that trap.  They write their device driver,
> along with some kind of framework which they make use try_get_module().
> They think its safe.  When you then echo the device name into the
> driver's unbind sysfs file, all hell breaks loose and the kernel oopses.
> 
> try_get_module is /totally/ useless for ensuring that things stick around.
> 
> The reality is that you can't make devices stick around.  Once that
> remove callback from the driver layer is called, that's it, the device
> _is_ going away whether you like it or not.  You can't stop it.  It's
> no good returning -EBUSY, because the remove return code is ignored.
> 
> What's more scarey is when you consider that in a real hotplug
> situation, when the remove callback is called, the device has
> /already/ gone.
> 
> So please, stop thinking that try_get_module() has some magic solution.
> Any "solution" to device lifetimes using try_get_module() totally misses
> the problem, and is just mere obfuscation and actually a bug in itself.

We need proper refcounting ofc, but we also need to make sure that as long
as the thing is around the text section for the final unref (at least
that) doesn't go poof. I'd prefer if the framework takes care of that
detail and drivers could just supply a THIS_MODULE at the right place.

But fully agree on your overall point that try_get_module alone is pure
snake oil.
-Daniel

Daniel Vetter Oct. 31, 2014, 3:59 p.m. UTC | #30

On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:
> On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
> > On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> > > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
> > > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > > >>>   * @driver_private: pointer to the bridge driver's internal context
> > > > >>>   */
> > > > >>>  struct drm_bridge {
> > > > >>> -     struct drm_device *dev;
> > > > >>> +     struct device *dev;
> > > > >>
> > > > >> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > > > >> drm structures still call it ->dev. Also, can't we use struct device_node
> > > > >> here like we do in the of helpers Russell added? See 7e435aad38083
> > > > >>
> > > > >
> > > > > I think this is modeled after the naming in drm_panel, FWIW. However,
> > > > > seems reasonable to keep the device_node instead.
> > > > 
> > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > drm_crtc drop the struct device and go directly to a struct
> > > > device_node. Since we don't really need the sturct device, the only
> > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > Should be all fairly simple to pull off with cocci.
> > > > 
> > > > Thierry?
> > > 
> > > The struct device * is in DRM panel because there's nothing device tree
> > > specific about the concept. Having a struct device_node * instead would
> > > indicate that it can only be used with a device tree, whereas the
> > > framework doesn't care the tiniest bit what type of device we have.
> > > 
> > > While the trend clearly is to use more device tree, I don't think we
> > > should make it impossible for anybody else to use these frameworks.
> > > 
> > > There are other advantages to keeping a struct device *, like having
> > > access to the proper device and its name. Also you get access to the
> > > device_node * via dev->of_node anyway. I don't see any advantage in
> > > switching to just a struct device_node *, only disadvantages.
> > 
> > Well the idea is to make the lookup key specific, and conditional on
> > #CONFIG_OF. If there's going to be another neat way to enumerate platform
> > devices then I think we should add that, too. Or maybe we should have a
> > void *platform_data or so.
> > 
> > The reason I really don't want a struct device * in core drm structures is
> > that two releases down the road people will have found tons of really
> > great ways to abuse them and re-create a midlayer. DRM core really should
> > only care about the sw objects and not be hw specific at all. Heck there's
> > not even an requirement to have any piece of actual hw, you could write a
> > completely fake drm driver (for e.g. testing like the new v4l driver).
> > 
> > Tbh I wonder a bit why we even have this registery embedded into the core
> > drm objects. Essentially the only thing you're doing is a list that maps
> > some platform specific key onto some subsystem specific driver structure
> > or fails the lookup. So instead of putting all these low-level details
> > into drm core structures can't we just have a generic hashtable/list for
> > this, plus some static inline helpers that cast the void * you get into
> > the one you want?
> > 
> > I also get the feeling that this really should be in the driver core (like
> > the component helpers), and that we should think a lot harder about
> > lifetimes and refcounting (see my other reply on that).
> 
> Yes, that sounds very useful indeed. Also see my reply to yours. =)

Just replying here with some of the irc discussions we've had. Since
drm_bridge/panel isn't a core drm interface object exposed to userspace
it's less critical. I still think that wasting a few brain cycles to have
a clear separation between the abstract interface object and how to bind
and unbind the pieces together is worthwhile, even though the cost when
getting it wrong is much less severe than in the case of a mandatory piece
of core infrastructure.

I think in general the recent armsoc motivated drm infrastructure lacks a
bit in attention to these details. E.g. the cma helpers are built into the
drm.ko module, but clearly are auxilliary library code. So they should be
pulled out and the headers clean up, so that we have a clear separation
between core and helpers. Otherwise someone will sooner or later screw up
and insert a helper depency into the core, and then we've started with the
midlayer mess. Same goes with drm_bridge/panel, which didn't even bother
to have separate headers from the core modeset header drm_crtc.h.

So would be great if someone could invest a bit of time into cleaning this
up. Writing proper api docs also helps a lot with achieving a clean and
sensible split ;-)
-Daniel

Daniel Vetter Oct. 31, 2014, 4:03 p.m. UTC | #31

On Wed, Oct 29, 2014 at 10:09:04AM +0100, Andrzej Hajda wrote:
> On 10/29/2014 08:58 AM, Daniel Vetter wrote:
> > On Tue, Oct 28, 2014 at 04:05:34PM +0100, Thierry Reding wrote:
> >> On Tue, Oct 28, 2014 at 08:16:44PM +0530, Ajay kumar wrote:
> >>> On Tue, Oct 28, 2014 at 8:11 PM, Thierry Reding
> >>> <[e-mail]> wrote:
> >>>> On Tue, Oct 28, 2014 at 03:19:36PM +0100, Daniel Vetter wrote:
> >>>>> On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar <[e-mail]> wrote:
> >>>>>> On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter <[e-mail]> wrote:
> >>>> [...]
> >>>>>>> Hm, if you do this can you pls also update drm_panel accordingly? It
> >>>>>>> shouldn't be a lot of fuzz and would make things around drm+dt more
> >>>>>>> consistent.
> >>>>>> Are you talking about using struct device_node instead of struct device?
> >>>>>> I guess you have misplaced the comment under the wrong section!
> >>>>>
> >>>>> Yeah, that should have been one up ;-)
> >>>>
> >>>> Like I said earlier, I don't think dropping struct device * in favour of
> >>>> struct device_node * is a good idea.
> >>> I am not sure about drm_panel.
> >>> But, I am not really doing anything with the struct device pointer in
> >>> case of bridge.
> >>> So, just wondering if it is really needed?
> >>
> >> I think it's useful to have it just to send the right message. DRM panel
> >> and DRM bridge aren't specific to device tree. They are completely
> >> generic and can work with any type of device, whether it was
> >> instantiated from the device tree or some other infrastructure. Dropping
> >> struct device * will make it effectively useless on anything but DT. I
> >> don't think we should strive for that, even if only DT-enabled platforms
> >> currently use them.
> > 
> > See my other reply, but I now think we should put neither into drm
> > structures. This "find me the driver structures for this device" problem
> > looks really generic, and it has nothing to do with the drm structures and
> > concepts like bridges/panels at all. It shouldn't be in there at all.
> > 
> > Adding it looks very much like reintroducing the drm midlayer that we just
> > finally made obsolete, just not at the top-level (struct drm_device) but
> > at a bunch of leaf nodes. I expect all the same issues though. And I'm
> > definitely not looking to de-midlayer more stuff that we're just adding.
> > 
> > Imo this should be solved as a separate helper thing, maybe in the driver
> > core akin to the component helpers from Russell.
> > -Daniel
> > 
> 
> As I understand you want something generic to look for panels, bridges,
> whatever and, like components, it should allow to safe hot plug/unplug.
> I have proposed such thing few months ago [1]. Have you looked at it?
> 
> [1]: https://lkml.org/lkml/2014/4/30/345

Yeah I think I've looked but wasn't convinced. I do agree though that we
should definitely aim for something in the driver core. Since if Greg
doesn't like it it's not suddenly better if we just hide it in the drm
subsystem. This really smells like a generic issue - after all we already
have a two implementations with bridges&panels already.

So maybe we need to augment the component framework with the possibility
to add additional devices later on at runtime, or something similar. Not
really sure since I don't have actual practice with these issues since
i915 doesn't (yet) have these problems.
-Daniel

Thierry Reding Nov. 3, 2014, 8:01 a.m. UTC | #32

On Fri, Oct 31, 2014 at 04:51:43PM +0100, Daniel Vetter wrote:
> On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote:
> > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
> > > That makes the entire thing a bit non-trivial, which is why I think it
> > > would be better as some generic helper. Which then gets embedded or
> > > instantiated for specific cases, like dt&drm_panel or dt&drm_bridge.
> > > Or maybe even acpi&drm_bridge, who knows ;-)
> > 
> > I worry a little about type safety. How will this generic helper know
> > what registry to look in for? Or conversely, if all these resources are
> > added to a single registry how do you know that they're of the correct
> > type? Failing to ensure this could cause situations where you're asking
> > for a panel and get a bridge in return because you've wrongly wired it
> > up in device tree for example.
> > 
> > But perhaps if both the registry and the device parts are turned into
> > helpers we could still have a single core implementation and then
> > instantiate that for each type, something roughly like this:
> > 
> > 	struct registry {
> > 		struct list_head list;
> > 		struct mutex lock;
> > 	};
> > 
> > 	struct registry_record {
> > 		struct list_head list;
> > 		struct module *owner;
> > 		struct kref *ref;
> > 
> > 		struct device *dev;
> > 	};
> > 
> > 	int registry_add(struct registry *registry, struct registry_record *record)
> > 	{
> > 		...
> > 		try_module_get(record->owner);
> > 		...
> > 	}
> > 
> > 	struct registry_record *registry_find_by_of_node(struct registry *registry,
> > 							 struct device_node *np)
> > 	{
> > 		...
> > 		kref_get(...);
> > 		...
> > 	}
> > 
> > That way it should be possible to embed these into other structures,
> > like so:
> > 
> > 	struct drm_panel {
> > 		struct registry_record record;
> > 		...
> > 	};
> > 
> > 	static struct registry drm_panels;
> > 
> > 	int drm_panel_add(struct drm_panel *panel)
> > 	{
> > 		return registry_add(&drm_panels, &panel->record);
> > 	}
> > 
> > 	struct drm_panel *of_drm_panel_find(struct device_node *np)
> > 	{
> > 		struct registry_record *record;
> > 
> > 		record = registry_find_by_of_node(&drm_panels, np);
> > 
> > 		return container_of(record, struct drm_panel, record);
> > 	}
> > 
> > Is that what you had in mind?
> 
> Yeah I've thought that we should instantiate using macros even, so that we
> have per-type registries. So you'd smash the usual set of
> DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take
> a (name, key, value) tripled. For the example here(of_drm_panel, struct
> device_node *, struct drm_panel *) or similar. I might be hand-waving over
> a few too many details though ;-)

Okay, I'll take a stab at this and see if I can convert DRM panel to it.

Thierry

Thierry Reding Nov. 3, 2014, 8:06 a.m. UTC | #33

On Fri, Oct 31, 2014 at 04:59:40PM +0100, Daniel Vetter wrote:
> On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:
> > On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
> > > On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> > > > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
> > > > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > > > >>>   * @driver_private: pointer to the bridge driver's internal context
> > > > > >>>   */
> > > > > >>>  struct drm_bridge {
> > > > > >>> -     struct drm_device *dev;
> > > > > >>> +     struct device *dev;
> > > > > >>
> > > > > >> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > > > > >> drm structures still call it ->dev. Also, can't we use struct device_node
> > > > > >> here like we do in the of helpers Russell added? See 7e435aad38083
> > > > > >>
> > > > > >
> > > > > > I think this is modeled after the naming in drm_panel, FWIW. However,
> > > > > > seems reasonable to keep the device_node instead.
> > > > > 
> > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > > drm_crtc drop the struct device and go directly to a struct
> > > > > device_node. Since we don't really need the sturct device, the only
> > > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > > Should be all fairly simple to pull off with cocci.
> > > > > 
> > > > > Thierry?
> > > > 
> > > > The struct device * is in DRM panel because there's nothing device tree
> > > > specific about the concept. Having a struct device_node * instead would
> > > > indicate that it can only be used with a device tree, whereas the
> > > > framework doesn't care the tiniest bit what type of device we have.
> > > > 
> > > > While the trend clearly is to use more device tree, I don't think we
> > > > should make it impossible for anybody else to use these frameworks.
> > > > 
> > > > There are other advantages to keeping a struct device *, like having
> > > > access to the proper device and its name. Also you get access to the
> > > > device_node * via dev->of_node anyway. I don't see any advantage in
> > > > switching to just a struct device_node *, only disadvantages.
> > > 
> > > Well the idea is to make the lookup key specific, and conditional on
> > > #CONFIG_OF. If there's going to be another neat way to enumerate platform
> > > devices then I think we should add that, too. Or maybe we should have a
> > > void *platform_data or so.
> > > 
> > > The reason I really don't want a struct device * in core drm structures is
> > > that two releases down the road people will have found tons of really
> > > great ways to abuse them and re-create a midlayer. DRM core really should
> > > only care about the sw objects and not be hw specific at all. Heck there's
> > > not even an requirement to have any piece of actual hw, you could write a
> > > completely fake drm driver (for e.g. testing like the new v4l driver).
> > > 
> > > Tbh I wonder a bit why we even have this registery embedded into the core
> > > drm objects. Essentially the only thing you're doing is a list that maps
> > > some platform specific key onto some subsystem specific driver structure
> > > or fails the lookup. So instead of putting all these low-level details
> > > into drm core structures can't we just have a generic hashtable/list for
> > > this, plus some static inline helpers that cast the void * you get into
> > > the one you want?
> > > 
> > > I also get the feeling that this really should be in the driver core (like
> > > the component helpers), and that we should think a lot harder about
> > > lifetimes and refcounting (see my other reply on that).
> > 
> > Yes, that sounds very useful indeed. Also see my reply to yours. =)
> 
> Just replying here with some of the irc discussions we've had. Since
> drm_bridge/panel isn't a core drm interface object exposed to userspace
> it's less critical. I still think that wasting a few brain cycles to have
> a clear separation between the abstract interface object and how to bind
> and unbind the pieces together is worthwhile, even though the cost when
> getting it wrong is much less severe than in the case of a mandatory piece
> of core infrastructure.
> 
> I think in general the recent armsoc motivated drm infrastructure lacks a
> bit in attention to these details. E.g. the cma helpers are built into the
> drm.ko module, but clearly are auxilliary library code. So they should be
> pulled out and the headers clean up, so that we have a clear separation
> between core and helpers. Otherwise someone will sooner or later screw up
> and insert a helper depency into the core, and then we've started with the
> midlayer mess. Same goes with drm_bridge/panel, which didn't even bother
> to have separate headers from the core modeset header drm_crtc.h.

DRM panel does have a separate header. It's still built into the core
DRM module, but using a separate drm-$(CONFIG_DRM_PANEL) += drm_panel.o
entry in the makefile. At the time it didn't seem worth to add a
completely separate module given the size of the code and the overhead
associated with having a separate module.

Do you still want me to split it off into a separate module to clarify
that it isn't part of the core?

> So would be great if someone could invest a bit of time into cleaning this
> up. Writing proper api docs also helps a lot with achieving a clean and
> sensible split ;-)

There's a bit of API documentation for panels, but I'll see if I can
find some time to enhance it.

Thierry

Daniel Vetter Nov. 3, 2014, 8:11 a.m. UTC | #34

On Mon, Nov 03, 2014 at 09:06:04AM +0100, Thierry Reding wrote:
> On Fri, Oct 31, 2014 at 04:59:40PM +0100, Daniel Vetter wrote:
> > On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:
> > > On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:
> > > > On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:
> > > > > On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:
> > > > > > On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul <[e-mail]> wrote:
> > > > > > >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs {
> > > > > > >>>   * @driver_private: pointer to the bridge driver's internal context
> > > > > > >>>   */
> > > > > > >>>  struct drm_bridge {
> > > > > > >>> -     struct drm_device *dev;
> > > > > > >>> +     struct device *dev;
> > > > > > >>
> > > > > > >> Please don't rename the ->dev pointer into drm. Because _all_ the other
> > > > > > >> drm structures still call it ->dev. Also, can't we use struct device_node
> > > > > > >> here like we do in the of helpers Russell added? See 7e435aad38083
> > > > > > >>
> > > > > > >
> > > > > > > I think this is modeled after the naming in drm_panel, FWIW. However,
> > > > > > > seems reasonable to keep the device_node instead.
> > > > > > 
> > > > > > Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with
> > > > > > drm_crtc drop the struct device and go directly to a struct
> > > > > > device_node. Since we don't really need the sturct device, the only
> > > > > > thing we care about is the of_node. For added bonus wrap an #ifdef
> > > > > > CONFIG_OF around all the various struct device_node in drm_foo.h.
> > > > > > Should be all fairly simple to pull off with cocci.
> > > > > > 
> > > > > > Thierry?
> > > > > 
> > > > > The struct device * is in DRM panel because there's nothing device tree
> > > > > specific about the concept. Having a struct device_node * instead would
> > > > > indicate that it can only be used with a device tree, whereas the
> > > > > framework doesn't care the tiniest bit what type of device we have.
> > > > > 
> > > > > While the trend clearly is to use more device tree, I don't think we
> > > > > should make it impossible for anybody else to use these frameworks.
> > > > > 
> > > > > There are other advantages to keeping a struct device *, like having
> > > > > access to the proper device and its name. Also you get access to the
> > > > > device_node * via dev->of_node anyway. I don't see any advantage in
> > > > > switching to just a struct device_node *, only disadvantages.
> > > > 
> > > > Well the idea is to make the lookup key specific, and conditional on
> > > > #CONFIG_OF. If there's going to be another neat way to enumerate platform
> > > > devices then I think we should add that, too. Or maybe we should have a
> > > > void *platform_data or so.
> > > > 
> > > > The reason I really don't want a struct device * in core drm structures is
> > > > that two releases down the road people will have found tons of really
> > > > great ways to abuse them and re-create a midlayer. DRM core really should
> > > > only care about the sw objects and not be hw specific at all. Heck there's
> > > > not even an requirement to have any piece of actual hw, you could write a
> > > > completely fake drm driver (for e.g. testing like the new v4l driver).
> > > > 
> > > > Tbh I wonder a bit why we even have this registery embedded into the core
> > > > drm objects. Essentially the only thing you're doing is a list that maps
> > > > some platform specific key onto some subsystem specific driver structure
> > > > or fails the lookup. So instead of putting all these low-level details
> > > > into drm core structures can't we just have a generic hashtable/list for
> > > > this, plus some static inline helpers that cast the void * you get into
> > > > the one you want?
> > > > 
> > > > I also get the feeling that this really should be in the driver core (like
> > > > the component helpers), and that we should think a lot harder about
> > > > lifetimes and refcounting (see my other reply on that).
> > > 
> > > Yes, that sounds very useful indeed. Also see my reply to yours. =)
> > 
> > Just replying here with some of the irc discussions we've had. Since
> > drm_bridge/panel isn't a core drm interface object exposed to userspace
> > it's less critical. I still think that wasting a few brain cycles to have
> > a clear separation between the abstract interface object and how to bind
> > and unbind the pieces together is worthwhile, even though the cost when
> > getting it wrong is much less severe than in the case of a mandatory piece
> > of core infrastructure.
> > 
> > I think in general the recent armsoc motivated drm infrastructure lacks a
> > bit in attention to these details. E.g. the cma helpers are built into the
> > drm.ko module, but clearly are auxilliary library code. So they should be
> > pulled out and the headers clean up, so that we have a clear separation
> > between core and helpers. Otherwise someone will sooner or later screw up
> > and insert a helper depency into the core, and then we've started with the
> > midlayer mess. Same goes with drm_bridge/panel, which didn't even bother
> > to have separate headers from the core modeset header drm_crtc.h.
> 
> DRM panel does have a separate header. It's still built into the core
> DRM module, but using a separate drm-$(CONFIG_DRM_PANEL) += drm_panel.o
> entry in the makefile. At the time it didn't seem worth to add a
> completely separate module given the size of the code and the overhead
> associated with having a separate module.
> 
> Do you still want me to split it off into a separate module to clarify
> that it isn't part of the core?

Oh, it doesn't need to be a complete standalone module, smashing it into
drm_kms_helper is imo totally ok. It's just to make really sure that
helpers are helpers and there's never a depency from drm.ko to any
optional helper code. Having a separate module for all the helper code
helps a lot in ensure that. Also, everything a helper can do, a driver
should be able to do too. Again a separate helper ensures that you haven't
missed any EXPORT_SYMBOL. Which should then be a good reminder to update
the kerneldoc ;-)

> > So would be great if someone could invest a bit of time into cleaning this
> > up. Writing proper api docs also helps a lot with achieving a clean and
> > sensible split ;-)
> 
> There's a bit of API documentation for panels, but I'll see if I can
> find some time to enhance it.

Imo pulling into the DocBook template is also important, since if you do
that the 0-day tester will complain if the kerneldoc gets out of sync.
Which does increases the changes of it staying up-to-date a lot.

Cheers, Daniel

Ajay kumar Nov. 3, 2014, 8:26 a.m. UTC | #35

On Mon, Nov 3, 2014 at 1:31 PM, Thierry Reding <[e-mail]> wrote:
> On Fri, Oct 31, 2014 at 04:51:43PM +0100, Daniel Vetter wrote:
>> On Wed, Oct 29, 2014 at 10:14:37AM +0100, Thierry Reding wrote:
>> > On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
>> > > That makes the entire thing a bit non-trivial, which is why I think it
>> > > would be better as some generic helper. Which then gets embedded or
>> > > instantiated for specific cases, like dt&drm_panel or dt&drm_bridge.
>> > > Or maybe even acpi&drm_bridge, who knows ;-)
>> >
>> > I worry a little about type safety. How will this generic helper know
>> > what registry to look in for? Or conversely, if all these resources are
>> > added to a single registry how do you know that they're of the correct
>> > type? Failing to ensure this could cause situations where you're asking
>> > for a panel and get a bridge in return because you've wrongly wired it
>> > up in device tree for example.
>> >
>> > But perhaps if both the registry and the device parts are turned into
>> > helpers we could still have a single core implementation and then
>> > instantiate that for each type, something roughly like this:
>> >
>> >     struct registry {
>> >             struct list_head list;
>> >             struct mutex lock;
>> >     };
>> >
>> >     struct registry_record {
>> >             struct list_head list;
>> >             struct module *owner;
>> >             struct kref *ref;
>> >
>> >             struct device *dev;
>> >     };
>> >
>> >     int registry_add(struct registry *registry, struct registry_record *record)
>> >     {
>> >             ...
>> >             try_module_get(record->owner);
>> >             ...
>> >     }
>> >
>> >     struct registry_record *registry_find_by_of_node(struct registry *registry,
>> >                                                      struct device_node *np)
>> >     {
>> >             ...
>> >             kref_get(...);
>> >             ...
>> >     }
>> >
>> > That way it should be possible to embed these into other structures,
>> > like so:
>> >
>> >     struct drm_panel {
>> >             struct registry_record record;
>> >             ...
>> >     };
>> >
>> >     static struct registry drm_panels;
>> >
>> >     int drm_panel_add(struct drm_panel *panel)
>> >     {
>> >             return registry_add(&drm_panels, &panel->record);
>> >     }
>> >
>> >     struct drm_panel *of_drm_panel_find(struct device_node *np)
>> >     {
>> >             struct registry_record *record;
>> >
>> >             record = registry_find_by_of_node(&drm_panels, np);
>> >
>> >             return container_of(record, struct drm_panel, record);
>> >     }
>> >
>> > Is that what you had in mind?
>>
>> Yeah I've thought that we should instantiate using macros even, so that we
>> have per-type registries. So you'd smash the usual set of
>> DECLARE/DEFINE_AUX_DEV_REGISTRY into headers/source files, and they'd take
>> a (name, key, value) tripled. For the example here(of_drm_panel, struct
>> device_node *, struct drm_panel *) or similar. I might be hand-waving over
>> a few too many details though ;-)
>
> Okay, I'll take a stab at this and see if I can convert DRM panel to it.
It would be great if you can do this soon. I would anyhow need a reference
for converting bridge framework as per Daniel's requirement :)

Ajay

Patch

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 4a55d59..bdbfb6f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -19,6 +19,7 @@  drm-y       :=	drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
 drm-$(CONFIG_PCI) += ati_pcigart.o
+drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o
 drm-$(CONFIG_DRM_PANEL) += drm_panel.o
 drm-$(CONFIG_OF) += drm_of.o
 
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 884923f..5a8e907 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -1,5 +1,16 @@ 
-config DRM_PTN3460
-	tristate "PTN3460 DP/LVDS bridge"
+config DRM_BRIDGE
+	tristate
 	depends on DRM
 	select DRM_KMS_HELPER
+	help
+	  Bridge registration and lookup framework.
+
+menu "bridge chips"
+	depends on DRM_BRIDGE
+
+config DRM_PTN3460
+	tristate "PTN3460 DP/LVDS bridge"
+	depends on DRM_BRIDGE
 	---help---
+
+endmenu
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
new file mode 100644
index 0000000..b2d43fd
--- /dev/null
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -0,0 +1,102 @@ 
+/*
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sub license,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+
+#include <drm/drm_crtc.h>
+
+#include "drm/drmP.h"
+
+static DEFINE_MUTEX(bridge_lock);
+static LIST_HEAD(bridge_list);
+
+int drm_bridge_add(struct drm_bridge *bridge)
+{
+	mutex_lock(&bridge_lock);
+	list_add_tail(&bridge->list, &bridge_list);
+	mutex_unlock(&bridge_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_bridge_add);
+
+void drm_bridge_remove(struct drm_bridge *bridge)
+{
+	mutex_lock(&bridge_lock);
+	list_del_init(&bridge->list);
+	mutex_unlock(&bridge_lock);
+}
+EXPORT_SYMBOL(drm_bridge_remove);
+
+int drm_bridge_attach(struct drm_bridge *bridge,
+				struct drm_encoder *encoder)
+{
+	int ret;
+
+	if (!bridge || !encoder)
+		return -EINVAL;
+
+	if (bridge->encoder)
+		return -EBUSY;
+
+	encoder->bridge = bridge;
+	bridge->encoder = encoder;
+	bridge->drm = encoder->dev;
+
+	ret = drm_bridge_init(bridge->drm, bridge);
+	if (ret) {
+		DRM_ERROR("Failed to register bridge with drm\n");
+		return ret;
+	}
+
+	if (bridge->funcs->attach)
+		return bridge->funcs->attach(bridge);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_bridge_attach);
+
+#ifdef CONFIG_OF
+struct drm_bridge *of_drm_find_bridge(struct device_node *np)
+{
+	struct drm_bridge *bridge;
+
+	mutex_lock(&bridge_lock);
+
+	list_for_each_entry(bridge, &bridge_list, list) {
+		if (bridge->dev->of_node == np) {
+			mutex_unlock(&bridge_lock);
+			return bridge;
+		}
+	}
+
+	mutex_unlock(&bridge_lock);
+	return NULL;
+}
+EXPORT_SYMBOL(of_drm_find_bridge);
+#endif
+
+MODULE_AUTHOR("Ajay Kumar <[e-mail]>");
+MODULE_DESCRIPTION("DRM bridge infrastructure");
+MODULE_LICENSE("GPL and additional rights");
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 25f5cfa..2fb22fa 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1039,7 +1039,7 @@  int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge)
 	if (ret)
 		goto out;
 
-	bridge->dev = dev;
+	bridge->drm = dev;
 
 	list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
 	dev->mode_config.num_bridge++;
@@ -1058,7 +1058,7 @@  EXPORT_SYMBOL(drm_bridge_init);
  */
 void drm_bridge_cleanup(struct drm_bridge *bridge)
 {
-	struct drm_device *dev = bridge->dev;
+	struct drm_device *dev = bridge->drm;
 
 	drm_modeset_lock_all(dev);
 	drm_mode_object_put(dev, &bridge->base);
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 0309539..bc9e5ff 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -33,7 +33,7 @@  static void hdmi_bridge_destroy(struct drm_bridge *bridge)
 
 static void power_on(struct drm_bridge *bridge)
 {
-	struct drm_device *dev = bridge->dev;
+	struct drm_device *dev = bridge->drm;
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
 	const struct hdmi_platform_config *config = hdmi->config;
@@ -67,7 +67,7 @@  static void power_on(struct drm_bridge *bridge)
 
 static void power_off(struct drm_bridge *bridge)
 {
-	struct drm_device *dev = bridge->dev;
+	struct drm_device *dev = bridge->drm;
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
 	const struct hdmi_platform_config *config = hdmi->config;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f48a436..f6f426f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -629,6 +629,7 @@  struct drm_plane {
 
 /**
  * drm_bridge_funcs - drm_bridge control functions
+ * @attach: Called during drm_bridge_attach
  * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge
  * @disable: Called right before encoder prepare, disables the bridge
  * @post_disable: Called right after encoder prepare, for lockstepped disable
@@ -638,6 +639,7 @@  struct drm_plane {
  * @destroy: make object go away
  */
 struct drm_bridge_funcs {
+	int (*attach)(struct drm_bridge *bridge);
 	bool (*mode_fixup)(struct drm_bridge *bridge,
 			   const struct drm_display_mode *mode,
 			   struct drm_display_mode *adjusted_mode);
@@ -660,8 +662,11 @@  struct drm_bridge_funcs {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
-	struct drm_device *dev;
+	struct device *dev;
+	struct drm_device *drm;
+	struct drm_encoder *encoder;
 	struct list_head head;
+	struct list_head list;
 
 	struct drm_mode_object base;
 
@@ -906,6 +911,11 @@  extern void drm_connector_cleanup(struct drm_connector *connector);
 /* helper to unplug all connectors from sysfs for device */
 extern void drm_connector_unplug_all(struct drm_device *dev);
 
+extern int drm_bridge_add(struct drm_bridge *bridge);
+extern void drm_bridge_remove(struct drm_bridge *bridge);
+extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
+extern int drm_bridge_attach(struct drm_bridge *bridge,
+				struct drm_encoder *encoder);
 extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge);
 extern void drm_bridge_cleanup(struct drm_bridge *bridge);