Re: [PATCH 4/5] drm/exynos: Initialize ptn3460 if present

[Inki Dae]

2013/10/4 Sean Paul <seanpaul@xxxxxxxxxxxx>:
> On Thu, Oct 3, 2013 at 1:18 PM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote:
>> 2013/10/4 Sean Paul <seanpaul@xxxxxxxxxxxx>:
>>> On Thu, Oct 3, 2013 at 10:43 AM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote:
>>>> 2013/10/2 Sean Paul <seanpaul@xxxxxxxxxxxx>:
>>>>> This patch adds code to look for the ptn3460 in the device tree file on
>>>>> exynos initialization. If ptn node is found, the driver will initialize
>>>>> the ptn3460 driver and skip creating a DP connector (since the bridge
>>>>> driver will register its own connector).
>>>>>
>>>>> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
>>>>> ---
>>>>>  drivers/gpu/drm/exynos/exynos_drm_core.c | 44 +++++++++++++++++++++++++++++++-
>>>>>  1 file changed, 43 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c
>>>>> index 1bef6dc..9cf4476 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c
>>>>> @@ -12,7 +12,9 @@
>>>>>   * option) any later version.
>>>>>   */
>>>>>
>>>>> +#include <linux/of_i2c.h>
>>>>>  #include <drm/drmP.h>
>>>>> +#include <drm/bridge/ptn3460.h>
>>>>>  #include "exynos_drm_drv.h"
>>>>>  #include "exynos_drm_encoder.h"
>>>>>  #include "exynos_drm_connector.h"
>>>>> @@ -20,6 +22,40 @@
>>>>>
>>>>>  static LIST_HEAD(exynos_drm_subdrv_list);
>>>>>
>>>>> +struct bridge_init {
>>>>> +       struct i2c_client *client;
>>>>> +       struct device_node *node;
>>>>> +};
>>>>> +
>>>>> +static bool find_bridge(const char *name, struct bridge_init *bridge)
>>>>> +{
>>>>> +       bridge->client = NULL;
>>>>> +       bridge->node = of_find_node_by_name(NULL, name);
>>>>
>>>> Not clear to me. Why do you try to handle device tree here, not real
>>>> device driver?. How about adding a output property to board specific
>>>> fimd dt node: i.e. output = <&ptn3460_bridge>?
>>>
>>> The problem doing something like this is that we won't have a handle
>>> to drm_device if it's just a standalone driver, and so we won't be
>>> able to register the bridge or connector. We need this init call one
>>> way or another.
>>>
>>
>> At least, dt binding shoul be done in real device driver so this way
>> is not good. Let's find a better way.
>>
>
> Right, so this is kind of tricky. If you do it in a "real" device
> driver, you end up parsing the dt stuff in the probe, and then racing
> the init callback. I figured it would be best just to do everything in
> one place without races.
>
> Hopefully I'm just missing a good way to solve this problem, any concrete ideas?
>
>>>
>>>> Actually, the output
>>>> device of fimd hw could be one of MIPI-DSI, eDP, mDNIe, LVDS bridge,
>>>> or LCD. And then, let's find ptn3460-bridge node in the fimd driver,
>>>> and initialize the ptn3460 bridge driver, and get power on or off
>>>> through  exynos_drm_display_ops of the fimd driver. And all these
>>>> codes could be hided from fimd driver by moving them into
>>>> exynos_drm_display_ops. Of course, for this, you would need additional
>>>> works. So let's do it if needed.
>>>>
>>>> The below is the outline of device tree I recommend,
>>>>
>>>> In board dts,
>>>>        i2c@I2CD000 {
>>>>              ptn3460_bridge: prn3460-bridge@20 {
>>>>                     ...
>>>>              }
>>>>         }
>>>>
>>>>         fimd@11c00000 {
>>>>              ...
>>>>              output_dev = <&ptn3460_bridge>;
>>>>         }
>>>>
>>>> With this, I believe that you can do all things you want for
>>>> controlling the LVDS bridge in fimd driver.
>>>>
>>>
>>> No, this isn't what I want to do. The bridge should not hang off fimd
>>> since it's a crtc. The bridge should only be initialized by the DP
>>> driver (it doesn't make sense to initialize ptn when you're using
>>> MIPI/LVDS/whatever).
>>
>> I don't mean that the bridge device should be initialized by fimd
>> directly but the fimd driver provides just interfaces abstracted to
>> control the bridge device. And basically, the exynos_drm_display_ops
>> shouldn't be in fimd driver but in real connector driver; i.e. lcd
>> panel or LVDS driver. The reason I placed the exynos_drm_display_ops
>> in fimd driver is that lcd panel driver is controlled by lcd class
>> depended on Linux framebuffer, and I thought the panel driver should
>> be shared with drm driver in case of ARM SoC. The
>> exynos_drm_display_ops should be moved into right place if something
>> better exists some time or other.
>>
>> So how can the DP driver control the bridge device as of now? the DP
>> you mentioned would be eDP, and the eDP driver is placed in
>> drivers/video/exynos/, and also MIPI-DSI driver.
>>
>
> It can't. The DP driver just operates on its own and display either
> comes up or it doesn't depending on whether fimd happens to be
> initialized first.

Ok, I don't know the DP hardware well. But, MIPI-DSI driver is
depending on fimd on/off ordering. ie. to enable display hw pipe, the
ordering should be FIMD----MIPI-DSI-----LCD because initial commands
_cannot be set_ to lcd panel if fimd is off. And to disable that, the
ordering should be LCD-------MIPI-DSI-------FIMD in same reason: to
get lcd panel off, off commands should be set to lcd panel. In similar
reason, I had posted FB_EARLY_EVENT_BLANK feature to mainline and that
have been merged.

> As I mentioned earlier, I have a patch set which
> moves DP driver into drm/exynos and removes the display_ops from fimd.
> That will fix this issue.

Ah... as you may know I mentioned about this issue some time ago;
moving eDP driver(maybe??) into drm/exynos.  At that time, I had
rejected it because I thought we should share these bus drivers with
Linux framebuffer driver: actually, I planned to use CDF for this.
But....  I'm not sure even CDF as of now. So let's have a discussion
about this issue with other people: it doesn't mean I oppose your
patch set.

>
> Sean
>
>
>>>
>>> Since the actual crtc/encoder drivers are abstracted through
>>> exynos_drm_core/crtc/encoder, we need to initialize the ptn driver in
>>> the abstraction layers in order to hook it directly into drm.
>>>
>>> Sean
>>>
>>>
>>>> Thanks,
>>>> Inki Dae
>>>>
>>>>> +       if (!bridge->node)
>>>>> +               return false;
>>>>> +
>>>>> +       bridge->client = of_find_i2c_device_by_node(bridge->node);
>>>>> +       if (!bridge->client)
>>>>> +               return false;
>>>>> +
>>>>> +       return true;
>>>>> +}
>>>>> +
>>>>> +/* returns the number of bridges attached */
>>>>> +static int exynos_drm_attach_lcd_bridge(struct drm_device *dev,
>>>>> +               struct drm_encoder *encoder)
>>>>> +{
>>>>> +       struct bridge_init bridge;
>>>>> +       int ret;
>>>>> +
>>>>> +       if (find_bridge("ptn3460-bridge", &bridge)) {
>>>>> +               ret = ptn3460_init(dev, encoder, bridge.client, bridge.node);
>>>>> +               if (!ret)
>>>>> +                       return 1;
>>>>> +       }
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>>  static int exynos_drm_create_enc_conn(struct drm_device *dev,
>>>>>                                         struct exynos_drm_subdrv *subdrv)
>>>>>  {
>>>>> @@ -36,6 +72,13 @@ static int exynos_drm_create_enc_conn(struct drm_device *dev,
>>>>>                 DRM_ERROR("failed to create encoder\n");
>>>>>                 return -EFAULT;
>>>>>         }
>>>>> +       subdrv->encoder = encoder;
>>>>> +
>>>>> +       if (subdrv->manager->display_ops->type == EXYNOS_DISPLAY_TYPE_LCD) {
>>>>> +               ret = exynos_drm_attach_lcd_bridge(dev, encoder);
>>>>> +               if (ret)
>>>>> +                       return 0;
>>>>> +       }
>>>>>
>>>>>         /*
>>>>>          * create and initialize a connector for this sub driver and
>>>>> @@ -48,7 +91,6 @@ static int exynos_drm_create_enc_conn(struct drm_device *dev,
>>>>>                 goto err_destroy_encoder;
>>>>>         }
>>>>>
>>>>> -       subdrv->encoder = encoder;
>>>>>         subdrv->connector = connector;
>>>>>
>>>>>         return 0;
>>>>> --
>>>>> 1.8.4
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> linux-arm-kernel mailing list
>>>>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/