All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Rob Clark <rob.clark@linaro.org>
Cc: Tomi Valkeinen <tomba@iki.fi>, Archit Taneja <archit@ti.com>,
	linux-omap@vger.kernel.org, linux-fbdev@vger.kernel.org,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available
Date: Wed, 07 Nov 2012 10:01:21 +0000	[thread overview]
Message-ID: <509A3171.8010100@ti.com> (raw)
In-Reply-To: <CAF6AEGuNyqO-teda1sv8c8m9x1j8Fm13HYuWB4N3gccJeKFTFg@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 5101 bytes --]

On 2012-11-06 16:40, Rob Clark wrote:

> I mean, similar to how we handle the subdev for dmm..  the
> omap_drm_init() does the platform_driver_register() for the dmm device
> before the platform_driver_register() for omapdrm itself, so we know
> if there is a dmm device, the driver gets probed first before omapdrm.

Well, I consider that a bit hacky too. That's not how linux device
framework is supposed to be used. I know it makes life easier to do the
registering like that, though.

> It could be a matter of iterating through a list, or something like
> this.. that is basically an implementation detail.  But the end result
> is that the order the drivers are registered is controlled so the
> probe sequence works out properly (not to mention suspend/resume
> sequence).

I feel that this kind of solution just tries to solve the generic
problem of init/suspend ordering in a single driver, instead of fixing
the device framework itself.

Or, of course it's possible that our drive architecture just sucks, and
the device framework is fine. In that case the workaround is even worse,
and we should fix our drivers.

>> I think we should support proper hotplugging of the panels. This would
>> fix the problem about init order, but it would also give us device
>> hotplug support. Obviously nobody is going to solder panel to a running
>> board, but I don't see any reason why panels, or, more likely, panels on
>> an add-on boards (like the capes being discussed in omap ml) would not
>> be hotpluggable using whatever connector is used on the particular use case.
>>
>> And even if we don't support removing of the devices, things like the
>> add-on capes could cause the panel on the cape to be identified at some
>> late time (the panel is not described in the board file or DT data, but
>> found at runtime depending on the ID of the cape). This would add
>> another step to the init sequence that should be just right, if we don't
>> support hotplug.
> 
> If capes are really hot-pluggable, then maybe it is worth thinking
> about how to make this more dynamic.  Although it is a bigger problem,
> which involves userspace being aware that connectors can dynamically
> appear/disappear.  And the dynamic disappearing is something I worry
> about more.. it adds the possibility of all sorts of interesting race
> conditions, such as connectors disappearing in the middle of modeset.
> I prefer not making things more complicated and error prone than they
> need to be.  If there is not a legitimate use case for connector hw
> dynamically appearing/disappearing then I don't think we should go
> there.  It sounds nice and simple and clean, but in reality I think it
> just introduces a whole lot of ways for things to go wrong.  A wise

Yes, I agree that it complicates things.

> man once said:
> 
> https://github.com/robclark/kernel-omap4/blob/master/Documentation/SubmittingPatches#L700

I've done things simple lots of times in the omapdss driver, only to
have to rewrite the thing in more complex way later to accommodate new
scenarios. I think it's good to write the code in a bit more generic way
than the use case at the moment of writing requires, because more often
than not, it'll save time in the future.

Hotplugging is not some abstract future scenario, we already have
hardware that could use it. For example, omap3 SDP board has a
switchable output to DVI or LCD panel. In this case we know what the two
options are, but the disabled component is still effectually removed
from the system, and plugged back in when it's enabled.

Hotplug is not a high priority item, but I do wish we get it supported
in common panel framework. Then it's at least possible to extend drm in
the future to support it.



Anyway, this makes me wonder... omapdrm currently maps the elements of
the whole video pipeline to drm elements (encoder, connector, etc).
Would it make more sense to just map the DISPC to these drm elements?
Connector would then be the output from DISPC.

This would map the drm elements to the static hardware blocks, and the
meaning of those blocks would be quite similar to what they are in the
desktop world (I guess).

The panel driver, the external chips, and the DSS internal output blocks
(dsi, dpi, ...) would be handled separately from those drm elements. The
DSS internal blocks are static, of course, but they can be effectively
considered the same way as external chips.

The omapdrm driver needs of course to access those separate elements
also, but that shouldn't be a problem. If omapdrm needs to call a
function in the panel driver, all it needs to do is go through the chain
to find the panel. Well, except if one output connected two two panels
via a bridge chip...

And if drm is at some point extended to support panel drivers, or chains
of external display entities, it would be easier to add that support.

What would it require the manage the elements like that? Would it help?
It sounds to me that this would simplify the model.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Rob Clark <rob.clark@linaro.org>
Cc: Tomi Valkeinen <tomba@iki.fi>, Archit Taneja <archit@ti.com>,
	linux-omap@vger.kernel.org, linux-fbdev@vger.kernel.org,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available
Date: Wed, 7 Nov 2012 12:01:21 +0200	[thread overview]
Message-ID: <509A3171.8010100@ti.com> (raw)
In-Reply-To: <CAF6AEGuNyqO-teda1sv8c8m9x1j8Fm13HYuWB4N3gccJeKFTFg@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 5101 bytes --]

On 2012-11-06 16:40, Rob Clark wrote:

> I mean, similar to how we handle the subdev for dmm..  the
> omap_drm_init() does the platform_driver_register() for the dmm device
> before the platform_driver_register() for omapdrm itself, so we know
> if there is a dmm device, the driver gets probed first before omapdrm.

Well, I consider that a bit hacky too. That's not how linux device
framework is supposed to be used. I know it makes life easier to do the
registering like that, though.

> It could be a matter of iterating through a list, or something like
> this.. that is basically an implementation detail.  But the end result
> is that the order the drivers are registered is controlled so the
> probe sequence works out properly (not to mention suspend/resume
> sequence).

I feel that this kind of solution just tries to solve the generic
problem of init/suspend ordering in a single driver, instead of fixing
the device framework itself.

Or, of course it's possible that our drive architecture just sucks, and
the device framework is fine. In that case the workaround is even worse,
and we should fix our drivers.

>> I think we should support proper hotplugging of the panels. This would
>> fix the problem about init order, but it would also give us device
>> hotplug support. Obviously nobody is going to solder panel to a running
>> board, but I don't see any reason why panels, or, more likely, panels on
>> an add-on boards (like the capes being discussed in omap ml) would not
>> be hotpluggable using whatever connector is used on the particular use case.
>>
>> And even if we don't support removing of the devices, things like the
>> add-on capes could cause the panel on the cape to be identified at some
>> late time (the panel is not described in the board file or DT data, but
>> found at runtime depending on the ID of the cape). This would add
>> another step to the init sequence that should be just right, if we don't
>> support hotplug.
> 
> If capes are really hot-pluggable, then maybe it is worth thinking
> about how to make this more dynamic.  Although it is a bigger problem,
> which involves userspace being aware that connectors can dynamically
> appear/disappear.  And the dynamic disappearing is something I worry
> about more.. it adds the possibility of all sorts of interesting race
> conditions, such as connectors disappearing in the middle of modeset.
> I prefer not making things more complicated and error prone than they
> need to be.  If there is not a legitimate use case for connector hw
> dynamically appearing/disappearing then I don't think we should go
> there.  It sounds nice and simple and clean, but in reality I think it
> just introduces a whole lot of ways for things to go wrong.  A wise

Yes, I agree that it complicates things.

> man once said:
> 
> https://github.com/robclark/kernel-omap4/blob/master/Documentation/SubmittingPatches#L700

I've done things simple lots of times in the omapdss driver, only to
have to rewrite the thing in more complex way later to accommodate new
scenarios. I think it's good to write the code in a bit more generic way
than the use case at the moment of writing requires, because more often
than not, it'll save time in the future.

Hotplugging is not some abstract future scenario, we already have
hardware that could use it. For example, omap3 SDP board has a
switchable output to DVI or LCD panel. In this case we know what the two
options are, but the disabled component is still effectually removed
from the system, and plugged back in when it's enabled.

Hotplug is not a high priority item, but I do wish we get it supported
in common panel framework. Then it's at least possible to extend drm in
the future to support it.



Anyway, this makes me wonder... omapdrm currently maps the elements of
the whole video pipeline to drm elements (encoder, connector, etc).
Would it make more sense to just map the DISPC to these drm elements?
Connector would then be the output from DISPC.

This would map the drm elements to the static hardware blocks, and the
meaning of those blocks would be quite similar to what they are in the
desktop world (I guess).

The panel driver, the external chips, and the DSS internal output blocks
(dsi, dpi, ...) would be handled separately from those drm elements. The
DSS internal blocks are static, of course, but they can be effectively
considered the same way as external chips.

The omapdrm driver needs of course to access those separate elements
also, but that shouldn't be a problem. If omapdrm needs to call a
function in the panel driver, all it needs to do is go through the chain
to find the panel. Well, except if one output connected two two panels
via a bridge chip...

And if drm is at some point extended to support panel drivers, or chains
of external display entities, it would be easier to add that support.

What would it require the manage the elements like that? Would it help?
It sounds to me that this would simplify the model.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

  reply	other threads:[~2012-11-07 10:01 UTC|newest]

Thread overview: 76+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-10-30 16:09 [PATCH 00/12] OMAPDSS: use DSI PLL clk for DPI Tomi Valkeinen
2012-10-30 16:09 ` Tomi Valkeinen
2012-10-30 16:09 ` [PATCH 01/12] OMAPFB: remove use of extended edid block Tomi Valkeinen
2012-10-30 16:09   ` Tomi Valkeinen
2012-10-31  6:10   ` Archit Taneja
2012-10-31  6:22     ` Archit Taneja
2012-10-31  6:23     ` Tomi Valkeinen
2012-10-31  6:23       ` Tomi Valkeinen
2012-10-30 16:09 ` [PATCH 02/12] OMAPFB: improve mode selection from EDID Tomi Valkeinen
2012-10-30 16:09   ` Tomi Valkeinen
2012-10-30 16:10 ` [PATCH 03/12] OMAPDSS: fix DPI & DSI init order Tomi Valkeinen
2012-10-30 16:10   ` Tomi Valkeinen
2012-10-30 16:10 ` [PATCH 04/12] OMAPDSS: fix DSI2 PLL clk names Tomi Valkeinen
2012-10-30 16:10   ` Tomi Valkeinen
2012-10-30 16:10 ` [PATCH 05/12] OMAPDSS: DSI: skip odd dividers when pck >= 100MHz Tomi Valkeinen
2012-10-30 16:10   ` Tomi Valkeinen
2012-10-31  6:45   ` Archit Taneja
2012-10-31  6:57     ` Archit Taneja
2012-10-31  7:26     ` Tomi Valkeinen
2012-10-31  7:26       ` Tomi Valkeinen
2012-10-31  7:32       ` Archit Taneja
2012-10-31  7:44         ` Archit Taneja
2012-10-30 16:10 ` [PATCH 06/12] OMAPDSS: DSI: workaround for HSDiv problem Tomi Valkeinen
2012-10-30 16:10   ` Tomi Valkeinen
2012-10-30 16:10 ` [PATCH 07/12] OMAPDSS: add dss_calc_clock_rates() back Tomi Valkeinen
2012-10-30 16:10   ` Tomi Valkeinen
2012-10-30 16:10 ` [PATCH 08/12] OMAPDSS: setup default dss fck Tomi Valkeinen
2012-10-30 16:10   ` Tomi Valkeinen
2012-10-31  6:31   ` Archit Taneja
2012-10-31  6:43     ` Archit Taneja
2012-10-31  7:32     ` Tomi Valkeinen
2012-10-31  7:32       ` Tomi Valkeinen
2012-10-30 16:10 ` [PATCH 09/12] OMAPDSS: hide dss_select_dispc_clk_source() Tomi Valkeinen
2012-10-30 16:10   ` Tomi Valkeinen
2012-10-31  6:54   ` Archit Taneja
2012-10-31  6:54     ` Archit Taneja
2012-10-31  7:17     ` Tomi Valkeinen
2012-10-31  7:17       ` Tomi Valkeinen
2012-10-30 16:10 ` [PATCH 10/12] OMAPDSS: DPI: use dpi.dsidev to see whether to use dsi pll Tomi Valkeinen
2012-10-30 16:10   ` Tomi Valkeinen
2012-10-30 16:10 ` [PATCH 11/12] OMAPDSS: DPI: verify if DSI PLL is operational Tomi Valkeinen
2012-10-30 16:10   ` Tomi Valkeinen
2012-10-30 16:10 ` [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available Tomi Valkeinen
2012-10-30 16:10   ` Tomi Valkeinen
2012-10-31  7:26   ` Archit Taneja
2012-10-31  7:38     ` Archit Taneja
2012-11-02 10:08     ` Tomi Valkeinen
2012-11-02 10:08       ` Tomi Valkeinen
2012-11-02 10:44       ` Archit Taneja
2012-11-02 10:56         ` Archit Taneja
2012-11-02 10:49         ` Tomi Valkeinen
2012-11-02 10:49           ` Tomi Valkeinen
2012-11-02 11:09           ` Archit Taneja
2012-11-02 11:21             ` Archit Taneja
2012-11-02 11:28             ` Tomi Valkeinen
2012-11-02 11:28               ` Tomi Valkeinen
2012-11-02 11:56               ` Archit Taneja
2012-11-02 11:56                 ` Archit Taneja
2012-11-05  8:55                 ` Tomi Valkeinen
2012-11-05  8:55                   ` Tomi Valkeinen
2012-11-05 14:21                   ` Rob Clark
2012-11-05 14:21                     ` Rob Clark
2012-11-06 13:41                     ` Tomi Valkeinen
2012-11-06 13:41                       ` Tomi Valkeinen
2012-11-06 14:40                       ` Rob Clark
2012-11-06 14:40                         ` Rob Clark
2012-11-07 10:01                         ` Tomi Valkeinen [this message]
2012-11-07 10:01                           ` Tomi Valkeinen
2012-11-07 14:32                           ` Rob Clark
2012-11-07 14:32                             ` Rob Clark
2012-11-07 15:13                             ` Tomi Valkeinen
2012-11-07 15:13                               ` Tomi Valkeinen
2012-11-07 19:18                               ` Rob Clark
2012-11-07 19:18                                 ` Rob Clark
2012-11-08  7:39                                 ` Tomi Valkeinen
2012-11-08  7:39                                   ` Tomi Valkeinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=509A3171.8010100@ti.com \
    --to=tomi.valkeinen@ti.com \
    --cc=archit@ti.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=rob.clark@linaro.org \
    --cc=tomba@iki.fi \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.