* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Alexander Holler @ 2013-01-29 20:35 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-fbdev, Florian Tobias Schandinat,
Bernie Thompson, Steve Glendinning, stable
In-Reply-To: <5107F014.4030704@ahsoftware.de>
Am 29.01.2013 16:51, schrieb Alexander Holler:
> Am 29.01.2013 12:11, schrieb Alexander Holler:
>
>>
>> To explain the problem on shutdown a bit further, I think the following
>> happens (usb and driver are statically linked and started by the kernel):
>>
>> shutdown -> kill signal -> usb stack shuts down -> udlfb waits (forever)
>> for a kill or an urb which it doesn't get.
>
> Having a second look at what I've written above, I'm not even sure if
> the kernel sends one or more fatal signals on shutdown at all. I've just
> assumed it because otherwise down_interruptible() wouldn't have worked
> before (it would have stalled on shutdown too (if an urb got missed),
> not only on disconnect).
>
> Sounds like an interesting question I should read about (if/when fatal
> signals are issued by the kernel). ;)
>
>> Maybe the sequence is different if the usb-stack and udlfb are used as a
>> module and/or udlfb is used only for X/fb. I'm not sure what actually
>> does shut down the usb-stack in such a case, but maybe more than one
>> kill signal might be thrown around.
If anyone still follows my monologue: The question was interesting
enough that I couldn't resist for long. ;)
(all pasted => format broken)
In drivers/tty/sysrq.c there is
------
static void send_sig_all(int sig)
{
struct task_struct *p;
read_lock(&tasklist_lock);
for_each_process(p) {
if (p->flags & PF_KTHREAD)
continue;
if (is_global_init(p))
continue;
do_send_sig_info(sig, SEND_SIG_FORCED, p, true);
}
read_unlock(&tasklist_lock);
}
static void sysrq_handle_term(int key)
{
send_sig_all(SIGTERM);
console_loglevel = 8;
}
(...)
static void sysrq_handle_kill(int key)
{
send_sig_all(SIGKILL);
console_loglevel = 8;
}
------
Now I've done some learning by doing (kernel 3.7.5 + some patches):
------
diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
index df249f3..db8a86c 100644
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -1876,14 +1876,18 @@ static void dlfb_free_urb_list(struct dlfb_data
*dev)
unsigned long flags;
pr_notice("Freeing all render urbs\n");
+ if (current->flags & PF_KTHREAD)
+ pr_info("AHO: I'm a kernel thread\n");
/* keep waiting and freeing, until we've got 'em all */
while (count--) {
/* Timeout likely occurs at disconnect (resulting in a
leak) */
ret = down_timeout_killable(&dev->urbs.limit_sem,
FREE_URB_TIMEOUT);
- if (ret)
+ if (ret) {
+ pr_info("AHO: ret %d\n", ret);
break;
+ }
spin_lock_irqsave(&dev->urbs.lock, flags);
------
Now I've disconnected the display. And, as send_sig_all() already
suggests, the result was (besides discovering an oops in
call_timer_fn.isra (once)):
------
[ 120.963010] udlfb: AHO: I'm a kernel thread
[ 122.957024] udlfb: AHO: ret -62
------
(-62 is -ETIME)
So, if the above down_timeout_killable() is only down_interruptible(),
as in kernel 3.7.5, the box would not shutdown afterwards, because on
shutdown no signal would be send to that kernel-thread which called
dlfb_free_urb_list().
A last note: dlfb_usb_disconnect() (thus dlfb_free_urb_list()) isn't
called on shutdown if the device would still be connected. So the
problem only might happen, if the screen will be disconnected before
shutdown (and an urb gets missed). So the subject of my patch is correct. ;)
</monologue>
Regards,
Alexander
^ permalink raw reply related
* Re: [Linaro-mm-sig] CDF discussions at FOSDEM
From: Marcus Lorentzon @ 2013-01-29 19:35 UTC (permalink / raw)
To: Daniel Vetter
Cc: Linux Fbdev development list, Syrjala, Ville,
dri-devel@lists.freedesktop.org, Linaro MM SIG, Laurent Pinchart,
Clark, Rob
In-Reply-To: <20130129155040.GR14766@phenom.ffwll.local>
On 01/29/2013 04:50 PM, Daniel Vetter wrote:
> On Tue, Jan 29, 2013 at 3:19 PM, Daniel Vetter<daniel.vetter@ffwll.ch> wrote:
> Ok, in the interest of pre-heating the discussion a bit I've written down
> my thoughts about display slave drivers. Adding a few more people and
> lists to make sure I haven't missed anyone ...
>
> Cheers, Daniel
> --
> Display Slaves
> =======
>
> A highly biased quick analysis from Daniel Vetter.
And here is my biased version as one of the initiators of the idea of CDF.
I work with ARM SoCs (ST-Ericsson) and mobile devices (DSI/DPI panels).
Of course some of these have the "PC" type of encoder devices like HDMI
and eDP or even VGA. But from what I have seen most of these encoders
are used by few different SoCs(GPUs?). And using these type of encoders
was quite straight forward from DRM encoders. My goal was to get some
common code of all the "mobile" panel encoders or "display module driver
IC"s as some call them. Instead of tens of drivers (my assumption) you
now have hundreds of drivers often using MIPI DSI/DPI/DBI or some
similar interface. And lots of new come each year. There are probably
more panel types than there are products on the market, since most
products use more than one type of panel on the same product to secure
sourcing for mass production (note multiple panels use same driver IC).
So that was the initial goal, to cover all of these, which most are
maintained per SoC/CPU out of kernel.org. If HDMI/DP etc fits in this
framework, then that is just a nice bonus.
I just wanted to give my history so we are not trying to include to many
different types of encoders without an actual need. Maybe the I2C drm
stuff is good enough for that type of encoders. But again, it would be
nice with one suit that fits all ...
I also like the idea to start out small. But if no support is added
initially for the mobile panel types. Then I think it will be hard to
get all vendors to start pushing those drivers, because the benefit of
doing so would be small. But maybe the CDF work with Linaro and Laurent
could just be a second step of adding the necessary details to your
really simple baseline. And I also favor the helpers over framework
approach but I miss a big piece which is the ops for panel drivers to
call back to display controller (the video source stuff).
Some inline comments below.
>
> A quick discussion about the issues surrounding some common framework for
> display slaves like panels, hdmi/DP/whatever encoders, ... Since these external
> chips are very often reused accross different SoCs, it would be beneficial to
> share slave driver code between different chipset drivers.
>
> Caveat Emperor!
> ---------------
>
> Current output types and slave encoders already have to deal with a pletoria of
> special cases and strange features. To avoid ending up with something not
> suitable for everyone, we should look at what's all supported already and how we
> could possibly deal with those things:
>
> - audio embedded into the display stream (hdmi/dp). x86 platforms with the HD
> Audio framework rely on ELD and forwarding certain events as interrupts
> through the hw between the display and audio side ...
I would assume any driver handling audio/video/cec like HDMI would hook
itself up as an mfd device. And one of those exposed functions would be
the CDF part. Instead of pushing everything into the "display parts". At
least that is sort of what we do today and it keeps the audio, cec and
display parts nicely separated.
> - hdmi/dp helpers: HDMI/DP are both standardized output connectors with nice
> complexity. DP is mostly about handling dp aux transactions and DPCD
> registers, hdmi mostly about infoframes and how to correctly set them up from
> the mode + edid.
Yes, it is a mess. But we have managed to hide that below a simple panel
API similar to CDF/omap so far.
> - dpms is 4 states in drm, even more in fbdev afaict, but real hw only supports
> on/off nowadays ... how should/do we care?
Agreed, they should all really go away unless someone find a valid use case.
> - Fancy modes and how to represent them. Random list of things we need to
> represent somehow: broadcast/reduced rbg range for hdmi, yuv modes, different
> bpc modes (and handling how this affects bandwidth/clocks, e.g. i915
> auto-dithers to 6bpc on DP if there's not enough), 3D hdmi modes (patches have
> floated on dri-devel for this), overscan compensation. Many of these things
> link in with e.g. the helper libraries for certain outputs, e.g. discovering
> DP sink capabilities or setting up the correct hdmi infoframe.
Are you saying drm modes doesn't support this as of today? I have not
used these types of modes in DRM yet. Maybe the common video mode
patches is a good start.
> - How to expose random madness as properties, e.g. backlight controllers,
> broadcast mode, enable/disable embedded audio (some screens advertise it, but
> don't like it). For additional fun I expect different users of a display slave
> driver to expect different set of "standardized" properties.
Some standardized properties would be nice :). Whatever is not standard
doesn't really matter.
> - Debug support: Register dumping, exposing random debugfs files, tracing.
> Preferably somewhat unified to keep things sane, since most often slave
> drivers are rather simple, but we expect quite a few different ones.
>
> - Random metadata surrounding a display sink, like output type. Or flags for
> support special modes (h/vsync polarity, interlaced/doublescan, pixel
> doubling, ...).
One thing that is needed is all the meta data related to the
control/data interface between display controller and encoder. Because
this has to be unified per interface type like DSI/DBI so the same CDF
driver can setup different display controllers. But I hope we could
split the "CDF API" (panel ops) from the control/data bus API
(host/source ops or CDF video source).
> - mode_fixup: Used a lot in drm-land to allow encoders to change the input mode,
> e.g. for lvds encoders which can do upscaling, or if the encoder supports
> progressive input with interlaced output and similar fancy stuff. See e.g. the
> intel sdvo encoder chip support.
>
> - Handling different control buses like i2c, direct access (not seen that yet),
> DSI, DP aux, some other protocols.
This is actually the place I wanted to start. With vendor specific panel
drivers using common ops to access the bus (DSI/I2C/DBI etc). Then once
we have a couple of panel drivers we could unify the API making them do
their stuff (like the current CDF ops). Or even better, maybe these two
could be made completely separate and worked on in parallel.
> - Handling of different display data standards like dsi (intel invented a few of
> its own, I'm sure we're not the only ones).
>
> - hpd support/polling. Depending upon desing hpd handling needs to be
> cooperative between slave and master, or is a slave only thing (which means
> the slave needs to be able to poke the master when something changes).
> Similarly, masters need to know which slaves require output polling.
I prefer a slave only thing forwarded to the drm encoder which I assume
would be the drm equivalent of the display slave. At least I have not
seen any need to involve the display controller in hpd (which I assume
you mean by master).
> - Initializing of slave drivers: of/devicetree based, compiled-in static tables
> in the driver, dynamic discovery by i2c probing, lookup through some
> platform-specific firmware table (ACPI). Related is how to forward random
> platform init values to the drivers from these sources (e.g. the panel fixed
> modes) to the slave driver.
I'm not that familiar with the bios/uefi world. But on our SoCs we
always have to show a splash screen from the boot loader (like bios,
usually little kernel, uboot etc). And so all probing is done by
bootloader and HW is running when kernel boot. And you are not allowed
to disrupt it either because that would yield visual glitches during
boot. So some way or the other the boot loader would need to transfer
the state to the kernel or you would have to reverse engineer the state
from hw at kernel probe.
> - get_hw_state support. One of the major point in the i915 modeset rewrite which
> landed in 3.7 is that a lot of the hw state can be cross-checked with the sw
> tracking. Helps tremendously in tracking down driver (writer) fumbles ;-)
This sounds more like a display controller feature than a display slave
feature.
> - PSR/dsi command mode and how the start/stop frame dance should be handled.
Again, a vital piece for the many mobile driver ICs. And I think we have
several sources (STE, Renesas, TI, Samsung, ...) on how to do this and
tested in many products. So I hope this could be an early step in the
evolution.
> - Random funny expectations around the modeset sequence, i.e. when (and how
> often) the video stream should be enabled/disabled. In the worst case this
> needs some serious cooperation between master and slaves. Even more fun for
> trained output links like DP where a re-training and so restarting parts - or
> even the complete - modeset sequence could be required to happen any time.
Again, we have several samples of platforms already doing this stuff. So
we should be able to get a draft pretty early. From my experience when
to enable/disable video stream could vary between versions of the same
display controller. So I think it could be pretty hairy to get a single
solution for all. Instead I think we need to leave some room for the
master/slave to decide when to enable/disable. And to be able to do this
we should try to have pretty specific ops on the slave and master. I'm
not sure prepare/modeset/commit is specific enough unless we document
what is expected to be done by the slave in each of these.
>
> - There's more I'm sure, gfx hw tends to be insane ...
Yes, and one is the chain of slaves issue that is "common" on mobile
systems. One example I have is
dispc->dsi->dsi2dsi-bridge->dsi2lvds-bridge->lvds-panel.
My proposal to hide this complexity in CDF was aggregate drivers. So
from drm there will only be one master (dispc) and one slave (dsi2dsi).
Then dsi2dsi will itself use another CDF/slave driver to talk to its
slave. This way the top master (dispc) driver never have to care about
this complexity. Whether this is possible to hide in practice we will
see ...
>
> Wishful Thinking
> ----------------
>
> Ignoring reality, let's look at what the perfect display slave framework should
> achieve to be useful:
>
> - Should be simple to share code between different master drivers - display slave
> drivers tend to be boring assemblies of register definitions and banging the
> right magic values into them. Which also means that we should aim for a high
> level of unification so that using, understanding and debugging drivers is
> easy.
>
> - Since we expect drivers to be simple, even little amounts of
> impedence-matching code can kill the benefits of the shared code. Furthermore
> it should be possible to extend drivers with whatever subset of the above
> feature list is required by the subsystem/driver using a slave driver. Again,
> without incurring unnecessary amounts of impendance matching. Ofc, not all
> users of slave drivers will be able to use all the crazy features.
This is also my fear. Which is why I wanted to start with one slave
interface at a time. And maybe even have different "API"s for differnt
type of panels. Like classic I2C encoders, DSI command mode "smart"
panels, DSI video mode, DPI ... and then do another layer of helpers in
drm encoders. That way a DSI command mode panel wouldn't have to be
forced into the same shell as a I2C HDMI encoder as they are very
different with very little overlap.
> Reality Check
> -------------
>
> We already have tons of different slave encoder frameworks sprinkled all over
> the kernel, which support different sets of crazy features and are used by
> different. Furthermore each subsystem seems to have come up with it's own way to
> describe metadata like display modes, all sorts of type enums, properties,
> helper functions for special output types.
>
> Conclusions:
>
> - Throwing away and rewriting all the existing code seems unwise, but we'll
> likely need tons of existing drivers with the new framework.
>
> - Unifying the metadata handling will be _really_ painful since it's deeply
> ingrained into each driver. Not unifying it otoh will lead to colossal amounts
> of impendance matching code.
>
> - The union of all the slave features used by all the existing frameworks is
> impressive, but also highly non-overlapping. Likely everyone has his own
> utterly "must-have" feature.
>
> Proposal
> --------
>
> I have to admit that I'm not too much in favour of the current CDF. It has a bit
> of midlayer smell to it imo, and looks like it will make many of the mentioned
> corner-case messy to enable. Also looking at things the proposed generic video
> mode structure it seems to lack some features e.g. drm_mode already has. Which
> does not include new insanity like 3d modes or some advanced infoframes stuff.
>
> So instead I'll throw around a few ideas and principles:
>
> - s/framework/helper library/ Yes, I really hate midlayers and just coming up
> with a different name seems to go a long way towards saner apis.
Me like, but I hope you agree to keep calling it CDF until it is merged.
We could call it Common Display Frelpers if you like ;)
> - I think we should reduce the scope of the intial version massively and instead
> increase the depth to fully cover everything. So instead of something which
> covers everything of a limited use-case from discover, setup, modes handling
> and mode-setting, concentrate on only one operation. The actual mode-set seems
> to be the best case, since it usually involves a lot of the boring register
> bashing code. The first interface version would ignore everything else
> completely.
To also cover and be useful to mobile panels I suggest starting with
on/off using a fixed mode initially. Because modeset is not used for
most mobile panels (they only have one mode).
> - Shot for the most powerful api for that little piece we're starting with, make
> it the canonical thing. I.e. for modeset we need a video mode thing, and imo
> it only makes sense if that's the native data structure for all invovled
> subsystems. At least it should be the aim. Yeah, that means tons of work. Even
> more important is that the new datastructure supports every feature already
> support in some insane way in one of the existing subsystems. Imo if we keep
> different datastructures everywhere, the impendance matching will eat up most
> of the code sharing benefits.
>
> - Since converting all invovled subsystems we should imo just forget about
> fbdev. For obvious reasons I'm also leaning towards simply ditching the
> drm prefix from the drm defines and using those ;-)
>
> - I haven't used it in a driver yet, but mandating regmap (might need some
> improvements) should get us decent unification between drivers. And hopefully
> also an easy way to have unified debug tools. regmap already has trace points
> and a few other cool things.
Guideline for I2C slave drivers maybe? Do we really want to enforce how
drivers are implemented when it doesn't affect the API?
Also, I don't think it fits in general for slaves. Since DSI/DBI have
not only registers but also operations you can execute using control
interface.
> - We need some built-in way to drill direct paths from the master display driver
> to the slave driver for the different subsystems. Jumping through hoops (or
> even making it impossible) to extend drivers in funny ways would be a big step
> backwards.
>
> - Locking will be fun, especially once we start to add slave->master callbacks
> (e.g. for stopping/starting the display signal, hpd interrupts, ...). As a
> general rule I think we should aim for no locks in the slave driver, with the
> master owning the slave and ensure exclusion with its own locks. Slaves which
> use shared resources and so need locks (everything doing i2c actually) may not
> call master callback functions with locks held.
Agreed, and I think we should rely on upper layers like drm as much as
possible for locking.
> Then, once we've gotten things of the ground and have some slave encoder drivers
> which are actually shared between different subsystems/drivers/platforms or
> whatever we can start to organically grow more common interfaces. Ime it's much
> easier to simply extract decent interfaces after the fact than trying to come
> up.
>
> Now let's pour this into a more concrete form:
>
> struct display_slave_ops {
> /* modeset ops, e.g. prepare/modset/commit from drm */
> };
>
> struct display_slave {
> struct display_slave_ops *ops;
> void *driver_private;
> };
>
> I think even just that will be worth a lot of flames to come up with a good and
> agreeable interface for everyone. It'll probably satisfactory to no one though.
>
> Then each subsystem adds it's own magic, e.g.
>
> struct drm_encoder_slave {
> struct display_slave slave;
>
> /* everything else which is there already and not covered by the display
> * slave interface. */
> };
I like the starting point. Hard to make it any more simple ;). But next
step would probably follow quickly. I also like the idea to have current
drivers aggregate the slave to make transition easier. CDF as it is now
is an all or nothing API. And since you don't care how slaves interact
with master (bus ops) I see the possibility still to separate "CDI
device API" and "CDF bus API". Which would allow using DSI bus API for
DSI panels and I2C bus API (or regmap) for I2C encoders instead of force
use of the video source API in all slave drivers.
> Other subsystems/drivers like DSS would embed the struct display_slave in their
> own equivalent data-structure.
>
> So now we have the little problem that we want to have one single _slave_ driver
> codebase, but it should be able to support n different interfaces and
> potentially even more ways to be initialized and set up. Here's my idea how this
> could be tackled:
>
> 1. Smash everything into one driver file/directory.
> 2. Use a common driver structure which contains pointers/members for all
> possible use-cases. For each interface the driver supports, it'll allocate the
> same structure and put the pointer into foo->slave.driver_private. This way
> different entry points from different interfaces could use the same internal
> functions since all deal with the same structure.
> 3. Add whatever magic is required to set up the driver for different platforms.
> E.g. and of match, drm_encoder_slave i2c match and some direct function to set
> up hardcoded cases could all live in the same file.
>
> Getting the kernel Kconfig stuff right will be fun, but we should get by with
> adding tons more stub functions. That might mean that an of/devicetree platform
> build carries around a bit of gunk for x86 vbt matching maybe, but imo that
> shouldn't ever get out of hand size-wise.
>
> Once we have a few such shared drivers in place, and even more important,
> unified that part of the subsystem using them a bit, it should be painfully
> obvious which is the next piece to extract into the common display slave library
> interface. After all, they'll live right next to each another in the driver
> sources ;-)
>
> Eventually we should get into the real fun part like dsi bus support or command
> mode/PSR ... Those advanced things probably need to be optional.
>
> But imo the key part is that we aim for real unification in the users of
> display_slave's, so internally convert over everything to the new structures.
> That should also make code-sharing much easier, so that we could move existing
> helper functions to the common display helper library.
What about drivers that are waiting for CDF to be pushed upstream
instead of having to push another custom panel framework? I'm talking of
my own KMS driver ... but maybe I could put most of it in staging and
move relevant parts of DSI/DPI/HDMI panel drivers to "common" slave
drivers ...
> Bikesheds
> ---------
>
> I.e. the boring details:
>
> - Where to put slave drivers? I'll vote for anything which does not include
> drivers/video ;-)
drivers/video +1, drivers/gpu -1, who came up with putting KMS under
drivers/gpu ;)
> - Maybe we want to start with a different part than modeset, or add a bit more
> on top. Though I really think we should start minimally and modesetting seemed
> like the most useful piece of the puzzle.
As suggested, start with on/off and static/fixed mode would help single
resolution LCDs. Actually that is almost all that is needed for mobile
panels and what I intended to get from CDF :)
>
> - Naming the new interfaces. I'll have more asbestos suites on order ...
Until you get them. Would it make sense to reuse the encoder name from
drm or is that to restrictive?
>
> - Can we just copy the new "native" interface structs from drm, pls?
I hope you are not talking about the helper interfaces at least ;). But
if CDF is going to be the new drm helpers of choice for
encoder/connector parts. Then it sounds like CDF would replace most of
the old helpers. It would be far to many layers with the old helpers
too. And I think I recall Jesse wanting to deprecate/remove them too.
Hopefully we could have some generic encoder/connector helper
implementations that only depend on CDF.
/BR
/Marcus
^ permalink raw reply
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Alexander Holler @ 2013-01-29 15:51 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-fbdev, Florian Tobias Schandinat,
Bernie Thompson, Steve Glendinning, stable
In-Reply-To: <5107AE4F.9000809@ahsoftware.de>
Am 29.01.2013 12:11, schrieb Alexander Holler:
>
> To explain the problem on shutdown a bit further, I think the following
> happens (usb and driver are statically linked and started by the kernel):
>
> shutdown -> kill signal -> usb stack shuts down -> udlfb waits (forever)
> for a kill or an urb which it doesn't get.
Having a second look at what I've written above, I'm not even sure if
the kernel sends one or more fatal signals on shutdown at all. I've just
assumed it because otherwise down_interruptible() wouldn't have worked
before (it would have stalled on shutdown too (if an urb got missed),
not only on disconnect).
Sounds like an interesting question I should read about (if/when fatal
signals are issued by the kernel). ;)
> Maybe the sequence is different if the usb-stack and udlfb are used as a
> module and/or udlfb is used only for X/fb. I'm not sure what actually
> does shut down the usb-stack in such a case, but maybe more than one
> kill signal might be thrown around.
Regards,
Alexander
^ permalink raw reply
* Re: [Linaro-mm-sig] CDF discussions at FOSDEM
From: Daniel Vetter @ 2013-01-29 15:50 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Linux Fbdev development list, Syrjala, Ville, dri-devel,
Linaro MM SIG, Clark, Rob
In-Reply-To: <CAKMK7uEJ+gyX1F=FqSEjmKYgqrrCUM3LgYZLCVWyq6eimWfGsQ@mail.gmail.com>
On Tue, Jan 29, 2013 at 3:19 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Tue, Jan 29, 2013 at 1:11 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>> DevRooms are also not supposed to open before 11:00 (which is already a
>>> massive improvement over 2011 and the years before, where i was happy
>>> to be able to put the cabling in at 12:00), and i tend to first get a
>>> nod of approval from the on-site devrooms supervisor before i go in and
>>> set up the room.
>>>
>>> So use the hackingroom this year. Things will hopefully be better next
>>> year.
>>
>> Saturday is pretty much out of question, given that most developers interested
>> in CDF will want to attend the X.org talks. I'll try to get a room for Sunday
>> then, but I'm not sure yet what time slots will be available. It would be
>> helpful if people interested in CDF discussions could tell me at what time
>> they plan to leave Brussels on Sunday.
>
> I'll stay till Monday early morning, so requirements from me. Adding a
> bunch of Intel guys who're interested, too.
Ok, in the interest of pre-heating the discussion a bit I've written down
my thoughts about display slave drivers. Adding a few more people and
lists to make sure I haven't missed anyone ...
Cheers, Daniel
--
Display Slaves
=======
A highly biased quick analysis from Daniel Vetter.
A quick discussion about the issues surrounding some common framework for
display slaves like panels, hdmi/DP/whatever encoders, ... Since these external
chips are very often reused accross different SoCs, it would be beneficial to
share slave driver code between different chipset drivers.
Caveat Emperor!
---------------
Current output types and slave encoders already have to deal with a pletoria of
special cases and strange features. To avoid ending up with something not
suitable for everyone, we should look at what's all supported already and how we
could possibly deal with those things:
- audio embedded into the display stream (hdmi/dp). x86 platforms with the HD
Audio framework rely on ELD and forwarding certain events as interrupts
through the hw between the display and audio side ...
- hdmi/dp helpers: HDMI/DP are both standardized output connectors with nice
complexity. DP is mostly about handling dp aux transactions and DPCD
registers, hdmi mostly about infoframes and how to correctly set them up from
the mode + edid.
- dpms is 4 states in drm, even more in fbdev afaict, but real hw only supports
on/off nowadays ... how should/do we care?
- Fancy modes and how to represent them. Random list of things we need to
represent somehow: broadcast/reduced rbg range for hdmi, yuv modes, different
bpc modes (and handling how this affects bandwidth/clocks, e.g. i915
auto-dithers to 6bpc on DP if there's not enough), 3D hdmi modes (patches have
floated on dri-devel for this), overscan compensation. Many of these things
link in with e.g. the helper libraries for certain outputs, e.g. discovering
DP sink capabilities or setting up the correct hdmi infoframe.
- How to expose random madness as properties, e.g. backlight controllers,
broadcast mode, enable/disable embedded audio (some screens advertise it, but
don't like it). For additional fun I expect different users of a display slave
driver to expect different set of "standardized" properties.
- Debug support: Register dumping, exposing random debugfs files, tracing.
Preferably somewhat unified to keep things sane, since most often slave
drivers are rather simple, but we expect quite a few different ones.
- Random metadata surrounding a display sink, like output type. Or flags for
support special modes (h/vsync polarity, interlaced/doublescan, pixel
doubling, ...).
- mode_fixup: Used a lot in drm-land to allow encoders to change the input mode,
e.g. for lvds encoders which can do upscaling, or if the encoder supports
progressive input with interlaced output and similar fancy stuff. See e.g. the
intel sdvo encoder chip support.
- Handling different control buses like i2c, direct access (not seen that yet),
DSI, DP aux, some other protocols.
- Handling of different display data standards like dsi (intel invented a few of
its own, I'm sure we're not the only ones).
- hpd support/polling. Depending upon desing hpd handling needs to be
cooperative between slave and master, or is a slave only thing (which means
the slave needs to be able to poke the master when something changes).
Similarly, masters need to know which slaves require output polling.
- Initializing of slave drivers: of/devicetree based, compiled-in static tables
in the driver, dynamic discovery by i2c probing, lookup through some
platform-specific firmware table (ACPI). Related is how to forward random
platform init values to the drivers from these sources (e.g. the panel fixed
modes) to the slave driver.
- get_hw_state support. One of the major point in the i915 modeset rewrite which
landed in 3.7 is that a lot of the hw state can be cross-checked with the sw
tracking. Helps tremendously in tracking down driver (writer) fumbles ;-)
- PSR/dsi command mode and how the start/stop frame dance should be handled.
- Random funny expectations around the modeset sequence, i.e. when (and how
often) the video stream should be enabled/disabled. In the worst case this
needs some serious cooperation between master and slaves. Even more fun for
trained output links like DP where a re-training and so restarting parts - or
even the complete - modeset sequence could be required to happen any time.
- There's more I'm sure, gfx hw tends to be insane ...
Wishful Thinking
----------------
Ignoring reality, let's look at what the perfect display slave framework should
achieve to be useful:
- Should be simple to share code between different master drivers - display slave
drivers tend to be boring assemblies of register definitions and banging the
right magic values into them. Which also means that we should aim for a high
level of unification so that using, understanding and debugging drivers is
easy.
- Since we expect drivers to be simple, even little amounts of
impedence-matching code can kill the benefits of the shared code. Furthermore
it should be possible to extend drivers with whatever subset of the above
feature list is required by the subsystem/driver using a slave driver. Again,
without incurring unnecessary amounts of impendance matching. Ofc, not all
users of slave drivers will be able to use all the crazy features.
Reality Check
-------------
We already have tons of different slave encoder frameworks sprinkled all over
the kernel, which support different sets of crazy features and are used by
different. Furthermore each subsystem seems to have come up with it's own way to
describe metadata like display modes, all sorts of type enums, properties,
helper functions for special output types.
Conclusions:
- Throwing away and rewriting all the existing code seems unwise, but we'll
likely need tons of existing drivers with the new framework.
- Unifying the metadata handling will be _really_ painful since it's deeply
ingrained into each driver. Not unifying it otoh will lead to colossal amounts
of impendance matching code.
- The union of all the slave features used by all the existing frameworks is
impressive, but also highly non-overlapping. Likely everyone has his own
utterly "must-have" feature.
Proposal
--------
I have to admit that I'm not too much in favour of the current CDF. It has a bit
of midlayer smell to it imo, and looks like it will make many of the mentioned
corner-case messy to enable. Also looking at things the proposed generic video
mode structure it seems to lack some features e.g. drm_mode already has. Which
does not include new insanity like 3d modes or some advanced infoframes stuff.
So instead I'll throw around a few ideas and principles:
- s/framework/helper library/ Yes, I really hate midlayers and just coming up
with a different name seems to go a long way towards saner apis.
- I think we should reduce the scope of the intial version massively and instead
increase the depth to fully cover everything. So instead of something which
covers everything of a limited use-case from discover, setup, modes handling
and mode-setting, concentrate on only one operation. The actual mode-set seems
to be the best case, since it usually involves a lot of the boring register
bashing code. The first interface version would ignore everything else
completely.
- Shot for the most powerful api for that little piece we're starting with, make
it the canonical thing. I.e. for modeset we need a video mode thing, and imo
it only makes sense if that's the native data structure for all invovled
subsystems. At least it should be the aim. Yeah, that means tons of work. Even
more important is that the new datastructure supports every feature already
support in some insane way in one of the existing subsystems. Imo if we keep
different datastructures everywhere, the impendance matching will eat up most
of the code sharing benefits.
- Since converting all invovled subsystems we should imo just forget about
fbdev. For obvious reasons I'm also leaning towards simply ditching the
drm prefix from the drm defines and using those ;-)
- I haven't used it in a driver yet, but mandating regmap (might need some
improvements) should get us decent unification between drivers. And hopefully
also an easy way to have unified debug tools. regmap already has trace points
and a few other cool things.
- We need some built-in way to drill direct paths from the master display driver
to the slave driver for the different subsystems. Jumping through hoops (or
even making it impossible) to extend drivers in funny ways would be a big step
backwards.
- Locking will be fun, especially once we start to add slave->master callbacks
(e.g. for stopping/starting the display signal, hpd interrupts, ...). As a
general rule I think we should aim for no locks in the slave driver, with the
master owning the slave and ensure exclusion with its own locks. Slaves which
use shared resources and so need locks (everything doing i2c actually) may not
call master callback functions with locks held.
Then, once we've gotten things of the ground and have some slave encoder drivers
which are actually shared between different subsystems/drivers/platforms or
whatever we can start to organically grow more common interfaces. Ime it's much
easier to simply extract decent interfaces after the fact than trying to come
up.
Now let's pour this into a more concrete form:
struct display_slave_ops {
/* modeset ops, e.g. prepare/modset/commit from drm */
};
struct display_slave {
struct display_slave_ops *ops;
void *driver_private;
};
I think even just that will be worth a lot of flames to come up with a good and
agreeable interface for everyone. It'll probably satisfactory to no one though.
Then each subsystem adds it's own magic, e.g.
struct drm_encoder_slave {
struct display_slave slave;
/* everything else which is there already and not covered by the display
* slave interface. */
};
Other subsystems/drivers like DSS would embed the struct display_slave in their
own equivalent data-structure.
So now we have the little problem that we want to have one single _slave_ driver
codebase, but it should be able to support n different interfaces and
potentially even more ways to be initialized and set up. Here's my idea how this
could be tackled:
1. Smash everything into one driver file/directory.
2. Use a common driver structure which contains pointers/members for all
possible use-cases. For each interface the driver supports, it'll allocate the
same structure and put the pointer into foo->slave.driver_private. This way
different entry points from different interfaces could use the same internal
functions since all deal with the same structure.
3. Add whatever magic is required to set up the driver for different platforms.
E.g. and of match, drm_encoder_slave i2c match and some direct function to set
up hardcoded cases could all live in the same file.
Getting the kernel Kconfig stuff right will be fun, but we should get by with
adding tons more stub functions. That might mean that an of/devicetree platform
build carries around a bit of gunk for x86 vbt matching maybe, but imo that
shouldn't ever get out of hand size-wise.
Once we have a few such shared drivers in place, and even more important,
unified that part of the subsystem using them a bit, it should be painfully
obvious which is the next piece to extract into the common display slave library
interface. After all, they'll live right next to each another in the driver
sources ;-)
Eventually we should get into the real fun part like dsi bus support or command
mode/PSR ... Those advanced things probably need to be optional.
But imo the key part is that we aim for real unification in the users of
display_slave's, so internally convert over everything to the new structures.
That should also make code-sharing much easier, so that we could move existing
helper functions to the common display helper library.
Bikesheds
---------
I.e. the boring details:
- Where to put slave drivers? I'll vote for anything which does not include
drivers/video ;-)
- Maybe we want to start with a different part than modeset, or add a bit more
on top. Though I really think we should start minimally and modesetting seemed
like the most useful piece of the puzzle.
- Naming the new interfaces. I'll have more asbestos suites on order ...
- Can we just copy the new "native" interface structs from drm, pls?
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH 1/3] atmel_lcdfb: fix 16-bpp modes on older SOCs
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-01-29 13:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1355142530-10366-2-git-send-email-jhovold@gmail.com>
On 13:28 Mon 10 Dec , Johan Hovold wrote:
> Fix regression introduced by commit 787f9fd23283 ("atmel_lcdfb: support
> 16bit BGR:565 mode, remove unsupported 15bit modes") which broke 16-bpp
> modes for older SOCs which use IBGR:555 (msb is intensity) rather
> than BGR:565.
>
> Use SOC-type to determine the pixel layout.
>
> Tested on custom at91sam9263-board.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
> drivers/video/atmel_lcdfb.c | 22 +++++++++++++++-------
> include/video/atmel_lcdc.h | 1 +
> 2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
> index 1505539..1f68fa6 100644
> --- a/drivers/video/atmel_lcdfb.c
> +++ b/drivers/video/atmel_lcdfb.c
> @@ -422,17 +422,22 @@ static int atmel_lcdfb_check_var(struct fb_var_screeninfo *var,
> = var->bits_per_pixel;
> break;
> case 16:
> + /* Older SOCs use IBGR:555 rather than BGR:565. */
> + if (sinfo->have_intensity_bit)
> + var->green.length = 5;
> + else
> + var->green.length = 6;
> +
> if (sinfo->lcd_wiring_mode = ATMEL_LCDC_WIRING_RGB) {
> - /* RGB:565 mode */
> - var->red.offset = 11;
> + /* RGB:5X5 mode */
> + var->red.offset = var->green.length + 5;
> var->blue.offset = 0;
> } else {
> - /* BGR:565 mode */
> + /* BGR:5X5 mode */
> var->red.offset = 0;
> - var->blue.offset = 11;
> + var->blue.offset = var->green.length + 5;
> }
> var->green.offset = 5;
> - var->green.length = 6;
> var->red.length = var->blue.length = 5;
> break;
> case 32:
> @@ -679,8 +684,7 @@ static int atmel_lcdfb_setcolreg(unsigned int regno, unsigned int red,
>
> case FB_VISUAL_PSEUDOCOLOR:
> if (regno < 256) {
> - if (cpu_is_at91sam9261() || cpu_is_at91sam9263()
> - || cpu_is_at91sam9rl()) {
> + if (sinfo->have_intensity_bit) {
> /* old style I+BGR:555 */
> val = ((red >> 11) & 0x001f);
> val |= ((green >> 6) & 0x03e0);
> @@ -870,6 +874,10 @@ static int __init atmel_lcdfb_probe(struct platform_device *pdev)
> }
> sinfo->info = info;
> sinfo->pdev = pdev;
> + if (cpu_is_at91sam9261() || cpu_is_at91sam9263() ||
> + cpu_is_at91sam9rl()) {
> + sinfo->have_intensity_bit = true;
> + }
nack
you need to drop the cpu_is as this can only be use now for the core
use platform_device_id to indetify the IP as done on at91-i2c as we can not
detect the IP version
Best Regards,
J.
>
> strcpy(info->fix.id, sinfo->pdev->name);
> info->flags = ATMEL_LCDFB_FBINFO_DEFAULT;
> diff --git a/include/video/atmel_lcdc.h b/include/video/atmel_lcdc.h
> index 28447f1..5f0e234 100644
> --- a/include/video/atmel_lcdc.h
> +++ b/include/video/atmel_lcdc.h
> @@ -62,6 +62,7 @@ struct atmel_lcdfb_info {
> void (*atmel_lcdfb_power_control)(int on);
> struct fb_monspecs *default_monspecs;
> u32 pseudo_palette[16];
> + bool have_intensity_bit;
> };
>
> #define ATMEL_LCDC_DMABADDR1 0x00
> --
> 1.8.0
>
^ permalink raw reply
* Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case
From: Thierry Reding @ 2013-01-29 12:30 UTC (permalink / raw)
To: Peter Ujfalusi
Cc: Richard Purdie, Grant Likely, Rob Landley,
Florian Tobias Schandinat, Andrew Morton, devicetree-discuss,
linux-doc, linux-kernel, linux-fbdev
In-Reply-To: <5107BC2B.5080400@ti.com>
[-- Attachment #1: Type: text/plain, Size: 5076 bytes --]
On Tue, Jan 29, 2013 at 01:10:19PM +0100, Peter Ujfalusi wrote:
> On 01/29/2013 11:17 AM, Thierry Reding wrote:
> > On Tue, Jan 29, 2013 at 09:17:04AM +0100, Peter Ujfalusi wrote:
> >> On 01/28/2013 10:01 PM, Thierry Reding wrote:
> >>> On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote:
> >>>> It is expected that board files would have:
> >>>> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, };
> >>>>
> >>>> static struct platform_pwm_backlight_data bl_data = {
> >>>> .levels = bl_levels,
> >>>> .max_brightness = ARRAY_SIZE(bl_levels),
> >>>> .dft_brightness = 4,
> >>>> .pwm_period_ns = 7812500,
> >>>> };
> >>>>
> >>>> In this case the max_brightness would be out of range in the levels array.
> >>>> Decrement the received max_brightness in every case (DT or non DT) when the
> >>>> levels has been provided.
> >>>
> >>> What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1
> >>> instead?
> >>
> >> There is nothing wrong with that either but IMHO it is more natural for board
> >> files to use just ARRAY_SIZE(bl_levels). In this way the handling of
> >> data->max_brightness among non DT and DT booted kernel is more uniform in the
> >> driver itself.
> >
> > The .max_brightness field is defined to be the maximum value that you
> > can set the brightness to. If you use .levels and set .max_brightness to
> > ARRAY_SIZE() then that's no longer true because the maximum value for
> > the brightness will actually be ARRAY_SIZE() - 1.
>
> Yes, in conjunction with .levels it would be better to have .nr_levels instead
> reusing the max_brightness.
>
> > The reason why in the DT case we decrement .max_brightness is that it is
> > used as a temporary variable to keep the number of levels, which
> > corresponds to ARRAY_SIZE() in your example, and adjust it later on to
> > match the definition. That's an implementation detail.
> >
> > Platform data content should be encoded properly without knowledge of
> > the internal implementation.
> >
> >> Right now all board files are using only the .max_brightness to specify the
> >> maximum value, I could not find any users of .levels in the kernel.
> >
> > Yes. But this is the legacy mode which should be considered deprecated.
> > The reason why the concept of brightness-levels was introduced back when
> > the DT bindings were created was that pretty much no hardware in
> > existence actually works that way. This was a topic of discussion at the
> > time when I first proposed these changes, see for example:
> >
> > http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09472.html
>
> Right. Now I know the background.
> However I do not agree with the conclusion. Probably it is fine in some cases
> to limit the number of configurable duty cycles to have only distinct steps.
> To not go too far, on my laptop I have:
> # cat /sys/class/backlight/acpi_video0/max_brightness
> 15
> # cat /sys/class/backlight/intel_backlight/max_brightness
> 93
FWIW, I have hardware with an Intel chipset that has max_brightness
13666. That doesn't mean all of these are necessarily valid or useful.
> In this case I would be more happier if the user space would use the
> intel_backlight than the acpi_video0. I'm fine if user space decides to allow
> me only 15 distinct steps on the intel_backlight but I would expect it to do
> so in a way when I change the brightness in the UI it would step down or up to
> the next user configurable level. Right now it uses the acpi_video0 to control
> the levels which means that I have (ugly) jumps in the backlight every time I
> adjust it.
>
> In my phone and tablet all transitions between backlight levels are smooth.
> This is not a case in my laptop (with exception when the backlight is set to
> auto, FW controlled).
>
> The perceived brightness change depends on the surrounding environment, you
> can not say that in high level you would need less steps or you need to have
> less steps in low brightness. If you in a dark room the low changes can be
> observed, while the same change when you are outside in a sunny day would not
> reflect the same.
>
> Sure we could do the ramps in driver, but what are the parameters? how many
> step between the two level? What is the time between the steps?
>
> If you are about to make a product you will end up specifying all the possible
> steps to provide the best user experience. So if the PWM have 127 duty cycle
> you will have levels from 0, 1, 2, 3, ...., 125, 126, 127.
That's not true. The duty-cycle merely defines a percentage of how long
the PWM signal will be high. You can choose an arbitrary number of
subdivisions.
Since the brightness of a display isn't linearly proportional to the
duty cycle of the PWM driving it, representing that brightness by a
linear range is misleading. Furthermore some panels have regions where
the backlight isn't lit at all or where changes in the PWM duty-cycle
don't make any difference.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case
From: Peter Ujfalusi @ 2013-01-29 12:10 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, Florian Tobias Schandinat,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Richard Purdie,
Andrew Morton
In-Reply-To: <20130129101709.GC16746-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org>
On 01/29/2013 11:17 AM, Thierry Reding wrote:
> On Tue, Jan 29, 2013 at 09:17:04AM +0100, Peter Ujfalusi wrote:
>> On 01/28/2013 10:01 PM, Thierry Reding wrote:
>>> On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote:
>>>> It is expected that board files would have:
>>>> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, };
>>>>
>>>> static struct platform_pwm_backlight_data bl_data = {
>>>> .levels = bl_levels,
>>>> .max_brightness = ARRAY_SIZE(bl_levels),
>>>> .dft_brightness = 4,
>>>> .pwm_period_ns = 7812500,
>>>> };
>>>>
>>>> In this case the max_brightness would be out of range in the levels array.
>>>> Decrement the received max_brightness in every case (DT or non DT) when the
>>>> levels has been provided.
>>>
>>> What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1
>>> instead?
>>
>> There is nothing wrong with that either but IMHO it is more natural for board
>> files to use just ARRAY_SIZE(bl_levels). In this way the handling of
>> data->max_brightness among non DT and DT booted kernel is more uniform in the
>> driver itself.
>
> The .max_brightness field is defined to be the maximum value that you
> can set the brightness to. If you use .levels and set .max_brightness to
> ARRAY_SIZE() then that's no longer true because the maximum value for
> the brightness will actually be ARRAY_SIZE() - 1.
Yes, in conjunction with .levels it would be better to have .nr_levels instead
reusing the max_brightness.
> The reason why in the DT case we decrement .max_brightness is that it is
> used as a temporary variable to keep the number of levels, which
> corresponds to ARRAY_SIZE() in your example, and adjust it later on to
> match the definition. That's an implementation detail.
>
> Platform data content should be encoded properly without knowledge of
> the internal implementation.
>
>> Right now all board files are using only the .max_brightness to specify the
>> maximum value, I could not find any users of .levels in the kernel.
>
> Yes. But this is the legacy mode which should be considered deprecated.
> The reason why the concept of brightness-levels was introduced back when
> the DT bindings were created was that pretty much no hardware in
> existence actually works that way. This was a topic of discussion at the
> time when I first proposed these changes, see for example:
>
> http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09472.html
Right. Now I know the background.
However I do not agree with the conclusion. Probably it is fine in some cases
to limit the number of configurable duty cycles to have only distinct steps.
To not go too far, on my laptop I have:
# cat /sys/class/backlight/acpi_video0/max_brightness
15
# cat /sys/class/backlight/intel_backlight/max_brightness
93
In this case I would be more happier if the user space would use the
intel_backlight than the acpi_video0. I'm fine if user space decides to allow
me only 15 distinct steps on the intel_backlight but I would expect it to do
so in a way when I change the brightness in the UI it would step down or up to
the next user configurable level. Right now it uses the acpi_video0 to control
the levels which means that I have (ugly) jumps in the backlight every time I
adjust it.
In my phone and tablet all transitions between backlight levels are smooth.
This is not a case in my laptop (with exception when the backlight is set to
auto, FW controlled).
The perceived brightness change depends on the surrounding environment, you
can not say that in high level you would need less steps or you need to have
less steps in low brightness. If you in a dark room the low changes can be
observed, while the same change when you are outside in a sunny day would not
reflect the same.
Sure we could do the ramps in driver, but what are the parameters? how many
step between the two level? What is the time between the steps?
If you are about to make a product you will end up specifying all the possible
steps to provide the best user experience. So if the PWM have 127 duty cycle
you will have levels from 0, 1, 2, 3, ...., 125, 126, 127.
--
Péter
^ permalink raw reply
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Alexander Holler @ 2013-01-29 11:11 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-fbdev, Florian Tobias Schandinat,
Bernie Thompson, Steve Glendinning, stable
In-Reply-To: <5107A5ED.7020009@ahsoftware.de>
Am 29.01.2013 11:35, schrieb Alexander Holler:
> Am 29.01.2013 01:56, schrieb Alexander Holler:
>> Am 29.01.2013 01:22, schrieb Andrew Morton:
>>> On Fri, 25 Jan 2013 19:49:27 +0100
>>> Alexander Holler <holler@ahsoftware.de> wrote:
>>>
>>>> When a device was disconnected the driver may hang at waiting for
>>>> urbs it never
>>>> will get. Fix this by using a timeout while waiting for the used
>>>> semaphore.
>>>>
>>>> There is still a memory leak if a timeout happens, but at least the
>>>> driver
>>>> now continues his disconnect routine.
>>>>
>>>> ...
>>>>
>>>> --- a/drivers/video/udlfb.c
>>>> +++ b/drivers/video/udlfb.c
>>>> @@ -1832,8 +1832,9 @@ static void dlfb_free_urb_list(struct
>>>> dlfb_data *dev)
>>>> /* keep waiting and freeing, until we've got 'em all */
>>>> while (count--) {
>>>>
>>>> - /* Getting interrupted means a leak, but ok at disconnect */
>>>> - ret = down_interruptible(&dev->urbs.limit_sem);
>>>> + /* Timeout likely occurs at disconnect (resulting in a
>>>> leak) */
>>>> + ret = down_timeout_killable(&dev->urbs.limit_sem,
>>>> + FREE_URB_TIMEOUT);
>>>> if (ret)
>>>> break;
>>>
>>> This is rather a hack. Do you have an understanding of the underlying
>>> bug? Why is the driver waiting for things which will never happen?
>
> To add a bit more explanation:
>
> I've experienced that bug after moving the fb-damage-handling into a
> workqueue (to make the driver usable as console). This likely has
> increased the possibility that an urb gets missed when the usb-stack
> calls the (usb-)disconnect function of the driver. But I don't know as I
> couldn't use the driver before (as fbcon) so I don't really have a
> comparison.
>
> What currently happens here is something like that:
>
> fb -> damage -> workload which sends urb and waits for answer
> device disconnect -> dlfb_usb_disconnect() -> stall (no answer to the
> above urb)
>
> I don't know why the disconnect waits for all urbs. The code looks like
> it does that just to free the allocated memory. As I'm not very familiar
> with the usb-stack, I would have to read up about the urb-handling to
> find out how to free the memory otherwise.
>
> As the previous comment in the code suggests that urbs already got
> missed (on shutdown) before, I assume that even without my patch, which
> moved the damage into a workqueue, the problem could occur which then
> prevents a shutdown as there is no timeout. As I've experienced that
> problem not only on disconnect, but on shutdown too (no shutdown was
> possible), I have to assume, that the previous used down_interruptible()
> didn't get a signal on shutdown (if the driver is used as fbcon),
> therefor I consider the timeout as necessary.
To explain the problem on shutdown a bit further, I think the following
happens (usb and driver are statically linked and started by the kernel):
shutdown -> kill signal -> usb stack shuts down -> udlfb waits (forever)
for a kill or an urb which it doesn't get.
Maybe the sequence is different if the usb-stack and udlfb are used as a
module and/or udlfb is used only for X/fb. I'm not sure what actually
does shut down the usb-stack in such a case, but maybe more than one
kill signal might be thrown around.
Regards,
Alexander
^ permalink raw reply
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Alexander Holler @ 2013-01-29 10:35 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-fbdev, Florian Tobias Schandinat,
Bernie Thompson, Steve Glendinning, stable
In-Reply-To: <51071E21.9030008@ahsoftware.de>
Am 29.01.2013 01:56, schrieb Alexander Holler:
> Am 29.01.2013 01:22, schrieb Andrew Morton:
>> On Fri, 25 Jan 2013 19:49:27 +0100
>> Alexander Holler <holler@ahsoftware.de> wrote:
>>
>>> When a device was disconnected the driver may hang at waiting for urbs it never
>>> will get. Fix this by using a timeout while waiting for the used semaphore.
>>>
>>> There is still a memory leak if a timeout happens, but at least the driver
>>> now continues his disconnect routine.
>>>
>>> ...
>>>
>>> --- a/drivers/video/udlfb.c
>>> +++ b/drivers/video/udlfb.c
>>> @@ -1832,8 +1832,9 @@ static void dlfb_free_urb_list(struct dlfb_data *dev)
>>> /* keep waiting and freeing, until we've got 'em all */
>>> while (count--) {
>>>
>>> - /* Getting interrupted means a leak, but ok at disconnect */
>>> - ret = down_interruptible(&dev->urbs.limit_sem);
>>> + /* Timeout likely occurs at disconnect (resulting in a leak) */
>>> + ret = down_timeout_killable(&dev->urbs.limit_sem,
>>> + FREE_URB_TIMEOUT);
>>> if (ret)
>>> break;
>>
>> This is rather a hack. Do you have an understanding of the underlying
>> bug? Why is the driver waiting for things which will never happen?
To add a bit more explanation:
I've experienced that bug after moving the fb-damage-handling into a
workqueue (to make the driver usable as console). This likely has
increased the possibility that an urb gets missed when the usb-stack
calls the (usb-)disconnect function of the driver. But I don't know as I
couldn't use the driver before (as fbcon) so I don't really have a
comparison.
What currently happens here is something like that:
fb -> damage -> workload which sends urb and waits for answer
device disconnect -> dlfb_usb_disconnect() -> stall (no answer to the
above urb)
I don't know why the disconnect waits for all urbs. The code looks like
it does that just to free the allocated memory. As I'm not very familiar
with the usb-stack, I would have to read up about the urb-handling to
find out how to free the memory otherwise.
As the previous comment in the code suggests that urbs already got
missed (on shutdown) before, I assume that even without my patch, which
moved the damage into a workqueue, the problem could occur which then
prevents a shutdown as there is no timeout. As I've experienced that
problem not only on disconnect, but on shutdown too (no shutdown was
possible), I have to assume, that the previous used down_interruptible()
didn't get a signal on shutdown (if the driver is used as fbcon),
therefor I consider the timeout as necessary.
Regards,
Alexander
^ permalink raw reply
* Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case
From: Thierry Reding @ 2013-01-29 10:17 UTC (permalink / raw)
To: Peter Ujfalusi
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, Florian Tobias Schandinat,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Richard Purdie,
Andrew Morton
In-Reply-To: <51078580.2000808-l0cyMroinI0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]
On Tue, Jan 29, 2013 at 09:17:04AM +0100, Peter Ujfalusi wrote:
> On 01/28/2013 10:01 PM, Thierry Reding wrote:
> > On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote:
> >> It is expected that board files would have:
> >> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, };
> >>
> >> static struct platform_pwm_backlight_data bl_data = {
> >> .levels = bl_levels,
> >> .max_brightness = ARRAY_SIZE(bl_levels),
> >> .dft_brightness = 4,
> >> .pwm_period_ns = 7812500,
> >> };
> >>
> >> In this case the max_brightness would be out of range in the levels array.
> >> Decrement the received max_brightness in every case (DT or non DT) when the
> >> levels has been provided.
> >
> > What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1
> > instead?
>
> There is nothing wrong with that either but IMHO it is more natural for board
> files to use just ARRAY_SIZE(bl_levels). In this way the handling of
> data->max_brightness among non DT and DT booted kernel is more uniform in the
> driver itself.
The .max_brightness field is defined to be the maximum value that you
can set the brightness to. If you use .levels and set .max_brightness to
ARRAY_SIZE() then that's no longer true because the maximum value for
the brightness will actually be ARRAY_SIZE() - 1.
The reason why in the DT case we decrement .max_brightness is that it is
used as a temporary variable to keep the number of levels, which
corresponds to ARRAY_SIZE() in your example, and adjust it later on to
match the definition. That's an implementation detail.
Platform data content should be encoded properly without knowledge of
the internal implementation.
> Right now all board files are using only the .max_brightness to specify the
> maximum value, I could not find any users of .levels in the kernel.
Yes. But this is the legacy mode which should be considered deprecated.
The reason why the concept of brightness-levels was introduced back when
the DT bindings were created was that pretty much no hardware in
existence actually works that way. This was a topic of discussion at the
time when I first proposed these changes, see for example:
http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg09472.html
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4] pwm_backlight: Add support for the whole range of the PWM in DT mode
From: Thierry Reding @ 2013-01-29 10:00 UTC (permalink / raw)
To: Peter Ujfalusi
Cc: Richard Purdie, Grant Likely, Rob Landley,
Florian Tobias Schandinat, Andrew Morton, devicetree-discuss,
linux-doc, linux-kernel, linux-fbdev
In-Reply-To: <1358861996-27194-5-git-send-email-peter.ujfalusi@ti.com>
[-- Attachment #1: Type: text/plain, Size: 977 bytes --]
On Tue, Jan 22, 2013 at 02:39:56PM +0100, Peter Ujfalusi wrote:
> When booting with DT make it possible to use the whole range of the PWM when
> controlling the backlight in a same way it is possible when the kernel is
> booted in non DT mode.
> A new property "max-brightness-level" can be used to specify the maximum
> value the PWM can handle (time slots).
> DTS files can use either the "brightness-levels" or the "max-brightness-level"
> to configure the PWM.
> In case both of these properties exist the driver will prefer the
> "brightness-levels" over the "max-brightness-level".
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
I don't think this is a good idea. The brightness-levels property was
specifically introduced in order to have a more reasonable interface to
specify brightness levels.
As such, all uses of the non-DT max_brightness to be deprecated. It is
only kept for backwards compatibility with non-DT boards.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case
From: Peter Ujfalusi @ 2013-01-29 8:17 UTC (permalink / raw)
To: Thierry Reding
Cc: Richard Purdie, Grant Likely, Rob Landley,
Florian Tobias Schandinat, Andrew Morton, devicetree-discuss,
linux-doc, linux-kernel, linux-fbdev
In-Reply-To: <20130128210123.GA24673@avionic-0098.mockup.avionic-design.de>
On 01/28/2013 10:01 PM, Thierry Reding wrote:
> On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote:
>> It is expected that board files would have:
>> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, };
>>
>> static struct platform_pwm_backlight_data bl_data = {
>> .levels = bl_levels,
>> .max_brightness = ARRAY_SIZE(bl_levels),
>> .dft_brightness = 4,
>> .pwm_period_ns = 7812500,
>> };
>>
>> In this case the max_brightness would be out of range in the levels array.
>> Decrement the received max_brightness in every case (DT or non DT) when the
>> levels has been provided.
>
> What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1
> instead?
There is nothing wrong with that either but IMHO it is more natural for board
files to use just ARRAY_SIZE(bl_levels). In this way the handling of
data->max_brightness among non DT and DT booted kernel is more uniform in the
driver itself.
Right now all board files are using only the .max_brightness to specify the
maximum value, I could not find any users of .levels in the kernel.
--
Péter
^ permalink raw reply
* [PATCH 2/2] video: exynos_dp: move disable_irq() to exynos_dp_suspend()
From: Jingoo Han @ 2013-01-29 7:38 UTC (permalink / raw)
To: linux-fbdev
From: Ajay Kumar <ajaykumar.rs@samsung.com>
disable_irq() should be moved to exynos_dp_suspend(), because
enable_irq() is called at exynos_dp_resume().
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/exynos/exynos_dp_core.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index c7374c0..9ba973a 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -1124,8 +1124,6 @@ static int exynos_dp_remove(struct platform_device *pdev)
struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
struct exynos_dp_device *dp = platform_get_drvdata(pdev);
- disable_irq(dp->irq);
-
flush_work(&dp->hotplug_work);
if (pdev->dev.of_node) {
@@ -1138,7 +1136,6 @@ static int exynos_dp_remove(struct platform_device *pdev)
clk_disable_unprepare(dp->clock);
-
return 0;
}
@@ -1148,6 +1145,8 @@ static int exynos_dp_suspend(struct device *dev)
struct exynos_dp_platdata *pdata = dev->platform_data;
struct exynos_dp_device *dp = dev_get_drvdata(dev);
+ disable_irq(dp->irq);
+
flush_work(&dp->hotplug_work);
if (dev->of_node) {
--
1.7.2.5
^ permalink raw reply related
* [PATCH 1/2] video: exynos_dp: add missing of_node_put()
From: Jingoo Han @ 2013-01-29 7:36 UTC (permalink / raw)
To: linux-fbdev
of_find_node_by_name() returns a node pointer with refcount
incremented, use of_node_put() on it when done.
of_find_node_by_name() will call of_node_put() against
the node pass to from parameter, thus we also need to call
of_node_get(from) before calling of_find_node_by_name().
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/exynos/exynos_dp_core.c | 19 +++++++++++++------
1 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index 2ed9776..c7374c0 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -965,10 +965,11 @@ static struct exynos_dp_platdata *exynos_dp_dt_parse_pdata(struct device *dev)
static int exynos_dp_dt_parse_phydata(struct exynos_dp_device *dp)
{
- struct device_node *dp_phy_node;
+ struct device_node *dp_phy_node = of_node_get(dp->dev->of_node);
u32 phy_base;
+ int ret = 0;
- dp_phy_node = of_find_node_by_name(dp->dev->of_node, "dptx-phy");
+ dp_phy_node = of_find_node_by_name(dp_phy_node, "dptx-phy");
if (!dp_phy_node) {
dev_err(dp->dev, "could not find dptx-phy node\n");
return -ENODEV;
@@ -976,22 +977,28 @@ static int exynos_dp_dt_parse_phydata(struct exynos_dp_device *dp)
if (of_property_read_u32(dp_phy_node, "reg", &phy_base)) {
dev_err(dp->dev, "faild to get reg for dptx-phy\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto err;
}
if (of_property_read_u32(dp_phy_node, "samsung,enable-mask",
&dp->enable_mask)) {
dev_err(dp->dev, "faild to get enable-mask for dptx-phy\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto err;
}
dp->phy_addr = ioremap(phy_base, SZ_4);
if (!dp->phy_addr) {
dev_err(dp->dev, "failed to ioremap dp-phy\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err;
}
- return 0;
+err:
+ of_node_put(dp_phy_node);
+
+ return ret;
}
static void exynos_dp_phy_init(struct exynos_dp_device *dp)
--
1.7.2.5
^ permalink raw reply related
* Re: [PATCH RESEND v4] video: imxfb: Do not crash on reboot
From: Shawn Guo @ 2013-01-29 2:47 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1359427171-18109-1-git-send-email-festevam@gmail.com>
Andrew,
On Tue, Jan 29, 2013 at 12:39:31AM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> Issuing a "reboot" command after the LCD times out causes the following
> warnings:
Please ignore this one. It has hit mainline through arm-soc tree
with 3.8-rc series, as we categorized it as a critical bug fixing.
Shawn
^ permalink raw reply
* [PATCH RESEND v2] video: Kconfig: Specify the SoCs that make use of FB_IMX
From: Fabio Estevam @ 2013-01-29 2:40 UTC (permalink / raw)
To: linux-fbdev
From: Fabio Estevam <fabio.estevam@freescale.com>
FB_IMX is the framebuffer driver used by MX1, MX21, MX25 and MX27 processors.
Pass this information to the Kconfig text to make it clear.
Acked-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Add mx1/21 to the list.
drivers/video/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 0cff083..f0c53b2 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -364,7 +364,7 @@ config FB_SA1100
Y here.
config FB_IMX
- tristate "Freescale i.MX LCD support"
+ tristate "Freescale i.MX1/21/25/27 LCD support"
depends on FB && IMX_HAVE_PLATFORM_IMX_FB
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
--
1.7.9.5
^ permalink raw reply related
* [PATCH RESEND v4] video: imxfb: Do not crash on reboot
From: Fabio Estevam @ 2013-01-29 2:39 UTC (permalink / raw)
To: linux-fbdev
From: Fabio Estevam <fabio.estevam@freescale.com>
Issuing a "reboot" command after the LCD times out causes the following
warnings:
Requesting system reboot
------------[ cut here ]------------
WARNING: at drivers/clk/clk.c:471 clk_disable+0x24/0x50()
Modules linked in:
[<c001ad90>] (unwind_backtrace+0x0/0xf4) from [<c0025aac>] (warn_slowpath_common+0x48/0x60)
[<c0025aac>] (warn_slowpath_common+0x48/0x60) from [<c0025ae0>] (warn_slowpath_null+0x1c/0x24)
[<c0025ae0>] (warn_slowpath_null+0x1c/0x24) from [<c03960a0>] (clk_disable+0x24/0x50)
[<c03960a0>] (clk_disable+0x24/0x50) from [<c02695a0>] (imxfb_disable_controller+0x48/0x7c)
[<c02695a0>] (imxfb_disable_controller+0x48/0x7c) from [<c029d838>] (platform_drv_shutdown+0x18/0x1c)
[<c029d838>] (platform_drv_shutdown+0x18/0x1c) from [<c02990fc>] (device_shutdown+0x48/0x14c)
[<c02990fc>] (device_shutdown+0x48/0x14c) from [<c003d09c>] (kernel_restart_prepare+0x2c/0x3c)
[<c003d09c>] (kernel_restart_prepare+0x2c/0x3c) from [<c003d0e4>] (kernel_restart+0xc/0x48)
[<c003d0e4>] (kernel_restart+0xc/0x48) from [<c003d1e8>] (sys_reboot+0xc0/0x1bc)
[<c003d1e8>] (sys_reboot+0xc0/0x1bc) from [<c0014ca0>] (ret_fast_syscall+0x0/0x2c)
---[ end trace da6b502ca79c854f ]---
------------[ cut here ]------------
WARNING: at drivers/clk/clk.c:380 clk_unprepare+0x1c/0x2c()
Modules linked in:
[<c001ad90>] (unwind_backtrace+0x0/0xf4) from [<c0025aac>] (warn_slowpath_common+0x48/0x60)
[<c0025aac>] (warn_slowpath_common+0x48/0x60) from [<c0025ae0>] (warn_slowpath_null+0x1c/0x24)
[<c0025ae0>] (warn_slowpath_null+0x1c/0x24) from [<c0396338>] (clk_unprepare+0x1c/0x2c)
[<c0396338>] (clk_unprepare+0x1c/0x2c) from [<c02695a8>] (imxfb_disable_controller+0x50/0x7c)
[<c02695a8>] (imxfb_disable_controller+0x50/0x7c) from [<c029d838>] (platform_drv_shutdown+0x18/0x1c)
[<c029d838>] (platform_drv_shutdown+0x18/0x1c) from [<c02990fc>] (device_shutdown+0x48/0x14c)
[<c02990fc>] (device_shutdown+0x48/0x14c) from [<c003d09c>] (kernel_restart_prepare+0x2c/0x3c)
[<c003d09c>] (kernel_restart_prepare+0x2c/0x3c) from [<c003d0e4>] (kernel_restart+0xc/0x48)
[<c003d0e4>] (kernel_restart+0xc/0x48) from [<c003d1e8>] (sys_reboot+0xc0/0x1bc)
[<c003d1e8>] (sys_reboot+0xc0/0x1bc) from [<c0014ca0>] (ret_fast_syscall+0x0/0x2c)
---[ end trace da6b502ca79c8550 ]---
------------[ cut here ]------------
This happens because "reboot" triggers imxfb_shutdown(), which calls
imxfb_disable_controller with the clocks already disabled.
To prevent this, add a clock enabled status so that we can check if the clocks
are enabled before disabling them.
Acked-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v3:
- Changed from 'clks_enabled' to 'enabled'
Changes since v2:
- Use a better naming for the clk enabled variable
- Return immediately in imxfb_enable_controller/imxfb_disable_controller
if the the clocks are already enabled/disabled.
Changes since v1:
- Protect the whole function instead of only the clocks
drivers/video/imxfb.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index cf2688d..ce7de9f 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -134,6 +134,7 @@ struct imxfb_info {
struct clk *clk_ipg;
struct clk *clk_ahb;
struct clk *clk_per;
+ int enabled;
/*
* These are the addresses we mapped
@@ -513,6 +514,10 @@ static void imxfb_exit_backlight(struct imxfb_info *fbi)
static void imxfb_enable_controller(struct imxfb_info *fbi)
{
+
+ if (fbi->enabled)
+ return;
+
pr_debug("Enabling LCD controller\n");
writel(fbi->screen_dma, fbi->regs + LCDC_SSA);
@@ -533,6 +538,7 @@ static void imxfb_enable_controller(struct imxfb_info *fbi)
clk_prepare_enable(fbi->clk_ipg);
clk_prepare_enable(fbi->clk_ahb);
clk_prepare_enable(fbi->clk_per);
+ fbi->enabled = 1;
if (fbi->backlight_power)
fbi->backlight_power(1);
@@ -542,6 +548,9 @@ static void imxfb_enable_controller(struct imxfb_info *fbi)
static void imxfb_disable_controller(struct imxfb_info *fbi)
{
+ if (!fbi->enabled)
+ return;
+
pr_debug("Disabling LCD controller\n");
if (fbi->backlight_power)
@@ -552,6 +561,7 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
clk_disable_unprepare(fbi->clk_per);
clk_disable_unprepare(fbi->clk_ipg);
clk_disable_unprepare(fbi->clk_ahb);
+ fbi->enabled = 0;
writel(0, fbi->regs + LCDC_RMCR);
}
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Alexander Holler @ 2013-01-29 0:56 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-fbdev, Florian Tobias Schandinat,
Bernie Thompson, Steve Glendinning, stable
In-Reply-To: <20130128162238.7fba92fe.akpm@linux-foundation.org>
Am 29.01.2013 01:22, schrieb Andrew Morton:
> On Fri, 25 Jan 2013 19:49:27 +0100
> Alexander Holler <holler@ahsoftware.de> wrote:
>
>> When a device was disconnected the driver may hang at waiting for urbs it never
>> will get. Fix this by using a timeout while waiting for the used semaphore.
>>
>> There is still a memory leak if a timeout happens, but at least the driver
>> now continues his disconnect routine.
>>
>> ...
>>
>> --- a/drivers/video/udlfb.c
>> +++ b/drivers/video/udlfb.c
>> @@ -1832,8 +1832,9 @@ static void dlfb_free_urb_list(struct dlfb_data *dev)
>> /* keep waiting and freeing, until we've got 'em all */
>> while (count--) {
>>
>> - /* Getting interrupted means a leak, but ok at disconnect */
>> - ret = down_interruptible(&dev->urbs.limit_sem);
>> + /* Timeout likely occurs at disconnect (resulting in a leak) */
>> + ret = down_timeout_killable(&dev->urbs.limit_sem,
>> + FREE_URB_TIMEOUT);
>> if (ret)
>> break;
>
> This is rather a hack. Do you have an understanding of the underlying
> bug? Why is the driver waiting for things which will never happen?
It is a hack, but I don't want to rewrite the whole driver. There is
already an attempt to do so, udl. The hack is a quick solution which
doesn't make something worse, just better. The only drawback might be
the few additional bytes for the implementation of
down_timeout_killable(), but I thought such should be already available,
and wondered it wasn't.
Fixing the underlying problem (including removing the leaks) would just
end up in another rewrite and I prefer to have a look at udl, maybe
fixing the current problems to use such a device as console there.
I've only posted this small patch series, because some (longterm) stable
kernels don't have udl, and smscufx, which is in large parts identical
to udlfb might want that patch too.
Just in case: I don't know anything about smscufx and have discovered
the similarities between those drivers only when I did a git grep to
search for something I fixed with a previous patch.
Regards,
Alexander
^ permalink raw reply
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Andrew Morton @ 2013-01-29 0:22 UTC (permalink / raw)
To: Alexander Holler
Cc: linux-kernel, linux-fbdev, Florian Tobias Schandinat,
Bernie Thompson, Steve Glendinning, stable
In-Reply-To: <1359139768-32294-2-git-send-email-holler@ahsoftware.de>
On Fri, 25 Jan 2013 19:49:27 +0100
Alexander Holler <holler@ahsoftware.de> wrote:
> When a device was disconnected the driver may hang at waiting for urbs it never
> will get. Fix this by using a timeout while waiting for the used semaphore.
>
> There is still a memory leak if a timeout happens, but at least the driver
> now continues his disconnect routine.
>
> ...
>
> --- a/drivers/video/udlfb.c
> +++ b/drivers/video/udlfb.c
> @@ -1832,8 +1832,9 @@ static void dlfb_free_urb_list(struct dlfb_data *dev)
> /* keep waiting and freeing, until we've got 'em all */
> while (count--) {
>
> - /* Getting interrupted means a leak, but ok at disconnect */
> - ret = down_interruptible(&dev->urbs.limit_sem);
> + /* Timeout likely occurs at disconnect (resulting in a leak) */
> + ret = down_timeout_killable(&dev->urbs.limit_sem,
> + FREE_URB_TIMEOUT);
> if (ret)
> break;
This is rather a hack. Do you have an understanding of the underlying
bug? Why is the driver waiting for things which will never happen?
^ permalink raw reply
* Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case
From: Thierry Reding @ 2013-01-28 21:01 UTC (permalink / raw)
To: Peter Ujfalusi
Cc: Richard Purdie, Grant Likely, Rob Landley,
Florian Tobias Schandinat, Andrew Morton, devicetree-discuss,
linux-doc, linux-kernel, linux-fbdev
In-Reply-To: <1358861996-27194-2-git-send-email-peter.ujfalusi@ti.com>
[-- Attachment #1: Type: text/plain, Size: 660 bytes --]
On Tue, Jan 22, 2013 at 02:39:53PM +0100, Peter Ujfalusi wrote:
> It is expected that board files would have:
> static unsigned int bl_levels[] = { 0, 50, 100, 150, 200, 250, };
>
> static struct platform_pwm_backlight_data bl_data = {
> .levels = bl_levels,
> .max_brightness = ARRAY_SIZE(bl_levels),
> .dft_brightness = 4,
> .pwm_period_ns = 7812500,
> };
>
> In this case the max_brightness would be out of range in the levels array.
> Decrement the received max_brightness in every case (DT or non DT) when the
> levels has been provided.
What's wrong with specifying .max_brightness = ARRAY_SIZE(bl_levels) - 1
instead?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
From: Rob Clark @ 2013-01-28 16:37 UTC (permalink / raw)
To: Mohammed, Afzal
Cc: dri-devel@lists.freedesktop.org, linux-omap@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, patches@linaro.org,
linux-fbdev@vger.kernel.org, Mike Turquette, Nori, Sekhar
In-Reply-To: <C8443D0743D26F4388EA172BF4E2A7A93EA944D2@DBDE01.ent.ti.com>
On Mon, Jan 28, 2013 at 3:56 AM, Mohammed, Afzal <afzal@ti.com> wrote:
> Hi Rob,
>
> On Fri, Jan 25, 2013 at 20:22:55, Rob Clark wrote:
>> On Fri, Jan 25, 2013 at 8:15 AM, Mohammed, Afzal <afzal@ti.com> wrote:
>
>> > It's not about being simple, but not doing the wrong way, here you are
>> > relying on a platform specific clock in a driver, think about the case
>> > where same IP is used on another platform. Either way it is not a good
>> > thing to handle platform specific details (about disp_clk) in driver.
>
>> Right, but I was trying to understand what was the benefit that the
>> added complexity is. I didn't realize on davinci that you are limited
>
> Here I am referring to usage of disp_clk,
>
> Driver is not supposed to do platform hacks - here you are trying to
> configure something (PLL) in your driver which is not part of LCDC IP.
> And LCDC IP is not aware of PLL which is a platform specific matter
> (existent only in AM335x), it is only aware of functional clock.
>
> You can set the rate on "fck" (functional clock) in AM335x using my patch,
> "ARM: AM33XX: clock: SET_RATE_PARENT in lcd path", and there
> would not be any need for driver to be aware of platform specific PLL.
right, but I think it would be better to just make another patch that
changes tilcdc to just set rate on fck after that patch is merged. I
mean, I'd rather have the driver at least usable on AM33xx until then,
rather than broken for everyone.
BR,
-R
>> >> >> + priv->clk = clk_get(dev->dev, "fck");
>
>> >> >> + priv->disp_clk = clk_get(dev->dev, "dpll_disp_ck");
>
>> at the moment all this discussion is a bit moot. I'd propose leaving
>> the driver as it is for now, because that at least makes it useful on
>> am33xx. And when CCF and davinci have the needed support in place,
>
> Let's forget about leveraging CCF in driver, but sane solution w.r.t PLL
> configuration would be to do as mentioned above.
>
> Regards
> Afzal
>
^ permalink raw reply
* Re: [git pull] fbcon locking fixes.
From: Maarten Lankhorst @ 2013-01-28 12:45 UTC (permalink / raw)
To: Dave Airlie
Cc: Linus Torvalds, m.b.lankhorst, linux-fbdev@vger.kernel.org,
Linux Kernel Mailing List, DRI mailing list, Andrew Morton
In-Reply-To: <CAPM=9txUNG_CeK+YBhcA_47BVv4Z2GAmEC_J59cY+D9nRgv54A@mail.gmail.com>
Hey,
Op 25-01-13 02:45, Dave Airlie schreef:
> On Fri, Jan 25, 2013 at 11:06 AM, Dave Airlie <airlied@gmail.com> wrote:
>> On Fri, Jan 25, 2013 at 10:53 AM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>> On Thu, Jan 24, 2013 at 4:42 PM, Dave Airlie <airlied@linux.ie> wrote:
>>>> These patches have been sailing around long enough, waiting for a maintainer
>>>> to reappear, so I've decided enough is enough, lockdep is kinda useful to have.
>>> Last this was tried, these patches failed miserably.
>>>
>>> They caused instant lockdep splat and then a total lockup with efifb.
>>> It may be that Takashi's patch helps fix that problem, but it's in no
>>> way clear that it does, so the patch series isn't at all obviously
>>> stable.
>>>
>>> Yes, lockdep is indeed "kinda useful", and there clearly are locking
>>> problems in fbdev. But I'm not seeing myself pulling these for 3.8.
>>> They've been too problematic to pull in at this late stage.
>>>
>> Okay I'll fix the efifb problem and then maybe queue them for -next.
> Okay I've just sent out another fbcon patch to fix the locking harder.
>
> There was a path going into set_con2fb_path if an fb driver was
> already registered, I just pushed the locking out further on anyone
> going in there.
>
> it boots on my EFI macbook here.
>
I cherry picked those patches to my tree, and the full series no longer triggers a lockdep warning.
It also no longer locks up during modprobing or vga-switcheroo either.
Tested-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
~Maarten
^ permalink raw reply
* RE: [PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)
From: Mohammed, Afzal @ 2013-01-28 9:56 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel@lists.freedesktop.org, linux-omap@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, patches@linaro.org,
linux-fbdev@vger.kernel.org, Mike Turquette, Nori, Sekhar
In-Reply-To: <CAF6AEGtrHBxvnp052K6z3aj3dAxJf1hy7H72EoAWr6PhTtwr8Q@mail.gmail.com>
SGkgUm9iLA0KDQpPbiBGcmksIEphbiAyNSwgMjAxMyBhdCAyMDoyMjo1NSwgUm9iIENsYXJrIHdy
b3RlOg0KPiBPbiBGcmksIEphbiAyNSwgMjAxMyBhdCA4OjE1IEFNLCBNb2hhbW1lZCwgQWZ6YWwg
PGFmemFsQHRpLmNvbT4gd3JvdGU6DQoNCj4gPiBJdCdzIG5vdCBhYm91dCBiZWluZyBzaW1wbGUs
IGJ1dCBub3QgZG9pbmcgdGhlIHdyb25nIHdheSwgaGVyZSB5b3UgYXJlDQo+ID4gcmVseWluZyBv
biBhIHBsYXRmb3JtIHNwZWNpZmljIGNsb2NrIGluIGEgZHJpdmVyLCB0aGluayBhYm91dCB0aGUg
Y2FzZQ0KPiA+IHdoZXJlIHNhbWUgSVAgaXMgdXNlZCBvbiBhbm90aGVyIHBsYXRmb3JtLiBFaXRo
ZXIgd2F5IGl0IGlzIG5vdCBhIGdvb2QNCj4gPiB0aGluZyB0byBoYW5kbGUgcGxhdGZvcm0gc3Bl
Y2lmaWMgZGV0YWlscyAoYWJvdXQgZGlzcF9jbGspIGluIGRyaXZlci4NCg0KPiBSaWdodCwgYnV0
IEkgd2FzIHRyeWluZyB0byB1bmRlcnN0YW5kIHdoYXQgd2FzIHRoZSBiZW5lZml0IHRoYXQgdGhl
DQo+IGFkZGVkIGNvbXBsZXhpdHkgaXMuICBJIGRpZG4ndCByZWFsaXplIG9uIGRhdmluY2kgdGhh
dCB5b3UgYXJlIGxpbWl0ZWQNCg0KSGVyZSBJIGFtIHJlZmVycmluZyB0byB1c2FnZSBvZiBkaXNw
X2NsaywNCg0KRHJpdmVyIGlzIG5vdCBzdXBwb3NlZCB0byBkbyBwbGF0Zm9ybSBoYWNrcyAtIGhl
cmUgeW91IGFyZSB0cnlpbmcgdG8NCmNvbmZpZ3VyZSBzb21ldGhpbmcgKFBMTCkgaW4geW91ciBk
cml2ZXIgd2hpY2ggaXMgbm90IHBhcnQgb2YgTENEQyBJUC4NCkFuZCBMQ0RDIElQIGlzIG5vdCBh
d2FyZSBvZiBQTEwgd2hpY2ggaXMgYSBwbGF0Zm9ybSBzcGVjaWZpYyBtYXR0ZXINCihleGlzdGVu
dCBvbmx5IGluIEFNMzM1eCksIGl0IGlzIG9ubHkgYXdhcmUgb2YgZnVuY3Rpb25hbCBjbG9jay4N
Cg0KWW91IGNhbiBzZXQgdGhlIHJhdGUgb24gImZjayIgKGZ1bmN0aW9uYWwgY2xvY2spIGluIEFN
MzM1eCB1c2luZyBteSBwYXRjaCwNCiJBUk06IEFNMzNYWDogY2xvY2s6IFNFVF9SQVRFX1BBUkVO
VCBpbiBsY2QgcGF0aCIsIGFuZCB0aGVyZQ0Kd291bGQgbm90IGJlIGFueSBuZWVkIGZvciBkcml2
ZXIgdG8gYmUgYXdhcmUgb2YgcGxhdGZvcm0gc3BlY2lmaWMgUExMLg0KDQo+ID4+ID4+ICsgICAg
IHByaXYtPmNsayA9IGNsa19nZXQoZGV2LT5kZXYsICJmY2siKTsNCg0KPiA+PiA+PiArICAgICBw
cml2LT5kaXNwX2NsayA9IGNsa19nZXQoZGV2LT5kZXYsICJkcGxsX2Rpc3BfY2siKTsNCg0KPiBh
dCB0aGUgbW9tZW50IGFsbCB0aGlzIGRpc2N1c3Npb24gaXMgYSBiaXQgbW9vdC4gIEknZCBwcm9w
b3NlIGxlYXZpbmcNCj4gdGhlIGRyaXZlciBhcyBpdCBpcyBmb3Igbm93LCBiZWNhdXNlIHRoYXQg
YXQgbGVhc3QgbWFrZXMgaXQgdXNlZnVsIG9uDQo+IGFtMzN4eC4gIEFuZCB3aGVuIENDRiBhbmQg
ZGF2aW5jaSBoYXZlIHRoZSBuZWVkZWQgc3VwcG9ydCBpbiBwbGFjZSwNCg0KTGV0J3MgZm9yZ2V0
IGFib3V0IGxldmVyYWdpbmcgQ0NGIGluIGRyaXZlciwgYnV0IHNhbmUgc29sdXRpb24gdy5yLnQg
UExMDQpjb25maWd1cmF0aW9uIHdvdWxkIGJlIHRvIGRvIGFzIG1lbnRpb25lZCBhYm92ZS4NCg0K
UmVnYXJkcw0KQWZ6YWwNCg0K
^ permalink raw reply
* [PATCH v5 12/12] video: da8xx-fb: set upstream clock rate (if reqd)
From: Afzal Mohammed @ 2013-01-28 9:33 UTC (permalink / raw)
To: linux-fbdev, linux-omap, devicetree-discuss, linux-doc,
linux-kernel
Cc: Florian Tobias Schandinat, Tomi Valkeinen, Grant Likely,
Rob Herring, Rob Landley, Mike Turquette
In-Reply-To: <cover.1359356015.git.afzal@ti.com>
LCDC IP has a clock divider to adjust pixel clock, this limits pixel
clock range to fck/255 - fck/2(fck - rate of input clock to LCDC IP).
In the case of AM335x, where this IP is present, default fck is not
sufficient to provide normal pixel clock rates, hence rendering this
driver unusable on AM335x.
If input clock too is configurable, allowable range of pixel clock
would increase. Here initially it is checked whether with present fck,
divider in IP could be configured to obtain required rate, if not,
fck is adjusted. This makes it usable on AM335x.
Note:
Another solution would be to model an inherited basic clock divider of
CCF, an advantage would be a better possible resolution for pixel clk.
And trying to instantiate a CCF clock would mean that to be consistent,
3 bits being turned on to enable clocks of LCDC IP would have to be
modeled as gate clocks. Now that would bring in a total of 4 clocks,
including necessity to create a new inherited divider clock, and that
mean a branch of clock tree would be present in LCDC driver. This
would add complexity to LCDC driver bringing in considerable amount
of clock handling code, and this would not bring in much advantage
for existing use cases other than providing a higher resolution of
pixel clock. And existing use cases work without relying on clock
modeling. Another fact is that out of the two platform's using this
driver DaVinci is not yet converted to CCF. In future if higher
resolution of pixel clock is required, and probably after DaVinci is
CCF'ed, modeling clock nodes inside driver may be considered.
Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
v5: use v2 method of configuring pixel clock rate instead of modeling
CCF clock nodes in driver, i.e. set divider if rate is within
the range that is configurable with existing input clock rate,
else change input clock rate as required.
v4: use new registration for clock divider having minimum divider
requirement and have ifdef'ery in a better way
v3: model CCF clock divider with parent propogation if CCF selected
v2: new patch
drivers/video/da8xx-fb.c | 76 ++++++++++++++++++++++++++++++++++++------------
1 file changed, 58 insertions(+), 18 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 5455682..09dfa12 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -133,6 +133,9 @@
#define WSI_TIMEOUT 50
#define PALETTE_SIZE 256
+#define CLK_MIN_DIV 2
+#define CLK_MAX_DIV 255
+
static void __iomem *da8xx_fb_reg_base;
static struct resource *lcdc_regs;
static unsigned int lcd_revision;
@@ -683,23 +686,21 @@ static void da8xx_fb_lcd_reset(void)
}
}
-static inline unsigned da8xx_fb_calc_clk_divider(struct da8xx_fb_par *par,
- unsigned pixclock)
-{
- return par->lcd_fck_rate / (PICOS2KHZ(pixclock) * 1000);
-}
-
-static inline unsigned da8xx_fb_round_clk(struct da8xx_fb_par *par,
- unsigned pixclock)
+static int da8xx_fb_config_clk_divider(struct da8xx_fb_par *par,
+ unsigned div, unsigned rate)
{
- unsigned div;
+ int ret;
- div = da8xx_fb_calc_clk_divider(par, pixclock);
- return KHZ2PICOS(par->lcd_fck_rate / (1000 * div));
-}
+ if (par->lcd_fck_rate != rate) {
+ ret = clk_set_rate(par->lcdc_clk, rate);
+ if (IS_ERR_VALUE(ret)) {
+ dev_err(par->dev,
+ "unable to set clock rate at %u\n", rate);
+ return ret;
+ }
+ par->lcd_fck_rate = clk_get_rate(par->lcdc_clk);
+ }
-static inline void da8xx_fb_config_clk_divider(unsigned div)
-{
/* Configure the LCD clock divisor. */
lcdc_write(LCD_CLK_DIVISOR(div) |
(LCD_RASTER_MODE & 0x1), LCD_CTRL_REG);
@@ -707,14 +708,49 @@ static inline void da8xx_fb_config_clk_divider(unsigned div)
if (lcd_revision = LCD_VERSION_2)
lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);
+
+ return 0;
+}
+
+static unsigned int da8xx_fb_calc_clk_divider(struct da8xx_fb_par *par,
+ unsigned pixclock,
+ unsigned *rate)
+{
+ unsigned div;
+
+ pixclock = PICOS2KHZ(pixclock) * 1000;
+
+ *rate = par->lcd_fck_rate;
+
+ if (pixclock < (*rate / CLK_MAX_DIV)) {
+ *rate = clk_round_rate(par->lcdc_clk, pixclock * CLK_MAX_DIV);
+ div = CLK_MAX_DIV;
+ } else if (pixclock > (*rate / CLK_MIN_DIV)) {
+ *rate = clk_round_rate(par->lcdc_clk, pixclock * CLK_MIN_DIV);
+ div = CLK_MIN_DIV;
+ } else {
+ div = *rate / pixclock;
+ }
+
+ return div;
}
-static inline void da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
+static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
struct fb_videomode *mode)
{
- unsigned div = da8xx_fb_calc_clk_divider(par, mode->pixclock);
+ unsigned rate;
+ unsigned div = da8xx_fb_calc_clk_divider(par, mode->pixclock, &rate);
- da8xx_fb_config_clk_divider(div);
+ return da8xx_fb_config_clk_divider(par, div, rate);
+}
+
+static inline unsigned da8xx_fb_round_clk(struct da8xx_fb_par *par,
+ unsigned pixclock)
+{
+ unsigned div, rate;
+
+ div = da8xx_fb_calc_clk_divider(par, pixclock, &rate);
+ return KHZ2PICOS(rate / (1000 * div));
}
static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
@@ -723,7 +759,11 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
u32 bpp;
int ret = 0;
- da8xx_fb_calc_config_clk_divider(par, panel);
+ ret = da8xx_fb_calc_config_clk_divider(par, panel);
+ if (IS_ERR_VALUE(ret)) {
+ dev_err(par->dev, "unable to configure clock\n");
+ return ret;
+ }
if (panel->sync & FB_SYNC_CLK_INVERT)
lcdc_write((lcdc_read(LCD_RASTER_TIMING_2_REG) |
--
1.7.12
^ permalink raw reply related
* [PATCH v5 11/12] video: da8xx-fb: setup struct lcd_ctrl_config for dt
From: Afzal Mohammed @ 2013-01-28 9:33 UTC (permalink / raw)
To: linux-fbdev, linux-omap, devicetree-discuss, linux-doc,
linux-kernel
Cc: Florian Tobias Schandinat, Tomi Valkeinen, Grant Likely,
Rob Herring, Rob Landley, Mike Turquette
In-Reply-To: <cover.1359356015.git.afzal@ti.com>
strcut lcd_ctrl_config information required for driver is currently
obtained via platform data. To handle DT probing, create
lcd_ctrl_config and populate it with default values, these values are
sufficient for the panels so far used with this controller to work.
Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
drivers/video/da8xx-fb.c | 34 +++++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 1c1a616..5455682 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -1254,6 +1254,35 @@ static struct fb_ops da8xx_fb_ops = {
.fb_blank = cfb_blank,
};
+static struct lcd_ctrl_config *da8xx_fb_create_cfg(struct platform_device *dev)
+{
+ struct lcd_ctrl_config *cfg;
+
+ cfg = devm_kzalloc(&dev->dev, sizeof(struct fb_videomode), GFP_KERNEL);
+ if (!cfg) {
+ dev_err(&dev->dev, "memory allocation failed\n");
+ return NULL;
+ }
+
+ /* default values */
+
+ if (lcd_revision = LCD_VERSION_1)
+ cfg->bpp = 16;
+ else
+ cfg->bpp = 32;
+
+ /*
+ * For panels so far used with this LCDC, below statement is sufficient.
+ * For new panels, if required, struct lcd_ctrl_cfg fields to be updated
+ * with additional/modified values. Those values would have to be then
+ * obtained from dt(requiring new dt bindings).
+ */
+
+ cfg->panel_shade = COLOR_ACTIVE;
+
+ return cfg;
+}
+
static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
{
struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
@@ -1345,7 +1374,10 @@ static int fb_probe(struct platform_device *device)
break;
}
- lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
+ if (device->dev.of_node)
+ lcd_cfg = da8xx_fb_create_cfg(device);
+ else
+ lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
if (!lcd_cfg) {
ret = -EINVAL;
--
1.7.12
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox