* [PATCH 02/13] OMAPDSS: DSI: flush posted write in send_bta
From: Tomi Valkeinen @ 2011-11-24 13:29 UTC (permalink / raw)
To: linux-fbdev, linux-omap; +Cc: archit, Tomi Valkeinen
In-Reply-To: <1322141381-5395-1-git-send-email-tomi.valkeinen@ti.com>
Flush posted write after setting the bit to send the BTA to ensure the
BTA is sent right away, as the code in dsi_vc_send_bta_sync() waits for
an interrupt caused indirectly by sending the BTA.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/omap2/dss/dsi.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 1331f92..4b1c074 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -2913,6 +2913,9 @@ static int dsi_vc_send_bta(struct platform_device *dsidev, int channel)
REG_FLD_MOD(dsidev, DSI_VC_CTRL(channel), 1, 6, 6); /* BTA_EN */
+ /* flush posted write */
+ dsi_read_reg(dsidev, DSI_VC_CTRL(channel));
+
return 0;
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH 01/13] OMAPDSS: DSI: flush posted write when entering ULPS
From: Tomi Valkeinen @ 2011-11-24 13:29 UTC (permalink / raw)
To: linux-fbdev, linux-omap; +Cc: archit, Tomi Valkeinen
In-Reply-To: <1322141381-5395-1-git-send-email-tomi.valkeinen@ti.com>
Flush posted write between writing the ULPS enable bits and waiting for
the interrupt.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
drivers/video/omap2/dss/dsi.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 5abf8e7..1331f92 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -3561,6 +3561,9 @@ static int dsi_enter_ulps(struct platform_device *dsidev)
REG_FLD_MOD(dsidev, DSI_COMPLEXIO_CFG2, (1 << 0) | (1 << 1) | (1 << 2),
7, 5);
+ /* flush posted write and wait for SCP interface to finish the write */
+ dsi_read_reg(dsidev, DSI_COMPLEXIO_CFG2);
+
if (wait_for_completion_timeout(&completion,
msecs_to_jiffies(1000)) = 0) {
DSSERR("ULPS enable timeout\n");
@@ -3575,6 +3578,9 @@ static int dsi_enter_ulps(struct platform_device *dsidev)
REG_FLD_MOD(dsidev, DSI_COMPLEXIO_CFG2, (0 << 0) | (0 << 1) | (0 << 2),
7, 5);
+ /* flush posted write and wait for SCP interface to finish the write */
+ dsi_read_reg(dsidev, DSI_COMPLEXIO_CFG2);
+
dsi_cio_power(dsidev, DSI_COMPLEXIO_POWER_ULPS);
dsi_if_enable(dsidev, false);
--
1.7.4.1
^ permalink raw reply related
* [PATCH 00/13] OMAPDSS: DSI fixes
From: Tomi Valkeinen @ 2011-11-24 13:29 UTC (permalink / raw)
To: linux-fbdev, linux-omap; +Cc: archit, Tomi Valkeinen
First a few patches to flush posted writes, and then DSI patches related to the
DSI lane handling. I'm not sure if the lane handling is still as clean as it
could be, but at least it feels cleaner than currently.
Tested entering ULPS on display disable on blaze tablet, and it seems to work.
However, I have no way to ensure that ULPS was actually entered, as I would
need a scope for that.
Tomi
Tomi Valkeinen (13):
OMAPDSS: DSI: flush posted write when entering ULPS
OMAPDSS: DSI: flush posted write in send_bta
OMAPDSS: DISPC: Flush posted writes when enabling outputs
OMAPDSS: DSI: count with number of lanes
OMAPDSS: DSI: Parse lane config
OMAPDSS: DSI: Use new lane config in dsi_set_lane_config
OMAPDSS: DSI: use lane config in dsi_get_lane_mask
OMAPDSS: DSI: use lane config in dsi_cio_wait_tx_clk_esc_reset
OMAPDSS: DSI: use lane config in dsi_cio_enable_lane_override
OMAPDSS: DSI: remove dsi_get_num_lanes_used
OMAPDSS: DSI: fix lane handling when entering ULPS
OMAPDSS: DSI: improve wait_for_bit_change
OMAPDSS: DSI: disable DDR_CLK_ALWAYS_ON when entering ULPS
drivers/video/omap2/dss/dispc.c | 10 +-
drivers/video/omap2/dss/dsi.c | 429 +++++++++++++++++++++------------------
2 files changed, 242 insertions(+), 197 deletions(-)
--
1.7.4.1
^ permalink raw reply
* Re: [RFC] Virtual CRTCs (proposal + experimental code)
From: Alan Cox @ 2011-11-24 12:58 UTC (permalink / raw)
To: Dave Airlie; +Cc: linux-fbdev, dri-devel
In-Reply-To: <CAPM=9tx3ue8+PQ6QBxL_0Do2kLB01Q0yN-FOZrJkY2RKiyNm4Q@mail.gmail.com>
> The thing is this is how optimus works, the nvidia gpus have an engine
> that you can program to move data from the nvidia tiled VRAM format to
This is even more of a special case than DisplayLink ;-)
> Probably a good idea to do some more research on intel/nvidia GPUs.
> With intel you can't read back from UMA since it'll be uncached memory
> so unuseable, so you'll need to use the GPU to detile and move to some
> sort of cached linear area you can readback from.
It's main memory so there are various ways to read it or pull it into
cached space.
> I merge this VCRTC stuff I give a lot of people an excuse for not
> bothering to fix the harder problems that hotplug and dynamic GPUs put
> in front of you.
I think both cases are slightly missing the mark, both are specialist
corner cases and once you add things like cameras to the mix that will
become even more painfully obvious.
The underlying need I think is a way to negotiate a shared buffer format
or pipeline between two devices. You also need in some cases to think
about shared fencing, and that is the bit that is really scary.
Figuring out the transform from A to B ('lets both use this buffer
format') or 'I can render then convert' is one thing. Dealing with two
GPUs firing into the same buffer while scanning it out I just pray
doesn't ever need shared fences.
Alan
^ permalink raw reply
* Re: [RFC] Virtual CRTCs (proposal + experimental code)
From: Daniel Vetter @ 2011-11-24 10:52 UTC (permalink / raw)
To: Dave Airlie; +Cc: linux-fbdev, dri-devel
In-Reply-To: <CAPM=9tx3ue8+PQ6QBxL_0Do2kLB01Q0yN-FOZrJkY2RKiyNm4Q@mail.gmail.com>
On Thu, Nov 24, 2011 at 08:52:45AM +0000, Dave Airlie wrote:
> So the main problem with taking all this code on-board is it sort of
> solves (a), and (b) needs another bunch of work. Now I'd rather not
> solve 50% of the issue and have future userspace apps just think they
> can ignore the problem. As much as I dislike the whole dual-gpu setups
> the fact is they exist and we can't change that, so writing userspace
> to ignore the problem because its too hard isn't going to work. So if
> I merge this VCRTC stuff I give a lot of people an excuse for not
> bothering to fix the harder problems that hotplug and dynamic GPUs put
> in front of you.
My 2 Rappen on this: I agree completely with your point that we should aim
for a full solution. GPU memory management across different devices is
hard, but solveable.
Furthermore I fear that a 50% solution that hides the memory management
and shuffling issues from userspace will end up being a leaky abstraction
(e.g. how and when is stuff transferred to the usb dp port, the kernel
might pin scanout buffers behind userspace's back screwing up the vram
accounting in userspace, random hotplugging of outputs ...). Also
v4l/embedded folks have similar issues (and the same tendency to just go
with a "simple" solution fitting their usecase) and with Intel dead-set on
entering the SoC market I'll have the joy to mess around with this stuff
pretty soon, too.
So I think we do have enough people interested in this and should be able
to cobble together something that does The Right Thing.
Cheers, Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply
* Re: [PATCH v3 1/3] fbdev: Add FOURCC-based format configuration API
From: Laurent Pinchart @ 2011-11-24 10:50 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: linux-fbdev, linux-media, magnus.damm
In-Reply-To: <201111201155.22948.laurent.pinchart@ideasonboard.com>
Hi Florian,
Gentle ping ?
On Sunday 20 November 2011 11:55:22 Laurent Pinchart wrote:
> On Sunday 20 November 2011 03:00:33 Florian Tobias Schandinat wrote:
> > Hi Laurent,
> >
> > On 08/31/2011 11:18 AM, Laurent Pinchart wrote:
> > > This API will be used to support YUV frame buffer formats in a standard
> > > way.
> >
> > looks like the union is causing problems. With this patch applied I get
> >
> > errors like this:
> > CC [M] drivers/auxdisplay/cfag12864bfb.o
> >
> > drivers/auxdisplay/cfag12864bfb.c:57: error: unknown field ‘red’
> > specified in initializer
>
> *ouch*
>
> gcc < 4.6 chokes on anonymous unions initializers :-/
>
> [snip]
>
> > > @@ -246,12 +251,23 @@ struct fb_var_screeninfo {
> > >
> > > __u32 yoffset; /* resolution */
> > >
> > > __u32 bits_per_pixel; /* guess what */
> > >
> > > - __u32 grayscale; /* != 0 Graylevels instead of colors */
> > >
> > > - struct fb_bitfield red; /* bitfield in fb mem if true color, */
> > > - struct fb_bitfield green; /* else only length is significant */
> > > - struct fb_bitfield blue;
> > > - struct fb_bitfield transp; /* transparency */
> > > + union {
> > > + struct { /* Legacy format API */
> > > + __u32 grayscale; /* 0 = color, 1 = grayscale */
> > > + /* bitfields in fb mem if true color, else only */
> > > + /* length is significant */
> > > + struct fb_bitfield red;
> > > + struct fb_bitfield green;
> > > + struct fb_bitfield blue;
> > > + struct fb_bitfield transp; /* transparency */
> > > + };
> > > + struct { /* FOURCC-based format API */
> > > + __u32 fourcc; /* FOURCC format */
> > > + __u32 colorspace;
> > > + __u32 reserved[11];
> > > + } fourcc;
> > > + };
>
> We can't name the union, otherwise this will change the userspace API.
>
> We could "fix" the problem on the kernel side with
>
> #ifdef __KERNEL__
> } color;
> #else
> };
> #endif
(and the structure that contains the grayscale, red, green, blue and transp
fields would need to be similarly named, the "rgb" name comes to mind)
> That's quite hackish though... What's your opinion ?
>
> It would also not handle userspace code that initializes an
> fb_var_screeninfo structure with named initializers, but that shouldn't
> happen, as application should read fb_var_screeninfo , modify it and write
> it back.
>
> > > __u32 nonstd; /* != 0 Non standard pixel format */
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFC] Virtual CRTCs (proposal + experimental code)
From: Dave Airlie @ 2011-11-24 8:52 UTC (permalink / raw)
To: Ilija Hadzic; +Cc: linux-fbdev, dri-devel
In-Reply-To: <Pine.GSO.4.62.1111232223110.18154@umail>
On Thu, Nov 24, 2011 at 5:59 AM, Ilija Hadzic
<ihadzic@research.bell-labs.com> wrote:
>
>
> On Wed, 23 Nov 2011, Dave Airlie wrote:
>
>> So another question I have is how you would intend this to work from a
>> user POV, like how it would integrate with a desktop environment, X or
>> wayland, i.e. with little or no configuration.
>>
>
> First thing to understand is that when a virtual CRTC is created, it looks
> to the user like the GPU has an additional DisplayPort connector.
> At present I "abuse" DisplayPort, but I have seen that you pushed a patch
> from VMware that adds Virtual connector, so eventually I'll switch to that
> naming. The number of virtual CRTCs is determined when the driver loads and
> that is a static configuration parameter. This does not restrict the user
> because unused virutal CRTCs are just like disconnected connectors on the
> GPU. In extreme case, a user could max out the number of virtual CRTCs (i.e.
> 32 minus #-of-physical-CRTCs), but in general the system needs to be booted
> with maximum number of anticipated CRTCs. Run-time addition and removal of
> CRTCs is not supported at this time and that would be much harder to
> implement and would affect the whole DRM module everywhere.
>
> So now we have a system that booted up and DRM sees all of its real
> connectors as well as virtual ones (as DisplayPorts at present). If there is
> no CTD device attached to virtual CRTCs, these virtual connectors are
> disconnected as far as DRM is concerned. Now the userspace must call
> "attach/fps" ioctl to associate CTDs with CRTCs. I'll explain shortly how to
> automate that and how to eliminate the burden from the user, but for now,
> please assume that "attach/fps" gets called from userland somehow.
>
> When the attach happens, that is a hotplug event (VCRTCM generates it) to
> DRM, just like someone plugged in the monitor. Then when XOrg starts, it
> will use the DisplayPort that represents a virtual CRTC just like any other
> connector. How it will use it, will depend on what the xorg.conf says, but
> the key point is that this connector is no different from any other
> connector that the GPU provides and is thus used as an "equal citizen". No
> special configuration is necessary once attached to CTD.
>
> If CTD is detached and new CTD attached, that is just like yanking out
> monitor cable and plugging in the new one. DRM will get all hotplug events
> and windowing system will do the same thing it would normally do with any
> other port. If RANDR is called to resize the desktop it will also work and X
> will have no idea that one of the connectors is on a virtual CRTC. I also
> have another feature, where when CTD is attached, it can ask the device it
> drives for the connection status and propagate that all the way back to DRM
> (this is useful for CTD devices that drive real monitors, like DisplayLink).
Okay so thats pretty much how I expected it to work, I don't think
Virtual makes sense for a displaylink attached device though,
again if you were using a real driver you would just re-use whatever
output type it uses, though I'm not sure how well that works,
Do you propogate full EDID information and all the modes or just the
supported modes? we use this in userspace to put monitor names in
GNOME display settings etc.
what does xrandr output looks like for a radeon GPU with 4 vcrtcs? do
you see 4 disconnected connectors? that again isn't a pretty user
experience.
> So this is your hotplug demo, but the difference is that the new desktop can
> use direct rendering. Also, everything that would work for a normal
> connector works here without having to do any additional tricks. RANDR also
> works seamlessly without having to do anything special. If you move away
> from Xorg, to some other system (Wayland?), it still works for as long as
> the new system knows how to deal with connectors that connect and
> disconnect.
My main problem with this is as I'll explain below it only covers some
of the use cases, and I don't want a 50% solution at this point, by
doing something like this you are making it harder to get proper
support into something like wayland as they can ignore some of the
problems, however since this doesn't solve all the other problems it
means getting to a finished solution is actually less likely to
happen.
>> I still foresee problems with tiling, we generally don't encourage
>> accel code to live in the kernel, and you'll really want a
>> tiled->untiled blit for this thing,
>
> Accel code should not go into the kernel (that I fully agree) and there is
> nothing here that would behove us to do so. Restricting my comments to
> Radeon GPU (which is the only one that I know well enough), shaders for blit
> copy live in the kernel and irrespective of VCRTCM work. I rely on them to
> move the frame buffer out of VRAM to CTD device but I don't add any
> additional features.
>
> Now for detiling, I think that it should be the responsibility of the
> receiving CTD device, not the GPU pushing the data (Alan mentioned that
> during the initial set of comments, and although I didn't say anything to it
> that has been my view as well).
That is a pretty much a fundamental problem, there is no way you can
enumerate all the detiling necessary in the CTD device and there is no
way I'd want to merge that code to the kernel. r600 has 16 tiling
modes (we might only see 2 of these on scanout), r300->r500 have
another different set, r100->r200 have another different set, nouveau
has an major amount of modes, and intel has a full set and has crazy
memory configuration dependant swizzling, then gma500, etc. This just
won't be workable or scalable.
> Even if you wanted to use GPU for detiling (which I'll explain shortly why
> you should not), it would not require any new accel code in the kernel. It
> would merely require one bit flip in the setup of blit copy that already
> lives in the kernel.
That is fine for radeon, not so much for intel, nouveau etc.
> However, de-tiling in GPU is a bad idea for two reasons. I tried to do that
> just as an experiment on Radeon GPUs and watched with the PCI Express
> analyzer what happens on the bus (yeah, I have some "heavy weapons" in my
> lab). Normally a tile is a continuous array of memory locations in VRAM. If
> blit-copy function is told to assume tiled source and linear destination
> (de-tiling) it will read a continuous set of addresses in VRAM, but then
> scatter 8 rows of 8 pixels each on non-contignuous set of addresses of the
> destination. If the destination is the PCI-Express bus, it will result in 8
> 32-byte write transactions instead of 2 128-byte transactions per each tile.
> That will choke the throughput of the bus right there.
The thing is this is how optimus works, the nvidia gpus have an engine
that you can program to move data from the nvidia tiled VRAM format to
the intel main memory tiled format, and make if efficent. radeon's
also have some engines that AMD so far haven't told us about, but
someone with no NDA with AMD could easily start REing that sort of
thing.
>
> Yes the read would be from UMA. I have not yet looked at Intel GPUs in
> detail, so I don't have an answer for you on what problems would pop up and
> how to solve them, but I'll be glad to revisit the Intel discussion once I
> do some homework.
Probably a good idea to do some more research on intel/nvidia GPUs.
With intel you can't read back from UMA since it'll be uncached memory
so unuseable, so you'll need to use the GPU to detile and move to some
sort of cached linear area you can readback from.
>> It also doesn't solve the optimus GPU problem in any useful fashion,
>> since it can't deal with all the use cases, so we still have to write
>> an alternate solution that can deal with them, so we just end up with
>> two answers.
>>
>
> Can you elaborate on some specific use cases that are of your concern? I
> have had this case in mind and I think I can make it work. First I would
> have to add CTD functionality to Intel driver. That should be
> straightforward. Once I get there, I'll be ready to experiment and we'll
> probably be in better position to discuss the specifics then (i.e. when we
> have something working to compare with what you did in PRIME experiemnt),
> but it would be good to know your specific concerns early.
>
Switchable/Optimus mode has two modes of operation,
a) nvidia GPU is rendering engine and the intel GPU is just used as a
scanout buffer for the LVDS panel. This mode is used when an external
digital display is plugged in, or in some plugged in configurations.
b) intel GPU is primary rendering engine, and the nvidia gpu is used
as an offload engine. This mode is used when on battery or power
saving, with no external displays plugged in. You can completely turn
on/off the nvidia GPU.
Moving between a and b has to be completely dynamic, userspace apps
need to deal with the whole world changing beneath them.
There is also switchable graphics mode, where there is a MUX used to
switch the outputs between the two GPUs.
So the main problem with taking all this code on-board is it sort of
solves (a), and (b) needs another bunch of work. Now I'd rather not
solve 50% of the issue and have future userspace apps just think they
can ignore the problem. As much as I dislike the whole dual-gpu setups
the fact is they exist and we can't change that, so writing userspace
to ignore the problem because its too hard isn't going to work. So if
I merge this VCRTC stuff I give a lot of people an excuse for not
bothering to fix the harder problems that hotplug and dynamic GPUs put
in front of you.
Dave.
>
> thanks,
>
> Ilija
>
>
^ permalink raw reply
* Re: [RFC] Virtual CRTCs (proposal + experimental code)
From: Ilija Hadzic @ 2011-11-24 5:59 UTC (permalink / raw)
To: Dave Airlie; +Cc: linux-fbdev, dri-devel
In-Reply-To: <CAPM=9tyPOkS7w3593-3ZtjSN6E+cO82125Lpmn2er8r9=PzROQ@mail.gmail.com>
On Wed, 23 Nov 2011, Dave Airlie wrote:
> So another question I have is how you would intend this to work from a
> user POV, like how it would integrate with a desktop environment, X or
> wayland, i.e. with little or no configuration.
>
First thing to understand is that when a virtual CRTC is created, it looks
to the user like the GPU has an additional DisplayPort connector.
At present I "abuse" DisplayPort, but I have seen that you pushed a patch
from VMware that adds Virtual connector, so eventually I'll switch to that
naming. The number of virtual CRTCs is determined when the driver loads
and that is a static configuration parameter. This does not restrict the
user because unused virutal CRTCs are just like disconnected connectors on
the GPU. In extreme case, a user could max out the number of virtual CRTCs
(i.e. 32 minus #-of-physical-CRTCs), but in general the system needs to be
booted with maximum number of anticipated CRTCs. Run-time addition and
removal of CRTCs is not supported at this time and that would be much
harder to implement and would affect the whole DRM module everywhere.
So now we have a system that booted up and DRM sees all of its real
connectors as well as virtual ones (as DisplayPorts at present). If there
is no CTD device attached to virtual CRTCs, these virtual connectors are
disconnected as far as DRM is concerned. Now the userspace must call
"attach/fps" ioctl to associate CTDs with CRTCs. I'll explain shortly how
to automate that and how to eliminate the burden from the user, but for
now, please assume that "attach/fps" gets called from userland somehow.
When the attach happens, that is a hotplug event (VCRTCM generates it) to
DRM, just like someone plugged in the monitor. Then when XOrg starts, it
will use the DisplayPort that represents a virtual CRTC just like any
other connector. How it will use it, will depend on what the xorg.conf
says, but the key point is that this connector is no different from any
other connector that the GPU provides and is thus used as an "equal
citizen". No special configuration is necessary once attached to CTD.
If CTD is detached and new CTD attached, that is just like yanking out
monitor cable and plugging in the new one. DRM will get all hotplug events
and windowing system will do the same thing it would normally do with any
other port. If RANDR is called to resize the desktop it will also work and
X will have no idea that one of the connectors is on a virtual CRTC. I
also have another feature, where when CTD is attached, it can ask the
device it drives for the connection status and propagate that all the way
back to DRM (this is useful for CTD devices that drive real monitors, like
DisplayLink).
So now let's go back to the attach/fps ioctl. To attach a CTD device this
ioctl must happen as a result of some policy. That can be done by having
CTD device generate UDEV events when it loads for which one can write
policies to determine which CTD device attaches to which virtual CRTC.
Ultimately that becomes an user configuration, but it's no different from
what one has to do now with UDEV policies to customize the system.
Having explained this, let's take your hotplug example that you put up on
your web page and redo it with virtual CRTCs. Here is how it would work:
Boot up the system and tell the GPU to create a few virtual CRTCs. Bring
up Xorg with no DisplayLink dongles in it. Now plug in the DisplayLink.
Once the CTD driver loads as the result of the hotplug (right now UDLCTD
is a separate driver, but as we discussed before, this is a temporary
state and at some point its CTD function should be merged wither with
UDLFB or with your UDL-V2) CTD function in the driver generates an UDEV
event. The policy directs UDEV to call run the program that issues the
ioctl to perform attach/fps. Attach/fps of the UDLCTD is now a hotplug
event and DRM "thinks" that a new connector changed the status from
disconnected to connected. That causes it to query the modes for the new
connector and because it's the virtual CRTC, it lands in the virtual CRTC
helpers in the GPU driver. Virtual CRTC helpers route it to VCRTCM, which
further routes to it to CTD (UDLCTD in this case). CTD returns the modes
and DRM gets them ... the rest you know better than me what happens ;-)
So this is your hotplug demo, but the difference is that the new desktop
can use direct rendering. Also, everything that would work for a normal
connector works here without having to do any additional tricks. RANDR
also works seamlessly without having to do anything special. If you move
away from Xorg, to some other system (Wayland?), it still works for as
long as the new system knows how to deal with connectors that connect and
disconnect.
Everything I described above is ready to go except the UDEV event from
UDLCTD and UDEV rules to automate this. Both are straightforwar and won't
take long to do. So very shortly, I'll be able to show the hotplug-bis.
From what you wrote in your blog, it sounds like this is exactly what you
are looking for. I recognize that it disrupts your current views/plans on
how this should be done, but I do want to work with you to find a suitable
middle ground that covers most of the possiblities.
In case you are looking at my code to follow the above-described
scenarios, please make sure you pull the latest stuff from my github
repository. I have been pushing new stuff since my original annoucement.
> I still foresee problems with tiling, we generally don't encourage
> accel code to live in the kernel, and you'll really want a
> tiled->untiled blit for this thing,
Accel code should not go into the kernel (that I fully agree) and there is
nothing here that would behove us to do so. Restricting my comments to
Radeon GPU (which is the only one that I know well enough), shaders for
blit copy live in the kernel and irrespective of VCRTCM work. I rely on
them to move the frame buffer out of VRAM to CTD device but I don't add
any additional features.
Now for detiling, I think that it should be the responsibility of the
receiving CTD device, not the GPU pushing the data (Alan mentioned that
during the initial set of comments, and although I didn't say anything to
it that has been my view as well).
Even if you wanted to use GPU for detiling (which I'll explain shortly why
you should not), it would not require any new accel code in the kernel. It
would merely require one bit flip in the setup of blit copy that already
lives in the kernel.
However, de-tiling in GPU is a bad idea for two reasons. I tried to do
that just as an experiment on Radeon GPUs and watched with the PCI Express
analyzer what happens on the bus (yeah, I have some "heavy weapons" in my
lab). Normally a tile is a continuous array of memory locations in VRAM.
If blit-copy function is told to assume tiled source and linear
destination (de-tiling) it will read a continuous set of addresses in
VRAM, but then scatter 8 rows of 8 pixels each on non-contignuous set of
addresses of the destination. If the destination is the PCI-Express bus,
it will result in 8 32-byte write transactions instead of 2 128-byte
transactions per each tile. That will choke the throughput of the bus
right there.
BTW, this is the crux of the blit-copy performance improvement that you
got from me back in October. Since blit-copy deals with copying a linear
array, playing with tiled/non-tiled bits only affects the order in which
addresses are accessed, so the trick was to get rid of short PCIe
transactions and also shape up linear to rectangle mapping to make address
pattern more friendly for the host.
> also for Intel GPUs where you have
> UMA, would you read from the UMA.
>
Yes the read would be from UMA. I have not yet looked at Intel GPUs in
detail, so I don't have an answer for you on what problems would pop up
and how to solve them, but I'll be glad to revisit the Intel discussion
once I do some homework.
Some initial thoughts is that frame buffer in Intel are at the end of the
day pages in the system memory, so anyone/anything can get to them if they
are correctly mapped.
> It also doesn't solve the optimus GPU problem in any useful fashion,
> since it can't deal with all the use cases, so we still have to write
> an alternate solution that can deal with them, so we just end up with
> two answers.
>
Can you elaborate on some specific use cases that are of your concern? I
have had this case in mind and I think I can make it work. First I would
have to add CTD functionality to Intel driver. That should be
straightforward. Once I get there, I'll be ready to experiment and we'll
probably be in better position to discuss the specifics then (i.e. when we
have something working to compare with what you did in PRIME experiemnt),
but it would be good to know your specific concerns early.
thanks,
Ilija
^ permalink raw reply
* Re: [PATCH 42/65] OMAPDSS: DSI: call mgr_enable/disable for cmd mode
From: Archit Taneja @ 2011-11-23 12:25 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <1322047652.28917.64.camel@deskari>
On Wednesday 23 November 2011 04:57 PM, Tomi Valkeinen wrote:
> On Wed, 2011-11-23 at 16:38 +0530, Archit Taneja wrote:
>>
>> I think it would be best to stuff the 'video mode enabling and
>> manager
>> enabling' functionality in omapdss_dsi_display_enable() itself, the
>> panel driver shouldn't need to call a function separately to enable
>> video mode for the panel. This way we would be more along the lines
>> of
>> the dpi driver, where dpi_display_enable() enables the manager in the
>> end.
>
> But we need to configure the panel between enabling the DSI interface
> and enabling the video output, so we can't combine those two functions.
Oh okay, that's right, we can't start video mode before preparing the panel.
Archit
>
> For DPI things are simpler, as enabling the interface and the video
> output are more or less the same thing.
>
> Tomi
>
^ permalink raw reply
* [PATCH] lxfb: add support for LQ057V3DG01 panel
From: Christian Gmeiner @ 2011-11-23 11:52 UTC (permalink / raw)
To: linux-fbdev; +Cc: linux-geode, linux-kernel
From 47df48b6fe482a108a0889bf2fdecfbde4aea8ee Mon Sep 17 00:00:00 2001
From: Christian Gmeiner <christian.gmeiner@gmail.com>
Date: Wed, 23 Nov 2011 12:44:14 +0100
Subject: [PATCH] lxfb: add support for LQ057V3DG01 panel
The LQ057V3DG01 panel needs some different timing
values so that the picture is centered.
Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
drivers/video/geode/lxfb_core.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/video/geode/lxfb_core.c b/drivers/video/geode/lxfb_core.c
index 416851c..7b75bae 100644
--- a/drivers/video/geode/lxfb_core.c
+++ b/drivers/video/geode/lxfb_core.c
@@ -40,6 +40,10 @@ static struct fb_videomode geode_modedb[] __devinitdata = {
{ NULL, 60, 640, 480, 39682, 48, 8, 25, 2, 88, 2,
FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
FB_VMODE_NONINTERLACED, 0 },
+ /* 640x480-60 adapted for LQ057V3DG01 panel */
+ { "LQ057V3DG01", 60, 640, 480, 39682, 48, 16, 34, 10, 88, 2,
+ FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
+ FB_VMODE_NONINTERLACED, 0 },
/* 640x400-70 */
{ NULL, 70, 640, 400, 39770, 40, 8, 28, 5, 96, 2,
FB_SYNC_HOR_HIGH_ACT,
--
1.7.5.4
^ permalink raw reply related
* Re: [RFC] Virtual CRTCs (proposal + experimental code)
From: Dave Airlie @ 2011-11-23 11:48 UTC (permalink / raw)
To: Ilija Hadzic; +Cc: linux-fbdev, dri-devel
In-Reply-To: <CA+4h6Hk3gE+8ou6NSMhoGJWZJ-5aM1OgC0HFoKs55fjfQ-qN8g@mail.gmail.com>
>
> I would like to bring to the attention of dri-devel and linux-fbdev
> community a set of hopefully useful and interesting patches that
> I (and a few other colleagues) have been working on during the past
> few months. Here, I will provide a short abstract, so that you can
> decide whether this is of interest for you. At the end, I will
> provide the pointers to the code and documentation.
>
> The code is based on Dave Arilie's tree, drm-next branch and it
> allows a GPU driver to have an arbitrary number of CRTCs
> (configurable by user) instead of only those CRTCs that represent
> real hardware.
So another question I have is how you would intend this to work from a
user POV, like how it would integrate with a desktop environment, X or
wayland, i.e. with little or no configuration.
I still foresee problems with tiling, we generally don't encourage
accel code to live in the kernel, and you'll really want a
tiled->untiled blit for this thing, also for Intel GPUs where you have
UMA, would you read from the UMA.
It also doesn't solve the optimus GPU problem in any useful fashion,
since it can't deal with all the use cases, so we still have to write
an alternate solution that can deal with them, so we just end up with
two answers.
Dave.
^ permalink raw reply
* Re: [PATCH 42/65] OMAPDSS: DSI: call mgr_enable/disable for cmd
From: Tomi Valkeinen @ 2011-11-23 11:27 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <4ECCD44A.2070008@ti.com>
[-- Attachment #1: Type: text/plain, Size: 712 bytes --]
On Wed, 2011-11-23 at 16:38 +0530, Archit Taneja wrote:
>
> I think it would be best to stuff the 'video mode enabling and
> manager
> enabling' functionality in omapdss_dsi_display_enable() itself, the
> panel driver shouldn't need to call a function separately to enable
> video mode for the panel. This way we would be more along the lines
> of
> the dpi driver, where dpi_display_enable() enables the manager in the
> end.
But we need to configure the panel between enabling the DSI interface
and enabling the video output, so we can't combine those two functions.
For DPI things are simpler, as enabling the interface and the video
output are more or less the same thing.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 42/65] OMAPDSS: DSI: call mgr_enable/disable for cmd mode
From: Archit Taneja @ 2011-11-23 11:20 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <1322044977.28917.57.camel@deskari>
On Wednesday 23 November 2011 04:12 PM, Tomi Valkeinen wrote:
> On Wed, 2011-11-23 at 15:40 +0530, Archit Taneja wrote:
>> On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
>>> The current code uses dsi_video_mode_enable/disable functions to
>>> enable/disable DISPC output for video mode displays. For command mode
>>> displays we have no notion in the DISPC side of whether the panel is
>>> enabled, except when a dss_start_update() call is made.
>>>
>>> However, to properly maintain the DISPC state in apply.c, we need to
>>> know if a manager used for a manual update display is currently in use.
>>>
>>> This patch achieves that by changing dsi_video_mode_enable/disable to
>>> dsi_enable/disable_video_output, which is called by both video and
>>> command mode displays. For video mode displays it starts the actual
>>> pixel stream, as it did before. For command mode displays it doesn't do
>>> anything else than mark that the manager is currently in use.
>>
>> dsi_video_mode_enable() doesn't only enable the DISPC output, it also
>> sends the long packet header to start video mode transfer.
>>
>> I think it would be better if we had 2 separate functions, one which
>> starts/stops DSI video mode, and the other which enables/disables the
>> DISPC video port.
>>
>> This way, a manual update panel would need to call only
>> dsi_enable/disable_video_output(which just enables or disables the
>> manager), whereas a video mode panel will need to call both.
>>
>> This is just a suggestion though. It's probably okay to have both in the
>> same function too. We might have to separate them out later if we were
>> planning to standardise mipi dsi across SoCs.
>
> If you think from the panel driver's point of view, it doesn't know
> about DISPC. It just wants to enable the video stream (on video mode
> displays).
>
> If we had two functions, could only the first be used? I.e. is it
> possible to just enable the video mode transfer, without enabling DISPC?
> If not, I'm not sure what would be the use for two separate functions.
> And even if it can, I'm not sure what use it would be to enable only the
> video mode output without the actual pixel data from DISPC.
>
> It is true that the function in thsi patch is not the best one. For
> command mode display it's more about reserving the ovl manager for use
> than actually enabling it. Then again, how I thought the function's
> purpose was that it enables the DSI video output, but because for
> command mode there's need to trigger the actual frame transfer later,
> the function doesn't start the pixel feed for command mode displays. It
> just "prepares" the output.
>
> But even if we had the functions separated, the function called by video
> mode and command mode displays would be different, as for the former one
> it enables the pixel stream, and for latter one it just reserves the
> output.
>
> So, I'm fine with changing the function, but the reasoning for what
> functions we have and what they do should come from the panel driver's
> perspective, not because of OMAP DSS's HW details.
If you look from the panel driver's perspective, we shouldn't split this.
I think it would be best to stuff the 'video mode enabling and manager
enabling' functionality in omapdss_dsi_display_enable() itself, the
panel driver shouldn't need to call a function separately to enable
video mode for the panel. This way we would be more along the lines of
the dpi driver, where dpi_display_enable() enables the manager in the end.
I guess we can leave this the way you are doing it currently, and move
this code into dsi_display_enable() code later on.
Archit
>
> Tomi
>
^ permalink raw reply
* Re: [PATCH 35/65] OMAPDSS: APPLY: move spinlock outside the struct
From: Tomi Valkeinen @ 2011-11-23 10:47 UTC (permalink / raw)
To: Sergey Kibrik; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <4ECCCB07.10702@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]
On Wed, 2011-11-23 at 12:29 +0200, Sergey Kibrik wrote:
> On 11/22/2011 11:21 AM, Tomi Valkeinen wrote:
> > dss_cache struct contains a spinlock used to protect the struct. A more
> > logical place for the spinlock is outside the struct that it is
> > protecting. So move it there.
>
> a small question: isn't it clearer to keep lock inside struct, so it would be easier to read code? Say, if we meet
>
> > spin_lock_irqsave(&dss_cache.lock, flags);
>
> in code we already aware of what struct being actually protected, and in case of external lock it's not that obvious
But if you meet code like:
op = get_ovl_priv(ovl);
You don't see that the data is inside the struct protected with the
spinlock. So you still need to understand what it protects, and what the
above function returns.
But I see your point. I'm not sure which way is better. I thought it
like this: the lock protects a struct, but if the lock is inside the
struct, the lock would protect also itself. Which it doesn't.
That said, I'm fine with both ways. It doesn't matter much. I didn't
really look for any established patterns for this in the kernel code,
but if everybody else have their locks inside the structs they protect,
then obviously we should also.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 42/65] OMAPDSS: DSI: call mgr_enable/disable for cmd
From: Tomi Valkeinen @ 2011-11-23 10:42 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <4ECCC68C.7070106@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3031 bytes --]
On Wed, 2011-11-23 at 15:40 +0530, Archit Taneja wrote:
> On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
> > The current code uses dsi_video_mode_enable/disable functions to
> > enable/disable DISPC output for video mode displays. For command mode
> > displays we have no notion in the DISPC side of whether the panel is
> > enabled, except when a dss_start_update() call is made.
> >
> > However, to properly maintain the DISPC state in apply.c, we need to
> > know if a manager used for a manual update display is currently in use.
> >
> > This patch achieves that by changing dsi_video_mode_enable/disable to
> > dsi_enable/disable_video_output, which is called by both video and
> > command mode displays. For video mode displays it starts the actual
> > pixel stream, as it did before. For command mode displays it doesn't do
> > anything else than mark that the manager is currently in use.
>
> dsi_video_mode_enable() doesn't only enable the DISPC output, it also
> sends the long packet header to start video mode transfer.
>
> I think it would be better if we had 2 separate functions, one which
> starts/stops DSI video mode, and the other which enables/disables the
> DISPC video port.
>
> This way, a manual update panel would need to call only
> dsi_enable/disable_video_output(which just enables or disables the
> manager), whereas a video mode panel will need to call both.
>
> This is just a suggestion though. It's probably okay to have both in the
> same function too. We might have to separate them out later if we were
> planning to standardise mipi dsi across SoCs.
If you think from the panel driver's point of view, it doesn't know
about DISPC. It just wants to enable the video stream (on video mode
displays).
If we had two functions, could only the first be used? I.e. is it
possible to just enable the video mode transfer, without enabling DISPC?
If not, I'm not sure what would be the use for two separate functions.
And even if it can, I'm not sure what use it would be to enable only the
video mode output without the actual pixel data from DISPC.
It is true that the function in thsi patch is not the best one. For
command mode display it's more about reserving the ovl manager for use
than actually enabling it. Then again, how I thought the function's
purpose was that it enables the DSI video output, but because for
command mode there's need to trigger the actual frame transfer later,
the function doesn't start the pixel feed for command mode displays. It
just "prepares" the output.
But even if we had the functions separated, the function called by video
mode and command mode displays would be different, as for the former one
it enables the pixel stream, and for latter one it just reserves the
output.
So, I'm fine with changing the function, but the reasoning for what
functions we have and what they do should come from the panel driver's
perspective, not because of OMAP DSS's HW details.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 35/65] OMAPDSS: APPLY: move spinlock outside the struct
From: Sergey Kibrik @ 2011-11-23 10:29 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <1321953724-6350-36-git-send-email-tomi.valkeinen@ti.com>
On 11/22/2011 11:21 AM, Tomi Valkeinen wrote:
> dss_cache struct contains a spinlock used to protect the struct. A more
> logical place for the spinlock is outside the struct that it is
> protecting. So move it there.
a small question: isn't it clearer to keep lock inside struct, so it would be easier to read code? Say, if we meet
> spin_lock_irqsave(&dss_cache.lock, flags);
in code we already aware of what struct being actually protected, and in case of external lock it's not that obvious
--
regards,
Sergey
^ permalink raw reply
* Re: [PATCH 42/65] OMAPDSS: DSI: call mgr_enable/disable for cmd mode
From: Archit Taneja @ 2011-11-23 10:22 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <1321953724-6350-43-git-send-email-tomi.valkeinen@ti.com>
On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
> The current code uses dsi_video_mode_enable/disable functions to
> enable/disable DISPC output for video mode displays. For command mode
> displays we have no notion in the DISPC side of whether the panel is
> enabled, except when a dss_start_update() call is made.
>
> However, to properly maintain the DISPC state in apply.c, we need to
> know if a manager used for a manual update display is currently in use.
>
> This patch achieves that by changing dsi_video_mode_enable/disable to
> dsi_enable/disable_video_output, which is called by both video and
> command mode displays. For video mode displays it starts the actual
> pixel stream, as it did before. For command mode displays it doesn't do
> anything else than mark that the manager is currently in use.
dsi_video_mode_enable() doesn't only enable the DISPC output, it also
sends the long packet header to start video mode transfer.
I think it would be better if we had 2 separate functions, one which
starts/stops DSI video mode, and the other which enables/disables the
DISPC video port.
This way, a manual update panel would need to call only
dsi_enable/disable_video_output(which just enables or disables the
manager), whereas a video mode panel will need to call both.
This is just a suggestion though. It's probably okay to have both in the
same function too. We might have to separate them out later if we were
planning to standardise mipi dsi across SoCs.
Archit
>
> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
> ---
> drivers/video/omap2/displays/panel-taal.c | 6 ++
> drivers/video/omap2/dss/apply.c | 6 ++-
> drivers/video/omap2/dss/dsi.c | 73 +++++++++++++++-------------
> include/video/omapdss.h | 4 +-
> 4 files changed, 51 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c
> index dd64bd1..00c5c61 100644
> --- a/drivers/video/omap2/displays/panel-taal.c
> +++ b/drivers/video/omap2/displays/panel-taal.c
> @@ -1182,6 +1182,10 @@ static int taal_power_on(struct omap_dss_device *dssdev)
> if (r)
> goto err;
>
> + r = dsi_enable_video_output(dssdev, td->channel);
> + if (r)
> + goto err;
> +
> td->enabled = 1;
>
> if (!td->intro_printed) {
> @@ -1211,6 +1215,8 @@ static void taal_power_off(struct omap_dss_device *dssdev)
> struct taal_data *td = dev_get_drvdata(&dssdev->dev);
> int r;
>
> + dsi_disable_video_output(dssdev, td->channel);
> +
> r = taal_dcs_write_0(td, MIPI_DCS_SET_DISPLAY_OFF);
> if (!r)
> r = taal_sleep_in(td);
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index 9ad2a36..66f4c56 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -648,7 +648,8 @@ void dss_mgr_enable(struct omap_overlay_manager *mgr)
> {
> mutex_lock(&apply_lock);
>
> - dispc_mgr_enable(mgr->id, true);
> + if (!mgr_manual_update(mgr))
> + dispc_mgr_enable(mgr->id, true);
> mgr->enabled = true;
>
> mutex_unlock(&apply_lock);
> @@ -658,7 +659,8 @@ void dss_mgr_disable(struct omap_overlay_manager *mgr)
> {
> mutex_lock(&apply_lock);
>
> - dispc_mgr_enable(mgr->id, false);
> + if (!mgr_manual_update(mgr))
> + dispc_mgr_enable(mgr->id, false);
> mgr->enabled = false;
>
> mutex_unlock(&apply_lock);
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 08d3de90..a35f3fb 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -3939,65 +3939,70 @@ static void dsi_proto_timings(struct omap_dss_device *dssdev)
> }
> }
>
> -int dsi_video_mode_enable(struct omap_dss_device *dssdev, int channel)
> +int dsi_enable_video_output(struct omap_dss_device *dssdev, int channel)
> {
> struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
> int bpp = dsi_get_pixel_size(dssdev->panel.dsi_pix_fmt);
> u8 data_type;
> u16 word_count;
>
> - switch (dssdev->panel.dsi_pix_fmt) {
> - case OMAP_DSS_DSI_FMT_RGB888:
> - data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
> - break;
> - case OMAP_DSS_DSI_FMT_RGB666:
> - data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
> - break;
> - case OMAP_DSS_DSI_FMT_RGB666_PACKED:
> - data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
> - break;
> - case OMAP_DSS_DSI_FMT_RGB565:
> - data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
> - break;
> - default:
> - BUG();
> - };
> + if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_VIDEO_MODE) {
> + switch (dssdev->panel.dsi_pix_fmt) {
> + case OMAP_DSS_DSI_FMT_RGB888:
> + data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
> + break;
> + case OMAP_DSS_DSI_FMT_RGB666:
> + data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
> + break;
> + case OMAP_DSS_DSI_FMT_RGB666_PACKED:
> + data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
> + break;
> + case OMAP_DSS_DSI_FMT_RGB565:
> + data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
> + break;
> + default:
> + BUG();
> + };
>
> - dsi_if_enable(dsidev, false);
> - dsi_vc_enable(dsidev, channel, false);
> + dsi_if_enable(dsidev, false);
> + dsi_vc_enable(dsidev, channel, false);
>
> - /* MODE, 1 = video mode */
> - REG_FLD_MOD(dsidev, DSI_VC_CTRL(channel), 1, 4, 4);
> + /* MODE, 1 = video mode */
> + REG_FLD_MOD(dsidev, DSI_VC_CTRL(channel), 1, 4, 4);
>
> - word_count = DIV_ROUND_UP(dssdev->panel.timings.x_res * bpp, 8);
> + word_count = DIV_ROUND_UP(dssdev->panel.timings.x_res * bpp, 8);
>
> - dsi_vc_write_long_header(dsidev, channel, data_type, word_count, 0);
> + dsi_vc_write_long_header(dsidev, channel, data_type,
> + word_count, 0);
>
> - dsi_vc_enable(dsidev, channel, true);
> - dsi_if_enable(dsidev, true);
> + dsi_vc_enable(dsidev, channel, true);
> + dsi_if_enable(dsidev, true);
> + }
>
> dss_mgr_enable(dssdev->manager);
>
> return 0;
> }
> -EXPORT_SYMBOL(dsi_video_mode_enable);
> +EXPORT_SYMBOL(dsi_enable_video_output);
>
> -void dsi_video_mode_disable(struct omap_dss_device *dssdev, int channel)
> +void dsi_disable_video_output(struct omap_dss_device *dssdev, int channel)
> {
> struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
>
> - dsi_if_enable(dsidev, false);
> - dsi_vc_enable(dsidev, channel, false);
> + if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_VIDEO_MODE) {
> + dsi_if_enable(dsidev, false);
> + dsi_vc_enable(dsidev, channel, false);
>
> - /* MODE, 0 = command mode */
> - REG_FLD_MOD(dsidev, DSI_VC_CTRL(channel), 0, 4, 4);
> + /* MODE, 0 = command mode */
> + REG_FLD_MOD(dsidev, DSI_VC_CTRL(channel), 0, 4, 4);
>
> - dsi_vc_enable(dsidev, channel, true);
> - dsi_if_enable(dsidev, true);
> + dsi_vc_enable(dsidev, channel, true);
> + dsi_if_enable(dsidev, true);
> + }
>
> dss_mgr_disable(dssdev->manager);
> }
> -EXPORT_SYMBOL(dsi_video_mode_disable);
> +EXPORT_SYMBOL(dsi_disable_video_output);
>
> static void dsi_update_screen_dispc(struct omap_dss_device *dssdev,
> u16 w, u16 h)
> diff --git a/include/video/omapdss.h b/include/video/omapdss.h
> index eaeca89..25ef771 100644
> --- a/include/video/omapdss.h
> +++ b/include/video/omapdss.h
> @@ -294,8 +294,8 @@ int dsi_vc_set_max_rx_packet_size(struct omap_dss_device *dssdev, int channel,
> u16 len);
> int dsi_vc_send_null(struct omap_dss_device *dssdev, int channel);
> int dsi_vc_send_bta_sync(struct omap_dss_device *dssdev, int channel);
> -int dsi_video_mode_enable(struct omap_dss_device *dssdev, int channel);
> -void dsi_video_mode_disable(struct omap_dss_device *dssdev, int channel);
> +int dsi_enable_video_output(struct omap_dss_device *dssdev, int channel);
> +void dsi_disable_video_output(struct omap_dss_device *dssdev, int channel);
>
> /* Board specific data */
> struct omap_dss_board_info {
^ permalink raw reply
* Re: [PATCH 40/65] OMAPDSS: APPLY: add mutex
From: Tomi Valkeinen @ 2011-11-23 10:17 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <4ECCC158.80902@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2410 bytes --]
On Wed, 2011-11-23 at 15:18 +0530, Archit Taneja wrote:
> Hi,
>
> On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
> > The functions in apply.c, called mostly via function pointers in overlay
> > and overlay_manager structs, will be divided into two groups. The other
> > group will not sleep and can be called from interrupts, and the other
> > group may sleep.
>
> Small sentence issue above, both groups are called the 'other group'.
Thanks, fixed.
> >
> > The idea is that the non-sleeping functions may only change certain
> > settings in overlays and managers, and those settings may only affect
> > the particular overlay/manager. For example, set the base address of the
> > overlay.
> >
> > The blocking functions, however, will handle more complex configuration
> > changes. For example, when an overlay is enabled and fifo-merge feature
> > is used, we need to do the enable in multiple steps, waiting in between,
> > and the change affects multiple overlays and managers.
> >
> > This patch adds the mutex which is used in the blocking functions to
> > have exclusive access to overlays and overlay managers.
>
> Previously, when we changed the links between 'overlay->managers' and
> 'manager->devices', it wasn't protected by a lock. Why is it needed now?
Previously many places were missing a lock =).
> As an example, suppose we are changing a manager's device to some other
> display. Is this lock preventing someone else to get the older
> 'mgr->device' rather than the new one?
Hmm. We need some lock there, that's for sure, as set/unset manager are
changing the manager's list of overlays. However, it is also protected
by the spinlock, so in that sense mutex is not necessary.
I have to say I'm not sure if mutex is needed at this point. However,
consider the end result when fifo-merge is used:
dss_ovl_enable() will take the mutex, then it does the configuration in
multiple steps, doing multiple spin_lock & spin_unlocks, waiting in
between.
If we had only a spinlock in set/unset_manager, the manager could be
changed while dss_ovl_enable is doing the process of enabling the
overlay.
So I may have added mutexes or spinlocks a bit early in the series to
some places, but I don't see any harm in that. It'd be rather difficult
to try to find the exact spots where a lock becomes a requirement.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 41/65] OMAPDSS: APPLY: add missing uses of spinlock
From: Tomi Valkeinen @ 2011-11-23 10:12 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <4ECCC332.4050608@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1663 bytes --]
On Wed, 2011-11-23 at 15:26 +0530, Archit Taneja wrote:
> On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
> > int dss_mgr_set_device(struct omap_overlay_manager *mgr,
> > @@ -745,16 +762,28 @@ err:
> > int dss_ovl_set_info(struct omap_overlay *ovl,
> > struct omap_overlay_info *info)
> > {
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&data_lock, flags);
> > +
> > ovl->info = *info;
> > ovl->info_dirty = true;
> >
> > + spin_unlock_irqrestore(&data_lock, flags);
> > +
> > return 0;
> > }
> >
> > void dss_ovl_get_info(struct omap_overlay *ovl,
> > struct omap_overlay_info *info)
> > {
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&data_lock, flags);
> > +
> > *info = ovl->info;
> > +
> > + spin_unlock_irqrestore(&data_lock, flags);
> > }
>
> The get/set info functions for overlays and managers only modify the
> omap_overlay_info or manager_info structs, these aren't really a part of
> 'dss_data', they only become a part of dss_data only when we call
> mgr->apply().
>
> So, are we protecting these functions so that 2 users of the same
> overlay don't see incorrect info values?
True, at this point the data_lock is a bit vague, and is protecting also
the info fields in omap_overlay and omap_overlay_manager.
A lock is needed, though, as otherwise the info struct may be only
partial. E.g. somebody calls set_info, which is half way copying the
values, and somebody else calls apply or get_info.
In the next patches the infos will be moved into the dss_data, and then
using dss_lock spin lock makes more sense.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 41/65] OMAPDSS: APPLY: add missing uses of spinlock
From: Archit Taneja @ 2011-11-23 9:57 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <1321953724-6350-42-git-send-email-tomi.valkeinen@ti.com>
On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
> The functions in apply.c, called mostly via function pointers in overlay
> and overlay_manager structs, will be divided into two groups. The other
> group will not sleep and can be called from interrupts, and the other
> group may sleep.
>
> The idea is that the non-sleeping functions may only change certain
> settings in overlays and managers, and those settings may only affect
> the particular overlay/manager. For example, set the base address of the
> overlay.
>
> The blocking functions, however, will handle more complex configuration
> changes. For example, when an overlay is enabled and fifo-merge feature
> is used, we need to do the enable in multiple steps, waiting in between,
> and the change affects multiple overlays and managers.
>
> apply.c already contains a spinlock, which has been used to protect
> (badly) the dss_data. This patch adds locks/unlocks of the spinlock to
> the missing places, and the lock should now properly protect dss_data.
>
> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
> ---
> drivers/video/omap2/dss/apply.c | 29 +++++++++++++++++++++++++++++
> 1 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index fb6d3c2..9ad2a36 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -405,6 +405,9 @@ void dss_start_update(struct omap_overlay_manager *mgr)
> struct mgr_priv_data *mp = get_mgr_priv(mgr);
> struct ovl_priv_data *op;
> struct omap_overlay *ovl;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&data_lock, flags);
>
> mp->do_manual_update = true;
> dss_write_regs();
> @@ -418,6 +421,8 @@ void dss_start_update(struct omap_overlay_manager *mgr)
> mp->shadow_dirty = false;
>
> dispc_mgr_enable(mgr->id, true);
> +
> + spin_unlock_irqrestore(&data_lock, flags);
> }
>
> static void dss_apply_irq_handler(void *data, u32 mask);
> @@ -662,16 +667,28 @@ void dss_mgr_disable(struct omap_overlay_manager *mgr)
> int dss_mgr_set_info(struct omap_overlay_manager *mgr,
> struct omap_overlay_manager_info *info)
> {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&data_lock, flags);
> +
> mgr->info = *info;
> mgr->info_dirty = true;
>
> + spin_unlock_irqrestore(&data_lock, flags);
> +
> return 0;
> }
>
> void dss_mgr_get_info(struct omap_overlay_manager *mgr,
> struct omap_overlay_manager_info *info)
> {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&data_lock, flags);
> +
> *info = mgr->info;
> +
> + spin_unlock_irqrestore(&data_lock, flags);
> }
>
> int dss_mgr_set_device(struct omap_overlay_manager *mgr,
> @@ -745,16 +762,28 @@ err:
> int dss_ovl_set_info(struct omap_overlay *ovl,
> struct omap_overlay_info *info)
> {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&data_lock, flags);
> +
> ovl->info = *info;
> ovl->info_dirty = true;
>
> + spin_unlock_irqrestore(&data_lock, flags);
> +
> return 0;
> }
>
> void dss_ovl_get_info(struct omap_overlay *ovl,
> struct omap_overlay_info *info)
> {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&data_lock, flags);
> +
> *info = ovl->info;
> +
> + spin_unlock_irqrestore(&data_lock, flags);
> }
The get/set info functions for overlays and managers only modify the
omap_overlay_info or manager_info structs, these aren't really a part of
'dss_data', they only become a part of dss_data only when we call
mgr->apply().
So, are we protecting these functions so that 2 users of the same
overlay don't see incorrect info values?
Archit
>
> int dss_ovl_set_manager(struct omap_overlay *ovl,
^ permalink raw reply
* Re: [PATCH 40/65] OMAPDSS: APPLY: add mutex
From: Archit Taneja @ 2011-11-23 9:49 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <1321953724-6350-41-git-send-email-tomi.valkeinen@ti.com>
Hi,
On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
> The functions in apply.c, called mostly via function pointers in overlay
> and overlay_manager structs, will be divided into two groups. The other
> group will not sleep and can be called from interrupts, and the other
> group may sleep.
Small sentence issue above, both groups are called the 'other group'.
>
> The idea is that the non-sleeping functions may only change certain
> settings in overlays and managers, and those settings may only affect
> the particular overlay/manager. For example, set the base address of the
> overlay.
>
> The blocking functions, however, will handle more complex configuration
> changes. For example, when an overlay is enabled and fifo-merge feature
> is used, we need to do the enable in multiple steps, waiting in between,
> and the change affects multiple overlays and managers.
>
> This patch adds the mutex which is used in the blocking functions to
> have exclusive access to overlays and overlay managers.
Previously, when we changed the links between 'overlay->managers' and
'manager->devices', it wasn't protected by a lock. Why is it needed now?
As an example, suppose we are changing a manager's device to some other
display. Is this lock preventing someone else to get the older
'mgr->device' rather than the new one?
Archit
>
> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
> ---
> drivers/video/omap2/dss/apply.c | 71 ++++++++++++++++++++++++++++++++++-----
> 1 files changed, 62 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index b935264..fb6d3c2 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -97,6 +97,8 @@ static struct {
>
> /* protects dss_data */
> static spinlock_t data_lock;
> +/* lock for blocking functions */
> +static DEFINE_MUTEX(apply_lock);
>
> static struct ovl_priv_data *get_ovl_priv(struct omap_overlay *ovl)
> {
> @@ -639,14 +641,22 @@ int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
>
> void dss_mgr_enable(struct omap_overlay_manager *mgr)
> {
> + mutex_lock(&apply_lock);
> +
> dispc_mgr_enable(mgr->id, true);
> mgr->enabled = true;
> +
> + mutex_unlock(&apply_lock);
> }
>
> void dss_mgr_disable(struct omap_overlay_manager *mgr)
> {
> + mutex_lock(&apply_lock);
> +
> dispc_mgr_enable(mgr->id, false);
> mgr->enabled = false;
> +
> + mutex_unlock(&apply_lock);
> }
>
> int dss_mgr_set_info(struct omap_overlay_manager *mgr,
> @@ -669,44 +679,65 @@ int dss_mgr_set_device(struct omap_overlay_manager *mgr,
> {
> int r;
>
> + mutex_lock(&apply_lock);
> +
> if (dssdev->manager) {
> DSSERR("display '%s' already has a manager '%s'\n",
> dssdev->name, dssdev->manager->name);
> - return -EINVAL;
> + r = -EINVAL;
> + goto err;
> }
>
> if ((mgr->supported_displays& dssdev->type) = 0) {
> DSSERR("display '%s' does not support manager '%s'\n",
> dssdev->name, mgr->name);
> - return -EINVAL;
> + r = -EINVAL;
> + goto err;
> }
>
> dssdev->manager = mgr;
> mgr->device = dssdev;
> mgr->device_changed = true;
>
> + mutex_unlock(&apply_lock);
> +
> return 0;
> +err:
> + mutex_unlock(&apply_lock);
> + return r;
> }
>
> int dss_mgr_unset_device(struct omap_overlay_manager *mgr)
> {
> + int r;
> +
> + mutex_lock(&apply_lock);
> +
> if (!mgr->device) {
> DSSERR("failed to unset display, display not set.\n");
> - return -EINVAL;
> + r = -EINVAL;
> + goto err;
> }
>
> /*
> * Don't allow currently enabled displays to have the overlay manager
> * pulled out from underneath them
> */
> - if (mgr->device->state != OMAP_DSS_DISPLAY_DISABLED)
> - return -EINVAL;
> + if (mgr->device->state != OMAP_DSS_DISPLAY_DISABLED) {
> + r = -EINVAL;
> + goto err;
> + }
>
> mgr->device->manager = NULL;
> mgr->device = NULL;
> mgr->device_changed = true;
>
> + mutex_unlock(&apply_lock);
> +
> return 0;
> +err:
> + mutex_unlock(&apply_lock);
> + return r;
> }
>
>
> @@ -729,18 +760,24 @@ void dss_ovl_get_info(struct omap_overlay *ovl,
> int dss_ovl_set_manager(struct omap_overlay *ovl,
> struct omap_overlay_manager *mgr)
> {
> + int r;
> +
> if (!mgr)
> return -EINVAL;
>
> + mutex_lock(&apply_lock);
> +
> if (ovl->manager) {
> DSSERR("overlay '%s' already has a manager '%s'\n",
> ovl->name, ovl->manager->name);
> - return -EINVAL;
> + r = -EINVAL;
> + goto err;
> }
>
> if (ovl->info.enabled) {
> DSSERR("overlay has to be disabled to change the manager\n");
> - return -EINVAL;
> + r = -EINVAL;
> + goto err;
> }
>
> ovl->manager = mgr;
> @@ -760,25 +797,41 @@ int dss_ovl_set_manager(struct omap_overlay *ovl,
> * the overlay, but before moving the overlay to TV.
> */
>
> + mutex_unlock(&apply_lock);
> +
> return 0;
> +err:
> + mutex_unlock(&apply_lock);
> + return r;
> }
>
> int dss_ovl_unset_manager(struct omap_overlay *ovl)
> {
> + int r;
> +
> + mutex_lock(&apply_lock);
> +
> if (!ovl->manager) {
> DSSERR("failed to detach overlay: manager not set\n");
> - return -EINVAL;
> + r = -EINVAL;
> + goto err;
> }
>
> if (ovl->info.enabled) {
> DSSERR("overlay has to be disabled to unset the manager\n");
> - return -EINVAL;
> + r = -EINVAL;
> + goto err;
> }
>
> ovl->manager = NULL;
> list_del(&ovl->list);
> ovl->manager_changed = true;
>
> + mutex_unlock(&apply_lock);
> +
> return 0;
> +err:
> + mutex_unlock(&apply_lock);
> + return r;
> }
>
^ permalink raw reply
* Re: [PATCH 35/65] OMAPDSS: APPLY: move spinlock outside the struct
From: Archit Taneja @ 2011-11-23 9:39 UTC (permalink / raw)
To: Archit Taneja; +Cc: Tomi Valkeinen, linux-fbdev, linux-omap, archit
In-Reply-To: <4ECCBBFC.10002@ti.com>
On Wednesday 23 November 2011 02:55 PM, Archit Taneja wrote:
<snip>
>
> Minor comment: The name 'data_lock' doesn't tell much that its
> protecting the dss_cache struct. Probably 'cache_lock' or
> 'priv_data_lock' or something like that may be more informative.
>
> Archit
Ah, just saw the next patch, you renamed dss_cache to dss_data, so
'data_lock' seems to make more sense now.
Archit
>
>> +
>> static struct ovl_priv_data *get_ovl_priv(struct omap_overlay *ovl)
>> {
>> return&dss_cache.ovl_priv_data_array[ovl->id];
>> @@ -108,7 +110,7 @@ static struct mgr_priv_data *get_mgr_priv(struct
>> omap_overlay_manager *mgr)
>>
>> void dss_apply_init(void)
>> {
>> - spin_lock_init(&dss_cache.lock);
>> + spin_lock_init(&data_lock);
>> }
>>
>> static bool ovl_manual_update(struct omap_overlay *ovl)
>> @@ -149,10 +151,10 @@ int dss_mgr_wait_for_go(struct
>> omap_overlay_manager *mgr)
>> unsigned long flags;
>> bool shadow_dirty, dirty;
>>
>> - spin_lock_irqsave(&dss_cache.lock, flags);
>> + spin_lock_irqsave(&data_lock, flags);
>> dirty = mp->dirty;
>> shadow_dirty = mp->shadow_dirty;
>> - spin_unlock_irqrestore(&dss_cache.lock, flags);
>> + spin_unlock_irqrestore(&data_lock, flags);
>>
>> if (!dirty&& !shadow_dirty) {
>> r = 0;
>> @@ -212,10 +214,10 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay
>> *ovl)
>> unsigned long flags;
>> bool shadow_dirty, dirty;
>>
>> - spin_lock_irqsave(&dss_cache.lock, flags);
>> + spin_lock_irqsave(&data_lock, flags);
>> dirty = op->dirty;
>> shadow_dirty = op->shadow_dirty;
>> - spin_unlock_irqrestore(&dss_cache.lock, flags);
>> + spin_unlock_irqrestore(&data_lock, flags);
>>
>> if (!dirty&& !shadow_dirty) {
>> r = 0;
>> @@ -464,7 +466,7 @@ static void dss_apply_irq_handler(void *data, u32
>> mask)
>> for (i = 0; i< num_mgrs; i++)
>> mgr_busy[i] = dispc_mgr_go_busy(i);
>>
>> - spin_lock(&dss_cache.lock);
>> + spin_lock(&data_lock);
>>
>> for (i = 0; i< num_ovls; ++i) {
>> ovl = omap_dss_get_overlay(i);
>> @@ -498,7 +500,7 @@ static void dss_apply_irq_handler(void *data, u32
>> mask)
>> dss_unregister_vsync_isr();
>>
>> end:
>> - spin_unlock(&dss_cache.lock);
>> + spin_unlock(&data_lock);
>> }
>>
>> static int omap_dss_mgr_apply_ovl(struct omap_overlay *ovl)
>> @@ -620,7 +622,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager
>> *mgr)
>> if (r)
>> return r;
>>
>> - spin_lock_irqsave(&dss_cache.lock, flags);
>> + spin_lock_irqsave(&data_lock, flags);
>>
>> /* Configure overlays */
>> list_for_each_entry(ovl,&mgr->overlays, list)
>> @@ -641,7 +643,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager
>> *mgr)
>> dss_write_regs();
>> }
>>
>> - spin_unlock_irqrestore(&dss_cache.lock, flags);
>> + spin_unlock_irqrestore(&data_lock, flags);
>>
>> dispc_runtime_put();
>>
>
>
^ permalink raw reply
* Re: [PATCH 35/65] OMAPDSS: APPLY: move spinlock outside the struct
From: Archit Taneja @ 2011-11-23 9:37 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <1321953724-6350-36-git-send-email-tomi.valkeinen@ti.com>
On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
> dss_cache struct contains a spinlock used to protect the struct. A more
> logical place for the spinlock is outside the struct that it is
> protecting. So move it there.
>
> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
> ---
> drivers/video/omap2/dss/apply.c | 22 ++++++++++++----------
> 1 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index 23c723a..17639c0 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -89,13 +89,15 @@ struct mgr_priv_data {
> };
>
> static struct {
> - spinlock_t lock;
> struct ovl_priv_data ovl_priv_data_array[MAX_DSS_OVERLAYS];
> struct mgr_priv_data mgr_priv_data_array[MAX_DSS_MANAGERS];
>
> bool irq_enabled;
> } dss_cache;
>
> +/* protects dss_cache */
> +static spinlock_t data_lock;
Minor comment: The name 'data_lock' doesn't tell much that its
protecting the dss_cache struct. Probably 'cache_lock' or
'priv_data_lock' or something like that may be more informative.
Archit
> +
> static struct ovl_priv_data *get_ovl_priv(struct omap_overlay *ovl)
> {
> return&dss_cache.ovl_priv_data_array[ovl->id];
> @@ -108,7 +110,7 @@ static struct mgr_priv_data *get_mgr_priv(struct omap_overlay_manager *mgr)
>
> void dss_apply_init(void)
> {
> - spin_lock_init(&dss_cache.lock);
> + spin_lock_init(&data_lock);
> }
>
> static bool ovl_manual_update(struct omap_overlay *ovl)
> @@ -149,10 +151,10 @@ int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr)
> unsigned long flags;
> bool shadow_dirty, dirty;
>
> - spin_lock_irqsave(&dss_cache.lock, flags);
> + spin_lock_irqsave(&data_lock, flags);
> dirty = mp->dirty;
> shadow_dirty = mp->shadow_dirty;
> - spin_unlock_irqrestore(&dss_cache.lock, flags);
> + spin_unlock_irqrestore(&data_lock, flags);
>
> if (!dirty&& !shadow_dirty) {
> r = 0;
> @@ -212,10 +214,10 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay *ovl)
> unsigned long flags;
> bool shadow_dirty, dirty;
>
> - spin_lock_irqsave(&dss_cache.lock, flags);
> + spin_lock_irqsave(&data_lock, flags);
> dirty = op->dirty;
> shadow_dirty = op->shadow_dirty;
> - spin_unlock_irqrestore(&dss_cache.lock, flags);
> + spin_unlock_irqrestore(&data_lock, flags);
>
> if (!dirty&& !shadow_dirty) {
> r = 0;
> @@ -464,7 +466,7 @@ static void dss_apply_irq_handler(void *data, u32 mask)
> for (i = 0; i< num_mgrs; i++)
> mgr_busy[i] = dispc_mgr_go_busy(i);
>
> - spin_lock(&dss_cache.lock);
> + spin_lock(&data_lock);
>
> for (i = 0; i< num_ovls; ++i) {
> ovl = omap_dss_get_overlay(i);
> @@ -498,7 +500,7 @@ static void dss_apply_irq_handler(void *data, u32 mask)
> dss_unregister_vsync_isr();
>
> end:
> - spin_unlock(&dss_cache.lock);
> + spin_unlock(&data_lock);
> }
>
> static int omap_dss_mgr_apply_ovl(struct omap_overlay *ovl)
> @@ -620,7 +622,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
> if (r)
> return r;
>
> - spin_lock_irqsave(&dss_cache.lock, flags);
> + spin_lock_irqsave(&data_lock, flags);
>
> /* Configure overlays */
> list_for_each_entry(ovl,&mgr->overlays, list)
> @@ -641,7 +643,7 @@ int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
> dss_write_regs();
> }
>
> - spin_unlock_irqrestore(&dss_cache.lock, flags);
> + spin_unlock_irqrestore(&data_lock, flags);
>
> dispc_runtime_put();
>
^ permalink raw reply
* Re: [PATCH 09/65] OMAPDSS: pass ovl manager to dss_start_update
From: Tomi Valkeinen @ 2011-11-23 7:32 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap, archit
In-Reply-To: <4ECC8A6D.5050000@ti.com>
[-- Attachment #1: Type: text/plain, Size: 522 bytes --]
On Wed, 2011-11-23 at 11:23 +0530, Archit Taneja wrote:
> On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:
> > dss_start_update() takes currently the dss device as a parameter. Change
> > the parameter to ovl manager, as that is what the dss_start_update()
> > actually needs.
>
> Minor comment: We could rename dss_start_update() to
> dss_mgr_start_update() to stick to the new way of telling if this
> function is meant for an overlay or a manager.
Good point, I'll make the change.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [GIT PULL] OMAP DSS fixes for 3.2-rc
From: Tomi Valkeinen @ 2011-11-23 7:31 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: linux-fbdev, linux-omap mailing list
In-Reply-To: <4ECC9DAF.3000005@gmx.de>
[-- Attachment #1: Type: text/plain, Size: 887 bytes --]
On Wed, 2011-11-23 at 07:15 +0000, Florian Tobias Schandinat wrote:
> Hi Tomi,
>
> On 11/18/2011 08:16 AM, Tomi Valkeinen wrote:
> > Hi Florian,
> >
> > Here are a few OMAP display subsystem fixes for 3.2-rc.
> >
> > One for the old omapfb, which was missing include module.h, and two for
> > the omapdss fixing issues related to HDMI.
>
> Pulled, but...
Thanks!
> > The following changes since commit cfcfc9eca2bcbd26a8e206baeb005b055dbf8e37:
> >
> > Linux 3.2-rc2 (2011-11-15 15:02:59 -0200)
>
> ...could you please try to base on something that is already in my tree, perhaps
> just add patches on top of the last pull request you sent me. If you need
> something more recent, that is okay, but don't force me to needlessly update my
> tree.
Ah, of course. I didn't think of that. I'll base the patches on top of
your tree from now on.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
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