| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAAQKjZOw-eVS5QU5y2We+2xEQHFVCZmsAH5Gk=Tu_44bay3tqQ@mail.gmail.com>
Date: Fri, 4 Oct 2013 03:10:30 +0900
From: Inki Dae <inki.dae@...sung.com>
To: Sean Paul <seanpaul@...omium.org>
Cc: "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-samsung-soc@...r.kernel.org"
<linux-samsung-soc@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
DRI mailing list <dri-devel@...ts.freedesktop.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 4/5] drm/exynos: Initialize ptn3460 if present
2013/10/4 Sean Paul <seanpaul@...omium.org>:
> On Thu, Oct 3, 2013 at 1:18 PM, Inki Dae <inki.dae@...sung.com> wrote:
>> 2013/10/4 Sean Paul <seanpaul@...omium.org>:
>>> On Thu, Oct 3, 2013 at 10:43 AM, Inki Dae <inki.dae@...sung.com> wrote:
>>>> 2013/10/2 Sean Paul <seanpaul@...omium.org>:
>>>>> 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@...omium.org>
>>>>> ---
>>>>> 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@...D000 {
>>>> ptn3460_bridge: prn3460-bridge@20 {
>>>> ...
>>>> }
>>>> }
>>>>
>>>> fimd@...00000 {
>>>> ...
>>>> 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@...ts.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@...ts.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists