i915 backlight
On Fri, Aug 2, 2013 at 6:35 PM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 01:58:55 PM Felipe Contreras wrote: >> On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki wrote: >> > On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote: >> >> I think it's pretty obvious that for the time being we need to >> >> blacklist a ton of machines so they boot without this OSI. In fact, in >> >> might make sense to simply remove the OSI completely for all machines >> >> (for now). >> > >> > That would have made sense 6 months ago, but not today. >> >> Today, like 6 months ago these machines remain broken, and it will be >> the same tomorrow, presumably on v3.11, and at least v3.12 as well. > > Can you possibly look at things from a bit broader perspective than just the > broken backlight? > > [I'm talking about "simply removing the OSI completely for all machines" if > that's not clear.] > > The problem is that for the last 6 months the kernel has responded to > OSI(Windows 2012) with a "yes" and now, after that time, you want it to > make a U turn and start saying "no" even though that may cause problems to > happen on other people's machines. That's simply irresponsible. The difference is we *know* *for sure* responding "Windows 2012" with a yes causes problems, on the other hand you *think* responding with a no, *might* cause problems. The only reason it would make sense to remain in the current situation is that if *knew* that switching to no would cause problems, but to my knowledge such problems are not real, merely theoretical. We have had proven brokedness for four releases (almost five now), if you disable it for v3.12 and it turns out it causes even more problems, then you enable it back again for v3.13, or even v3.12.1. The only way to know for certain is to go ahead and try it. But we know it's going to be all right, because v3.6 was all right. >> > The reason is that you don't really know what's affected by that and I'm >> > pretty sure it's not only backlight. >> >> I haven't heard a single comment that says acpi_osi="!Windows 2012" >> breaks other things. OTOH everybody is saying it fixes the backlight >> problem (if indeed it's the same problem). >> >> Are you claiming that those users are wrong? > > No, they are saying what they see and they are the people having the backlight > problem in the first place. You have no data from people for whom things work > now. The data comes from v3.6, who complained? Anyway, it's easy to find out; just disable it for *one release*. >> > So no, we won't do that. >> >> Yeah, because that would fix the backlight problems, not tomorrow, or >> several months from now, *today*. Geez, who would want that? >> >> Here is the patch to fix the problem, *today*. > > It doesn't "fix" anything. It just creates a blacklist of systems where > acpi_osi="!Windows 2012" happens to help with the backlight control problem. >From the user's point of view; right now it doesn't work, after the patch it does. That is a fix. > You don't even know why exactly it happens to work on those machines in the > first place and you don't know what is affected by that apart from backlight > (you can't be sure that nothing is affected in particular). I have a fairly good idea of the *real* problems involved with the backlight. The other problems you speak of are hypothetical, and yes, I don't know if there will be more problems, but neither do you. This is an argument from ignorance fallacy, and it's easy to solve; let's try it for one release and see how it goes. Then we would *know*. >> https://bugzilla.kernel.org/show_bug.cgi?id=60682 >> >> This is what we should do: >> >> 1) Improve that blacklist list >> 2) Fix the Intel driver issues >> 3) Enable your patch that uses the Intel driver instead >> 4) Remove that patch >> >> Anything else is not be good for the users. > > Actually, the users can easily put the acpi_osi="!Windows 2012" into the > kernel command line (which is what they have been doing already for some > time I suppose). The kernel should just work out-of-the-box. You can't expect every user out there to put all sorts of quirks into their boot command, that's why we have quirks in the kernel in the first place. > However, if we add the blacklist to the kernel, that will > mean we kind of give up fixing the backlight control for them (because they > won't have any incentive to test anything else then). No, it doesn't mean that. You should not break systems willingly and knowingly only to create incentives to the developers.
i915 backlight
On Fri, Aug 2, 2013 at 3:11 PM, Josep Lladonosa wrote: > "Before" means with previous kernels that worked with > > GRUB_CMDLINE_LINUX="acpi_osi=Linux acpi_backlight=vendor" That's probably a different issue. You would need to bisect the problem. > I have not checked this issue with acpi_osi="!Windows 2012". Please do. -- Felipe Contreras
i915 backlight
On Fri, Aug 2, 2013 at 3:03 PM, Josep Lladonosa wrote: > With this setup, something has happened: in xorg, when screen goes to > screensaver and after, enters into Standby mode, when I press a key, > it keeps black and, to recover screen, I have to adjust brightness > manually (by increasing), as if it didn't remember previous value to > standby mode. > > This was something that before didn't happen. You mean with acpi_osi="!Windows 2012"? And when you say "before", what do you mean? -- Felipe Contreras
i915 backlight
On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki wrote: > On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote: >> On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa >> wrote: >> > Hello, >> > >> > I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to >> > change to this parameter to the kernel boot: >> > >> > >> > GRUB_CMDLINE_LINUX="acpi_osi=\"!Windows 2012\"" >> >> I think it's pretty obvious that for the time being we need to >> blacklist a ton of machines so they boot without this OSI. In fact, in >> might make sense to simply remove the OSI completely for all machines >> (for now). > > That would have made sense 6 months ago, but not today. Today, like 6 months ago these machines remain broken, and it will be the same tomorrow, presumably on v3.11, and at least v3.12 as well. > The reason is that you don't really know what's affected by that and I'm > pretty sure it's not only backlight. I haven't heard a single comment that says acpi_osi="!Windows 2012" breaks other things. OTOH everybody is saying it fixes the backlight problem (if indeed it's the same problem). Are you claiming that those users are wrong? > So no, we won't do that. Yeah, because that would fix the backlight problems, not tomorrow, or several months from now, *today*. Geez, who would want that? Here is the patch to fix the problem, *today*. https://bugzilla.kernel.org/show_bug.cgi?id=60682 This is what we should do: 1) Improve that blacklist list 2) Fix the Intel driver issues 3) Enable your patch that uses the Intel driver instead 4) Remove that patch Anything else is not be good for the users. -- Felipe Contreras
Re: i915 backlight
On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa jllad...@gmail.com wrote: Hello, I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to change to this parameter to the kernel boot: GRUB_CMDLINE_LINUX=acpi_osi=\!Windows 2012\ I think it's pretty obvious that for the time being we need to blacklist a ton of machines so they boot without this OSI. In fact, in might make sense to simply remove the OSI completely for all machines (for now). -- Felipe Contreras ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: i915 backlight
On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 1:25 AM, Josep Lladonosa jllad...@gmail.com wrote: Hello, I am using a Lenovo Edge E530 and, with kernel 3.11.0-rc3, I had to change to this parameter to the kernel boot: GRUB_CMDLINE_LINUX=acpi_osi=\!Windows 2012\ I think it's pretty obvious that for the time being we need to blacklist a ton of machines so they boot without this OSI. In fact, in might make sense to simply remove the OSI completely for all machines (for now). That would have made sense 6 months ago, but not today. Today, like 6 months ago these machines remain broken, and it will be the same tomorrow, presumably on v3.11, and at least v3.12 as well. The reason is that you don't really know what's affected by that and I'm pretty sure it's not only backlight. I haven't heard a single comment that says acpi_osi=!Windows 2012 breaks other things. OTOH everybody is saying it fixes the backlight problem (if indeed it's the same problem). Are you claiming that those users are wrong? So no, we won't do that. Yeah, because that would fix the backlight problems, not tomorrow, or several months from now, *today*. Geez, who would want that? Here is the patch to fix the problem, *today*. https://bugzilla.kernel.org/show_bug.cgi?id=60682 This is what we should do: 1) Improve that blacklist list 2) Fix the Intel driver issues 3) Enable your patch that uses the Intel driver instead 4) Remove that patch Anything else is not be good for the users. -- Felipe Contreras ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: i915 backlight
On Fri, Aug 2, 2013 at 3:03 PM, Josep Lladonosa jllad...@gmail.com wrote: With this setup, something has happened: in xorg, when screen goes to screensaver and after, enters into Standby mode, when I press a key, it keeps black and, to recover screen, I have to adjust brightness manually (by increasing), as if it didn't remember previous value to standby mode. This was something that before didn't happen. You mean with acpi_osi=!Windows 2012? And when you say before, what do you mean? -- Felipe Contreras ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: i915 backlight
On Fri, Aug 2, 2013 at 3:11 PM, Josep Lladonosa jllad...@gmail.com wrote: Before means with previous kernels that worked with GRUB_CMDLINE_LINUX=acpi_osi=Linux acpi_backlight=vendor That's probably a different issue. You would need to bisect the problem. I have not checked this issue with acpi_osi=!Windows 2012. Please do. -- Felipe Contreras ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: i915 backlight
On Fri, Aug 2, 2013 at 6:35 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 01:58:55 PM Felipe Contreras wrote: On Fri, Aug 2, 2013 at 9:03 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, August 02, 2013 01:48:37 AM Felipe Contreras wrote: I think it's pretty obvious that for the time being we need to blacklist a ton of machines so they boot without this OSI. In fact, in might make sense to simply remove the OSI completely for all machines (for now). That would have made sense 6 months ago, but not today. Today, like 6 months ago these machines remain broken, and it will be the same tomorrow, presumably on v3.11, and at least v3.12 as well. Can you possibly look at things from a bit broader perspective than just the broken backlight? [I'm talking about simply removing the OSI completely for all machines if that's not clear.] The problem is that for the last 6 months the kernel has responded to OSI(Windows 2012) with a yes and now, after that time, you want it to make a U turn and start saying no even though that may cause problems to happen on other people's machines. That's simply irresponsible. The difference is we *know* *for sure* responding Windows 2012 with a yes causes problems, on the other hand you *think* responding with a no, *might* cause problems. The only reason it would make sense to remain in the current situation is that if *knew* that switching to no would cause problems, but to my knowledge such problems are not real, merely theoretical. We have had proven brokedness for four releases (almost five now), if you disable it for v3.12 and it turns out it causes even more problems, then you enable it back again for v3.13, or even v3.12.1. The only way to know for certain is to go ahead and try it. But we know it's going to be all right, because v3.6 was all right. The reason is that you don't really know what's affected by that and I'm pretty sure it's not only backlight. I haven't heard a single comment that says acpi_osi=!Windows 2012 breaks other things. OTOH everybody is saying it fixes the backlight problem (if indeed it's the same problem). Are you claiming that those users are wrong? No, they are saying what they see and they are the people having the backlight problem in the first place. You have no data from people for whom things work now. The data comes from v3.6, who complained? Anyway, it's easy to find out; just disable it for *one release*. So no, we won't do that. Yeah, because that would fix the backlight problems, not tomorrow, or several months from now, *today*. Geez, who would want that? Here is the patch to fix the problem, *today*. It doesn't fix anything. It just creates a blacklist of systems where acpi_osi=!Windows 2012 happens to help with the backlight control problem. From the user's point of view; right now it doesn't work, after the patch it does. That is a fix. You don't even know why exactly it happens to work on those machines in the first place and you don't know what is affected by that apart from backlight (you can't be sure that nothing is affected in particular). I have a fairly good idea of the *real* problems involved with the backlight. The other problems you speak of are hypothetical, and yes, I don't know if there will be more problems, but neither do you. This is an argument from ignorance fallacy, and it's easy to solve; let's try it for one release and see how it goes. Then we would *know*. https://bugzilla.kernel.org/show_bug.cgi?id=60682 This is what we should do: 1) Improve that blacklist list 2) Fix the Intel driver issues 3) Enable your patch that uses the Intel driver instead 4) Remove that patch Anything else is not be good for the users. Actually, the users can easily put the acpi_osi=!Windows 2012 into the kernel command line (which is what they have been doing already for some time I suppose). The kernel should just work out-of-the-box. You can't expect every user out there to put all sorts of quirks into their boot command, that's why we have quirks in the kernel in the first place. However, if we add the blacklist to the kernel, that will mean we kind of give up fixing the backlight control for them (because they won't have any incentive to test anything else then). No, it doesn't mean that. You should not break systems willingly and knowingly only to create incentives to the developers. When things are ready, you enable the fix again, if they break, you disable it again. At no point in time does it make sense to retain code that we know breaks user-experience. That said this is a controverisal matter and we evidently don't agree with each other. I have my reasons, you have your arguments and it doesn't look like any of us is likely to change his mind, so why don't we do what's normally done in such cases: Why don't we ask others? Matthew, Aaron, Rui, what do you think about this? Should we create
[Update][PATCH 0/3] Fix backlight issues on some Windows 8 systems
On Wed, Jul 17, 2013 at 7:16 PM, Rafael J. Wysocki wrote: > On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote: >> Windows 8 introduced new policy for backlight control by pushing it out to >> graphics drivers. This appears to have coincided with a range of vendors >> adding Windows 8 checks to their backlight control code which trigger either >> awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest >> thing to do would be to just disable ACPI backlight control entirely if the >> firmware indicates Windows 8 support, but it's entirely possible that >> individual graphics drivers might still make use of the ACPI functionality in >> preference to native control. >> >> The first two patches in this series are picked from other patchesets aimed >> at >> solving similar problems. The last simply unregisters ACPI backlight control >> on Windows 8 systems when using an Intel GPU. Similar code could be added to >> other drivers, but I'm reluctant to do so without further investigation as >> to the behaviour of the vendor drivers under Windows. > > Well, after some more time spent on that, we now have a series of 3 patches > (different from the $subject one) that we think may be used to address this > issue. As far as I can say, it has been tested by multiple people whose > systems have those problems and they generally saw improvement. > > It is not my ideal approach, but it seems to be the least intrusive and/or > with the least amount of possible side effects that we can do right now > as a general measure (alternatively, we could create a possibly long > blacklist table of affected systems with different workarounds for them, > but let's just say that is not overwhelmingly attractive). > > [1/3] Make ACPICA export things that we need for checking OSI(Win8). > > [2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even > if it is not going to register the backlight interface (needed for > Thinkpads). > > [3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes > we are Windows 8. > > Many thanks to everyone involved! I tried this patch series and it's as I expected, it's the same as acpi_backlight=vendor, and the intel backlight driver doesn't work correctly in this machine. If you are actually serious about the mantra of "no user-space regressions", then for this machine at least, you need to use the ACPI backlight with Windows8 OSI disabled, until the intel backlight driver is fixed. My patch does that: http://article.gmane.org/gmane.linux.acpi.devel/60969 -- Felipe Contreras
Re: [Update][PATCH 0/3] Fix backlight issues on some Windows 8 systems
On Wed, Jul 17, 2013 at 7:16 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Sunday, June 09, 2013 07:01:36 PM Matthew Garrett wrote: Windows 8 introduced new policy for backlight control by pushing it out to graphics drivers. This appears to have coincided with a range of vendors adding Windows 8 checks to their backlight control code which trigger either awkward behaviour (Lenovo) or complete brokenness (some Dells). The simplest thing to do would be to just disable ACPI backlight control entirely if the firmware indicates Windows 8 support, but it's entirely possible that individual graphics drivers might still make use of the ACPI functionality in preference to native control. The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows. Well, after some more time spent on that, we now have a series of 3 patches (different from the $subject one) that we think may be used to address this issue. As far as I can say, it has been tested by multiple people whose systems have those problems and they generally saw improvement. It is not my ideal approach, but it seems to be the least intrusive and/or with the least amount of possible side effects that we can do right now as a general measure (alternatively, we could create a possibly long blacklist table of affected systems with different workarounds for them, but let's just say that is not overwhelmingly attractive). [1/3] Make ACPICA export things that we need for checking OSI(Win8). [2/3] Make acpi_video_device_find_cap() call acpi_video_init_brightness() even if it is not going to register the backlight interface (needed for Thinkpads). [3/3] Avoid using ACPI backlight if i915 is in use and the firmware believes we are Windows 8. Many thanks to everyone involved! I tried this patch series and it's as I expected, it's the same as acpi_backlight=vendor, and the intel backlight driver doesn't work correctly in this machine. If you are actually serious about the mantra of no user-space regressions, then for this machine at least, you need to use the ACPI backlight with Windows8 OSI disabled, until the intel backlight driver is fixed. My patch does that: http://article.gmane.org/gmane.linux.acpi.devel/60969 -- Felipe Contreras ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/3] Fix backlight issues on some Windows 8 systems
On Sat, Jun 22, 2013 at 4:46 PM, Yves-Alexis Perez wrote:
> On dim., 2013-06-09 at 19:01 -0400, Matthew Garrett wrote:
>> The first two patches in this series are picked from other patchesets aimed
>> at
>> solving similar problems. The last simply unregisters ACPI backlight control
>> on Windows 8 systems when using an Intel GPU. Similar code could be added to
>> other drivers, but I'm reluctant to do so without further investigation as
>> to the behaviour of the vendor drivers under Windows.
>
> Hi,
>
> I've read this thread coming from
> https://bugzilla.kernel.org/show_bug.cgi?id=51231 and tried the patches
> on a Lenovo ThinkPad X230 with intel graphics.
>
> The problem with thoses fixes is that they still introduce a regression
> in how the brightness is handled, at least for me.
For me too.
> Before Linux support for acpi_osi("Windows 2012") (and when booting with
> acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
> just fine, whether in console, in the display manager or in my desktop
> environment (Xfce). xfce4-power-manager just needs to be told that the
> brightness keys are already handled and it doesn't need to do anything.
How do you tell xfce4-power-manager that?
For me everything works fine when acpi_osi="!Windows 2012", which is
why I wrote a patch for this particular laptop.
http://article.gmane.org/gmane.linux.acpi.devel/60969
> So can the previous behavior be actually restored?
It *should*. The #1 rule of the Linux kernel is to never ever break
user-space, isn't it?
> Please keep me on CC: on replies, I'm not subscribed to the various
> lists.
You don't need to ask that in mailing lists that don't have reply-to
munged (sane ones), and vger ones don't.
Cheers.
--
Felipe Contreras
Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems
On Sat, Jun 22, 2013 at 4:46 PM, Yves-Alexis Perez cor...@debian.org wrote: On dim., 2013-06-09 at 19:01 -0400, Matthew Garrett wrote: The first two patches in this series are picked from other patchesets aimed at solving similar problems. The last simply unregisters ACPI backlight control on Windows 8 systems when using an Intel GPU. Similar code could be added to other drivers, but I'm reluctant to do so without further investigation as to the behaviour of the vendor drivers under Windows. Hi, I've read this thread coming from https://bugzilla.kernel.org/show_bug.cgi?id=51231 and tried the patches on a Lenovo ThinkPad X230 with intel graphics. The problem with thoses fixes is that they still introduce a regression in how the brightness is handled, at least for me. For me too. Before Linux support for acpi_osi(Windows 2012) (and when booting with acpi_osi=!Windows 2012), brightness keys were handled by the kernel just fine, whether in console, in the display manager or in my desktop environment (Xfce). xfce4-power-manager just needs to be told that the brightness keys are already handled and it doesn't need to do anything. How do you tell xfce4-power-manager that? For me everything works fine when acpi_osi=!Windows 2012, which is why I wrote a patch for this particular laptop. http://article.gmane.org/gmane.linux.acpi.devel/60969 So can the previous behavior be actually restored? It *should*. The #1 rule of the Linux kernel is to never ever break user-space, isn't it? Please keep me on CC: on replies, I'm not subscribed to the various lists. You don't need to ask that in mailing lists that don't have reply-to munged (sane ones), and vger ones don't. Cheers. -- Felipe Contreras ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] omap2+: add drm device
On Tue, Jan 24, 2012 at 5:54 PM, Rob Clark wrote:
> On Tue, Jan 24, 2012 at 9:33 AM, Felipe Contreras
> wrote:
>> On Mon, Jan 16, 2012 at 11:24 PM, Rob Clark wrote:
>>> On Mon, Jan 16, 2012 at 2:37 PM, Felipe Contreras
>>> wrote:
>>>> On Mon, Jan 16, 2012 at 7:01 PM, Rob Clark wrote:
>>>>> On Mon, Jan 16, 2012 at 10:59 AM, Felipe Contreras
>>>>> wrote:
>>>>>> #if defined(CONFIG_DRM_OMAP) || defined(CONFIG_DRM_OMAP_MODULE)
>>>>>> extern void omapdrm_reserve_vram(void);
>>>>>> #else
>>>>>> static inline void omapdrm_reserve_vram(void) { }
>>>>>> #endif
>>>>>>
>>>>>> Like how it's done with DSP stuff.
>>>>>
>>>>> right, but then you'd miss the omapdrm_reserve_vram() call at startup..
>>>>
>>>> Why?
>>>
>>> I guess drm.o is ending up in a module, but the code that calls that
>>> (in common.c) is not in a module, so you get an unresolved symbol at
>>> link
>>
>> Right... But that code can be moved as well. Just like with the bridge.
>
> Hmm, looks like for dsp bridge the memory reservation is moved to
> devices.c. ?Although I'm not entirely sure how that is better... and
> there is precedent for both approaches, ie. omapfb works like omapdrm,
> and tidspbridge works as you suggest.
>
> seems a bit bikeshed'ish to me
I will send a patch to fix omapfb, perhaps after that it will be a bit
more clear how it should be done :) (if it gets accepted)
--
Felipe Contreras
[PATCH 1/2] omap2+: add drm device
On Mon, Jan 16, 2012 at 11:24 PM, Rob Clark wrote:
> On Mon, Jan 16, 2012 at 2:37 PM, Felipe Contreras
> wrote:
>> On Mon, Jan 16, 2012 at 7:01 PM, Rob Clark wrote:
>>> On Mon, Jan 16, 2012 at 10:59 AM, Felipe Contreras
>>> wrote:
>>>> #if defined(CONFIG_DRM_OMAP) || defined(CONFIG_DRM_OMAP_MODULE)
>>>> extern void omapdrm_reserve_vram(void);
>>>> #else
>>>> static inline void omapdrm_reserve_vram(void) { }
>>>> #endif
>>>>
>>>> Like how it's done with DSP stuff.
>>>
>>> right, but then you'd miss the omapdrm_reserve_vram() call at startup..
>>
>> Why?
>
> I guess drm.o is ending up in a module, but the code that calls that
> (in common.c) is not in a module, so you get an unresolved symbol at
> link
Right... But that code can be moved as well. Just like with the bridge.
--
Felipe Contreras
Re: [PATCH 1/2] omap2+: add drm device
On Mon, Jan 16, 2012 at 11:24 PM, Rob Clark rob.cl...@linaro.org wrote:
On Mon, Jan 16, 2012 at 2:37 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
On Mon, Jan 16, 2012 at 7:01 PM, Rob Clark rob.cl...@linaro.org wrote:
On Mon, Jan 16, 2012 at 10:59 AM, Felipe Contreras
felipe.contre...@gmail.com wrote:
#if defined(CONFIG_DRM_OMAP) || defined(CONFIG_DRM_OMAP_MODULE)
extern void omapdrm_reserve_vram(void);
#else
static inline void omapdrm_reserve_vram(void) { }
#endif
Like how it's done with DSP stuff.
right, but then you'd miss the omapdrm_reserve_vram() call at startup..
Why?
I guess drm.o is ending up in a module, but the code that calls that
(in common.c) is not in a module, so you get an unresolved symbol at
link
Right... But that code can be moved as well. Just like with the bridge.
--
Felipe Contreras
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] omap2+: add drm device
On Tue, Jan 24, 2012 at 5:54 PM, Rob Clark rob.cl...@linaro.org wrote:
On Tue, Jan 24, 2012 at 9:33 AM, Felipe Contreras
felipe.contre...@gmail.com wrote:
On Mon, Jan 16, 2012 at 11:24 PM, Rob Clark rob.cl...@linaro.org wrote:
On Mon, Jan 16, 2012 at 2:37 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
On Mon, Jan 16, 2012 at 7:01 PM, Rob Clark rob.cl...@linaro.org wrote:
On Mon, Jan 16, 2012 at 10:59 AM, Felipe Contreras
felipe.contre...@gmail.com wrote:
#if defined(CONFIG_DRM_OMAP) || defined(CONFIG_DRM_OMAP_MODULE)
extern void omapdrm_reserve_vram(void);
#else
static inline void omapdrm_reserve_vram(void) { }
#endif
Like how it's done with DSP stuff.
right, but then you'd miss the omapdrm_reserve_vram() call at startup..
Why?
I guess drm.o is ending up in a module, but the code that calls that
(in common.c) is not in a module, so you get an unresolved symbol at
link
Right... But that code can be moved as well. Just like with the bridge.
Hmm, looks like for dsp bridge the memory reservation is moved to
devices.c. Although I'm not entirely sure how that is better... and
there is precedent for both approaches, ie. omapfb works like omapdrm,
and tidspbridge works as you suggest.
seems a bit bikeshed'ish to me
I will send a patch to fix omapfb, perhaps after that it will be a bit
more clear how it should be done :) (if it gets accepted)
--
Felipe Contreras
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] omap2+: add drm device
On Mon, Jan 16, 2012 at 7:01 PM, Rob Clark wrote:
> On Mon, Jan 16, 2012 at 10:59 AM, Felipe Contreras
> wrote:
>> On Mon, Jan 16, 2012 at 6:37 PM, Rob Clark wrote:
>>> On Mon, Jan 16, 2012 at 8:12 AM, Felipe Contreras
>>> wrote:
>>>> On Fri, Jan 13, 2012 at 11:19 PM, Rob Clark
>>>> wrote:
>>>>> On Fri, Jan 13, 2012 at 2:59 PM, Felipe Contreras
>>>>> wrote:
>>>>>> On Fri, Jan 13, 2012 at 10:41 PM, Rob Clark
>>>>>> wrote:
>>>>>>> diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile
>>>>>>> index 9a58461..b86e6cb 100644
>>>>>>> --- a/arch/arm/plat-omap/Makefile
>>>>>>> +++ b/arch/arm/plat-omap/Makefile
>>>>>>> @@ -4,7 +4,7 @@
>>>>>>>
>>>>>>> ?# Common support
>>>>>>> ?obj-y := common.o sram.o clock.o devices.o dma.o mux.o \
>>>>>>> - ? ? ? ?usb.o fb.o counter_32k.o
>>>>>>> + ? ? ? ?usb.o fb.o counter_32k.o drm.o
>>>>>>
>>>>>> Should be something like this:
>>>>>> obj-$(CONFIG_DRM_OMAP) += drm.o
>>>>>
>>>>> btw, how does that work if CONFIG_DRM_OMAP=m? ?Do you end up w/ a
>>>>> plat-omap module?
>>>>
>>>> Yes, and platform drivers are supposed to be loaded automatically at
>>>> boot-time by udev (or similar).
>>>
>>> oh, but this won't work, because common.c has to call
>>> omapdrm_reserve_vram() (similar to how omapfb_reserve_sdram_memblock()
>>> works).. so I think it has to stay the way it is in this patch.
>>
>> #if defined(CONFIG_DRM_OMAP) || defined(CONFIG_DRM_OMAP_MODULE)
>> extern void omapdrm_reserve_vram(void);
>> #else
>> static inline void omapdrm_reserve_vram(void) { }
>> #endif
>>
>> Like how it's done with DSP stuff.
>
> right, but then you'd miss the omapdrm_reserve_vram() call at startup..
Why?
--
Felipe Contreras
[PATCH 1/2] omap2+: add drm device
On Mon, Jan 16, 2012 at 6:37 PM, Rob Clark wrote:
> On Mon, Jan 16, 2012 at 8:12 AM, Felipe Contreras
> wrote:
>> On Fri, Jan 13, 2012 at 11:19 PM, Rob Clark wrote:
>>> On Fri, Jan 13, 2012 at 2:59 PM, Felipe Contreras
>>> wrote:
>>>> On Fri, Jan 13, 2012 at 10:41 PM, Rob Clark
>>>> wrote:
>>>>> diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile
>>>>> index 9a58461..b86e6cb 100644
>>>>> --- a/arch/arm/plat-omap/Makefile
>>>>> +++ b/arch/arm/plat-omap/Makefile
>>>>> @@ -4,7 +4,7 @@
>>>>>
>>>>> ?# Common support
>>>>> ?obj-y := common.o sram.o clock.o devices.o dma.o mux.o \
>>>>> - ? ? ? ?usb.o fb.o counter_32k.o
>>>>> + ? ? ? ?usb.o fb.o counter_32k.o drm.o
>>>>
>>>> Should be something like this:
>>>> obj-$(CONFIG_DRM_OMAP) += drm.o
>>>
>>> btw, how does that work if CONFIG_DRM_OMAP=m? ?Do you end up w/ a
>>> plat-omap module?
>>
>> Yes, and platform drivers are supposed to be loaded automatically at
>> boot-time by udev (or similar).
>
> oh, but this won't work, because common.c has to call
> omapdrm_reserve_vram() (similar to how omapfb_reserve_sdram_memblock()
> works).. so I think it has to stay the way it is in this patch.
#if defined(CONFIG_DRM_OMAP) || defined(CONFIG_DRM_OMAP_MODULE)
extern void omapdrm_reserve_vram(void);
#else
static inline void omapdrm_reserve_vram(void) { }
#endif
Like how it's done with DSP stuff.
--
Felipe Contreras
[PATCH 1/2] omap2+: add drm device
On Fri, Jan 13, 2012 at 11:19 PM, Rob Clark wrote: > On Fri, Jan 13, 2012 at 2:59 PM, Felipe Contreras > wrote: >> On Fri, Jan 13, 2012 at 10:41 PM, Rob Clark wrote: >>> diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile >>> index 9a58461..b86e6cb 100644 >>> --- a/arch/arm/plat-omap/Makefile >>> +++ b/arch/arm/plat-omap/Makefile >>> @@ -4,7 +4,7 @@ >>> >>> ?# Common support >>> ?obj-y := common.o sram.o clock.o devices.o dma.o mux.o \ >>> - ? ? ? ?usb.o fb.o counter_32k.o >>> + ? ? ? ?usb.o fb.o counter_32k.o drm.o >> >> Should be something like this: >> obj-$(CONFIG_DRM_OMAP) += drm.o > > btw, how does that work if CONFIG_DRM_OMAP=m? ?Do you end up w/ a > plat-omap module? Yes, and platform drivers are supposed to be loaded automatically at boot-time by udev (or similar). -- Felipe Contreras
Re: [PATCH 1/2] omap2+: add drm device
On Fri, Jan 13, 2012 at 11:19 PM, Rob Clark rob.cl...@linaro.org wrote: On Fri, Jan 13, 2012 at 2:59 PM, Felipe Contreras felipe.contre...@gmail.com wrote: On Fri, Jan 13, 2012 at 10:41 PM, Rob Clark rob.cl...@linaro.org wrote: diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile index 9a58461..b86e6cb 100644 --- a/arch/arm/plat-omap/Makefile +++ b/arch/arm/plat-omap/Makefile @@ -4,7 +4,7 @@ # Common support obj-y := common.o sram.o clock.o devices.o dma.o mux.o \ - usb.o fb.o counter_32k.o + usb.o fb.o counter_32k.o drm.o Should be something like this: obj-$(CONFIG_DRM_OMAP) += drm.o btw, how does that work if CONFIG_DRM_OMAP=m? Do you end up w/ a plat-omap module? Yes, and platform drivers are supposed to be loaded automatically at boot-time by udev (or similar). -- Felipe Contreras ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] omap2+: add drm device
On Mon, Jan 16, 2012 at 6:37 PM, Rob Clark rob.cl...@linaro.org wrote:
On Mon, Jan 16, 2012 at 8:12 AM, Felipe Contreras
felipe.contre...@gmail.com wrote:
On Fri, Jan 13, 2012 at 11:19 PM, Rob Clark rob.cl...@linaro.org wrote:
On Fri, Jan 13, 2012 at 2:59 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
On Fri, Jan 13, 2012 at 10:41 PM, Rob Clark rob.cl...@linaro.org wrote:
diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile
index 9a58461..b86e6cb 100644
--- a/arch/arm/plat-omap/Makefile
+++ b/arch/arm/plat-omap/Makefile
@@ -4,7 +4,7 @@
# Common support
obj-y := common.o sram.o clock.o devices.o dma.o mux.o \
- usb.o fb.o counter_32k.o
+ usb.o fb.o counter_32k.o drm.o
Should be something like this:
obj-$(CONFIG_DRM_OMAP) += drm.o
btw, how does that work if CONFIG_DRM_OMAP=m? Do you end up w/ a
plat-omap module?
Yes, and platform drivers are supposed to be loaded automatically at
boot-time by udev (or similar).
oh, but this won't work, because common.c has to call
omapdrm_reserve_vram() (similar to how omapfb_reserve_sdram_memblock()
works).. so I think it has to stay the way it is in this patch.
#if defined(CONFIG_DRM_OMAP) || defined(CONFIG_DRM_OMAP_MODULE)
extern void omapdrm_reserve_vram(void);
#else
static inline void omapdrm_reserve_vram(void) { }
#endif
Like how it's done with DSP stuff.
--
Felipe Contreras
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] omap2+: add drm device
On Mon, Jan 16, 2012 at 7:01 PM, Rob Clark rob.cl...@linaro.org wrote:
On Mon, Jan 16, 2012 at 10:59 AM, Felipe Contreras
felipe.contre...@gmail.com wrote:
On Mon, Jan 16, 2012 at 6:37 PM, Rob Clark rob.cl...@linaro.org wrote:
On Mon, Jan 16, 2012 at 8:12 AM, Felipe Contreras
felipe.contre...@gmail.com wrote:
On Fri, Jan 13, 2012 at 11:19 PM, Rob Clark rob.cl...@linaro.org wrote:
On Fri, Jan 13, 2012 at 2:59 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
On Fri, Jan 13, 2012 at 10:41 PM, Rob Clark rob.cl...@linaro.org wrote:
diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile
index 9a58461..b86e6cb 100644
--- a/arch/arm/plat-omap/Makefile
+++ b/arch/arm/plat-omap/Makefile
@@ -4,7 +4,7 @@
# Common support
obj-y := common.o sram.o clock.o devices.o dma.o mux.o \
- usb.o fb.o counter_32k.o
+ usb.o fb.o counter_32k.o drm.o
Should be something like this:
obj-$(CONFIG_DRM_OMAP) += drm.o
btw, how does that work if CONFIG_DRM_OMAP=m? Do you end up w/ a
plat-omap module?
Yes, and platform drivers are supposed to be loaded automatically at
boot-time by udev (or similar).
oh, but this won't work, because common.c has to call
omapdrm_reserve_vram() (similar to how omapfb_reserve_sdram_memblock()
works).. so I think it has to stay the way it is in this patch.
#if defined(CONFIG_DRM_OMAP) || defined(CONFIG_DRM_OMAP_MODULE)
extern void omapdrm_reserve_vram(void);
#else
static inline void omapdrm_reserve_vram(void) { }
#endif
Like how it's done with DSP stuff.
right, but then you'd miss the omapdrm_reserve_vram() call at startup..
Why?
--
Felipe Contreras
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] omap2+: add drm device
On Fri, Jan 13, 2012 at 10:41 PM, Rob Clark wrote: > diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile > index 9a58461..b86e6cb 100644 > --- a/arch/arm/plat-omap/Makefile > +++ b/arch/arm/plat-omap/Makefile > @@ -4,7 +4,7 @@ > > ?# Common support > ?obj-y := common.o sram.o clock.o devices.o dma.o mux.o \ > - ? ? ? ?usb.o fb.o counter_32k.o > + ? ? ? ?usb.o fb.o counter_32k.o drm.o Should be something like this: obj-$(CONFIG_DRM_OMAP) += drm.o No? -- Felipe Contreras
Re: [PATCH 1/2] omap2+: add drm device
On Fri, Jan 13, 2012 at 10:41 PM, Rob Clark rob.cl...@linaro.org wrote: diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile index 9a58461..b86e6cb 100644 --- a/arch/arm/plat-omap/Makefile +++ b/arch/arm/plat-omap/Makefile @@ -4,7 +4,7 @@ # Common support obj-y := common.o sram.o clock.o devices.o dma.o mux.o \ - usb.o fb.o counter_32k.o + usb.o fb.o counter_32k.o drm.o Should be something like this: obj-$(CONFIG_DRM_OMAP) += drm.o No? -- Felipe Contreras ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/omap: avoid aquiring mutex in atomic context (v2)
On Fri, Dec 16, 2011 at 7:34 PM, Rob Clark wrote:
> --- a/drivers/staging/omapdrm/omap_gem.c
> +++ b/drivers/staging/omapdrm/omap_gem.c
> @@ -538,10 +538,22 @@ int omap_gem_roll(struct drm_gem_object *obj, uint32_t
> roll)
> ? ? ? ? ? ? ? ?return -EINVAL;
> ? ? ? ?}
>
> - ? ? ? mutex_lock(>dev->struct_mutex);
> -
> ? ? ? ?omap_obj->roll = roll;
>
> + ? ? ? if (in_atomic() || mutex_is_locked(>dev->struct_mutex)) {
> + ? ? ? ? ? ? ? /* this can get called from fbcon in atomic context.. so
> + ? ? ? ? ? ? ? ?* just ignore it and wait for next time called from
> + ? ? ? ? ? ? ? ?* interruptible context to update the PAT.. the result
> + ? ? ? ? ? ? ? ?* may be that user sees wrap-around instead of scrolling
> + ? ? ? ? ? ? ? ?* momentarily on the screen. ?If we wanted to be fancier
> + ? ? ? ? ? ? ? ?* we could perhaps schedule some workqueue work at this
> + ? ? ? ? ? ? ? ?* point.
> + ? ? ? ? ? ? ? ?*/
Multi-line comments are supposed to be of the form:
/*
* Foo.
*/
> + ? ? ? ? ? ? ? return 0;
> + ? ? ? }
> +
> + ? ? ? mutex_lock(>dev->struct_mutex);
> +
> ? ? ? ?/* if we aren't mapped yet, we don't need to do anything */
> ? ? ? ?if (omap_obj->block) {
> ? ? ? ? ? ? ? ?struct page **pages;
> --
Cheers.
--
Felipe Contreras
Re: [PATCH] drm/omap: avoid aquiring mutex in atomic context (v2)
On Fri, Dec 16, 2011 at 7:34 PM, Rob Clark rob.cl...@linaro.org wrote:
--- a/drivers/staging/omapdrm/omap_gem.c
+++ b/drivers/staging/omapdrm/omap_gem.c
@@ -538,10 +538,22 @@ int omap_gem_roll(struct drm_gem_object *obj, uint32_t
roll)
return -EINVAL;
}
- mutex_lock(obj-dev-struct_mutex);
-
omap_obj-roll = roll;
+ if (in_atomic() || mutex_is_locked(obj-dev-struct_mutex)) {
+ /* this can get called from fbcon in atomic context.. so
+ * just ignore it and wait for next time called from
+ * interruptible context to update the PAT.. the result
+ * may be that user sees wrap-around instead of scrolling
+ * momentarily on the screen. If we wanted to be fancier
+ * we could perhaps schedule some workqueue work at this
+ * point.
+ */
Multi-line comments are supposed to be of the form:
/*
* Foo.
*/
+ return 0;
+ }
+
+ mutex_lock(obj-dev-struct_mutex);
+
/* if we aren't mapped yet, we don't need to do anything */
if (omap_obj-block) {
struct page **pages;
--
Cheers.
--
Felipe Contreras
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Proposal for a low-level Linux display framework
On Thu, Sep 15, 2011 at 9:58 PM, Alan Cox wrote: >> One of my biggest problems with KMS is that it has (naturally) a lot more >> complexity than the fb API which leads to instability. Basically it's very > > It shouldn't do - and a sample of one (your machine) is not a > statistically valid set. Fb is pretty much ununsable in contrast on my > main box, but that's not a statistically valid sample either. > > I'm not that convinced by the complexity either. For a simple video card > setup such as those that the fb layer can kind of cope with (ie linear > buffer, simple mode changes, no client rendering, no vblank flipping, > limited mode management, no serious multi-head) a DRM driver is also > pretty tiny and simple. That's not true, many drivers work around the lack of features in the fb API by providing custom interfaces. For example, in omapfb it's possible to use the overlays from user-space, configure some YUV format, do vsink, and multipages just fine: https://github.com/felipec/gst-omapfb/blob/master/omapfb.c It's perfect to render video clips. Of course, it would be even better if those custom interfaces were merged into the fb API. -- Felipe Contreras
Re: Proposal for a low-level Linux display framework
On Thu, Sep 15, 2011 at 9:58 PM, Alan Cox a...@lxorguk.ukuu.org.uk wrote: One of my biggest problems with KMS is that it has (naturally) a lot more complexity than the fb API which leads to instability. Basically it's very It shouldn't do - and a sample of one (your machine) is not a statistically valid set. Fb is pretty much ununsable in contrast on my main box, but that's not a statistically valid sample either. I'm not that convinced by the complexity either. For a simple video card setup such as those that the fb layer can kind of cope with (ie linear buffer, simple mode changes, no client rendering, no vblank flipping, limited mode management, no serious multi-head) a DRM driver is also pretty tiny and simple. That's not true, many drivers work around the lack of features in the fb API by providing custom interfaces. For example, in omapfb it's possible to use the overlays from user-space, configure some YUV format, do vsink, and multipages just fine: https://github.com/felipec/gst-omapfb/blob/master/omapfb.c It's perfect to render video clips. Of course, it would be even better if those custom interfaces were merged into the fb API. -- Felipe Contreras ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] Standardize YUV support in the fbdev API
On Wed, May 18, 2011 at 1:07 AM, Laurent Pinchart laurent.pinch...@ideasonboard.com wrote: I need to implement support for a YUV frame buffer in an fbdev driver. As the fbdev API doesn't support this out of the box, I've spent a couple of days reading fbdev (and KMS) code and thinking about how we could cleanly add YUV support to the API. I'd like to share my findings and thoughts, and hopefully receive some comments back. The terms 'format', 'pixel format', 'frame buffer format' and 'data format' will be used interchangeably in this e-mail. They all refer to the way pixels are stored in memory, including both the representation of a pixel as integer values and the layout of those integer values in memory. This is a great proposal. It was about time! The third solution has my preference. Comments and feedback will be appreciated. I will then work on a proof of concept and submit patches. I also would prefer the third solution. I don't think there's much difference from the user-space point of view, and a new ioctl would be cleaner. Also the v4l2 fourcc's should do. Cheers. -- Felipe Contreras ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
