* Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
From: Darius Augulis @ 2012-03-16 7:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <001d01cd0315$935cc720$ba165560$%han@samsung.com>
On Fri, Mar 16, 2012 at 3:39 AM, Jingoo Han <jg1.han@samsung.com> wrote:
>> > So, your code can be removed.
>>
>> I don't think so. It does not break anything yet. One who make
>> changes, should ensure backwards compatibility, at least talking about
>> functionality and hardware (LCD) support, which was added few months
>> ago. Remember, we talk about kernel parameters line - now it lets
>> bootloader to select correct LCD size. After your changes, board with
>> 7" LCD wont show anything.
>
> [CC'ed Tushar Behera]
>
> Yes, your code is working. However, your code is bug.
it's not a bug, because it's working like it was intended to. it was
reviewed and accepted by maintainers before merging to main line.
you do not have rights to drop it because you don't like it.
You are doing changes - please do it correctly and do not break
existing functionality.
btw, you still not answered my question: why these two s3c_fb_pd_win
structures in mach-mini6410.c is a problem? They are declared on the
stack, but platform data uses only single one. mini6410 rewrites its
s3c-fb platform data with data from one of these two structures
dynamically. Why it's a bug? You want to remove this dynamic selection
which could be leaved there with reworked framework too.
> 'struct s3c_fb_pd_win' is used not for LCD, but for windows of FIMD IP. So, 'struct s3c_fb_pd_win' should not be used to select LCD.
> The mach-aquila.c uses two 'struct s3c_fb_pd_win' like you, however 'struct s3c_fb_pd_win' don't make problem with Thomas's
> patchset. It is because mach-aquila.c use 'struct s3c_fb_pd_win' for only windows of FIMD IP.
> We don't have to responsibility to keep wrong code. If you have kept the rule, Thomas's patch would not make the compatibility
> problem with your code. Why we should keep aberration such as your code? If you want to select two LCDs, please find other way.
>
>
>>
>> Darius A.
>
^ permalink raw reply
* RE: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
From: Jingoo Han @ 2012-03-16 8:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAEYLNCrGCmb5EJjURLY=EGfM=4N54MirNkCCyQRxqKEN3Eka8Q@mail.gmail.com>
> -----Original Message-----
> From: Darius Augulis [mailto:augulis.darius@gmail.com]
> Sent: Friday, March 16, 2012 4:47 PM
> To: Jingoo Han
> Cc: Thomas Abraham; linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-arm-
> kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; ben-linux@fluff.org; patches@linaro.org;
> kgene.kim@samsung.com; Kyungmin Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter
> Korsgaard; Maurus Cuelenaere; Tushar Behera
> Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
>
> On Fri, Mar 16, 2012 at 3:39 AM, Jingoo Han <jg1.han@samsung.com> wrote:
>
> >> > So, your code can be removed.
> >>
> >> I don't think so. It does not break anything yet. One who make
> >> changes, should ensure backwards compatibility, at least talking about
> >> functionality and hardware (LCD) support, which was added few months
> >> ago. Remember, we talk about kernel parameters line - now it lets
> >> bootloader to select correct LCD size. After your changes, board with
> >> 7" LCD wont show anything.
> >
> > [CC'ed Tushar Behera]
> >
> > Yes, your code is working. However, your code is bug.
>
> it's not a bug, because it's working like it was intended to. it was
> reviewed and accepted by maintainers before merging to main line.
> you do not have rights to drop it because you don't like it.
> You are doing changes - please do it correctly and do not break
> existing functionality.
> btw, you still not answered my question: why these two s3c_fb_pd_win
> structures in mach-mini6410.c is a problem? They are declared on the
> stack, but platform data uses only single one. mini6410 rewrites its
> s3c-fb platform data with data from one of these two structures
> dynamically. Why it's a bug? You want to remove this dynamic selection
> which could be leaved there with reworked framework too.
It's just bug.
struct s3c_fb_pd_win should be used for windows of FIMD IP.
It should not be used for selecting multi-LCDs.
struct s3c_fb_pd_win should include information of windows of FIMD IP, s3c_fb_pd_win[0], s3c_fb_pd_win[1] represents window 0,
window 1 of FIMD IP. However, your code includes information of LCD, it means that struct s3c_fb_pd_win represents 4.3" LCD, 7.0"
LCD. It is wrong usage.
>
> > 'struct s3c_fb_pd_win' is used not for LCD, but for windows of FIMD IP. So, 'struct s3c_fb_pd_win'
> should not be used to select LCD.
> > The mach-aquila.c uses two 'struct s3c_fb_pd_win' like you, however 'struct s3c_fb_pd_win' don't make
> problem with Thomas's
> > patchset. It is because mach-aquila.c use 'struct s3c_fb_pd_win' for only windows of FIMD IP.
> > We don't have to responsibility to keep wrong code. If you have kept the rule, Thomas's patch would
> not make the compatibility
> > problem with your code. Why we should keep aberration such as your code? If you want to select two
> LCDs, please find other way.
> >
> >
> >>
> >> Darius A.
> >
^ permalink raw reply
* Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
From: Darius Augulis @ 2012-03-16 9:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <002d01cd034b$c56f2b00$504d8100$%han@samsung.com>
On Fri, Mar 16, 2012 at 10:07 AM, Jingoo Han <jg1.han@samsung.com> wrote:
>
>
>> -----Original Message-----
>> From: Darius Augulis [mailto:augulis.darius@gmail.com]
>> Sent: Friday, March 16, 2012 4:47 PM
>> To: Jingoo Han
>> Cc: Thomas Abraham; linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-arm-
>> kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; ben-linux@fluff.org; patches@linaro.org;
>> kgene.kim@samsung.com; Kyungmin Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter
>> Korsgaard; Maurus Cuelenaere; Tushar Behera
>> Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
>>
>> On Fri, Mar 16, 2012 at 3:39 AM, Jingoo Han <jg1.han@samsung.com> wrote:
>>
>> it's not a bug, because it's working like it was intended to. it was
>> reviewed and accepted by maintainers before merging to main line.
>> you do not have rights to drop it because you don't like it.
>> You are doing changes - please do it correctly and do not break
>> existing functionality.
>> btw, you still not answered my question: why these two s3c_fb_pd_win
>> structures in mach-mini6410.c is a problem? They are declared on the
>> stack, but platform data uses only single one. mini6410 rewrites its
>> s3c-fb platform data with data from one of these two structures
>> dynamically. Why it's a bug? You want to remove this dynamic selection
>> which could be leaved there with reworked framework too.
>
>
> It's just bug.
>
> struct s3c_fb_pd_win should be used for windows of FIMD IP.
> It should not be used for selecting multi-LCDs.
why not? s3c_fb_pd_win contains timing values which depend directly on
LCD size and resolution.
Please look into the code again - mini6410 doesn't use platform data
so select different LCD sizes.
It has only single window in platform data - exactly what you want.
Now tell my, why you are arguing, that this platform data cannot be
rewritten dynamically by board setup routines at kernel startup time?
Many ARM architectures are doing the same - takes kernel command line
argument to select LCD and passes necessary data for its fb drivers
via platform data structures. It not a bug, it's correct approach to
support different sizes of LCD display for the same board.
You are simply lazy to deal with little bit different code of mini6410
and real6410 because they have support for multiple LCDs and other
s3c-fb boards doesn't.
This is a reason why you want to drop it and not because it's a bug.
Hope other maintainers and code reviewers understand this situation correctly.
>
> struct s3c_fb_pd_win should include information of windows of FIMD IP, s3c_fb_pd_win[0], s3c_fb_pd_win[1] represents window 0,
> window 1 of FIMD IP. However, your code includes information of LCD, it means that struct s3c_fb_pd_win represents 4.3" LCD, 7.0"
> LCD. It is wrong usage.
>
>
>>
>> > 'struct s3c_fb_pd_win' is used not for LCD, but for windows of FIMD IP. So, 'struct s3c_fb_pd_win'
>> should not be used to select LCD.
>> > The mach-aquila.c uses two 'struct s3c_fb_pd_win' like you, however 'struct s3c_fb_pd_win' don't make
>> problem with Thomas's
>> > patchset. It is because mach-aquila.c use 'struct s3c_fb_pd_win' for only windows of FIMD IP.
>> > We don't have to responsibility to keep wrong code. If you have kept the rule, Thomas's patch would
>> not make the compatibility
>> > problem with your code. Why we should keep aberration such as your code? If you want to select two
>> LCDs, please find other way.
>> >
>> >
>> >>
>> >> Darius A.
>> >
>
^ permalink raw reply
* RE: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
From: Jingoo Han @ 2012-03-16 9:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAEYLNCpfQ0cYb4efw6PZNnKjQ2rF25rj5Ed=i884LJXHAwigBw@mail.gmail.com>
> -----Original Message-----
> From: Darius Augulis [mailto:augulis.darius@gmail.com]
> Sent: Friday, March 16, 2012 6:16 PM
> To: Jingoo Han
> Cc: Thomas Abraham; linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-arm-
> kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; ben-linux@fluff.org; patches@linaro.org;
> kgene.kim@samsung.com; Kyungmin Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter
> Korsgaard; Maurus Cuelenaere; Tushar Behera
> Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
>
> On Fri, Mar 16, 2012 at 10:07 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Darius Augulis [mailto:augulis.darius@gmail.com]
> >> Sent: Friday, March 16, 2012 4:47 PM
> >> To: Jingoo Han
> >> Cc: Thomas Abraham; linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-arm-
> >> kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; ben-linux@fluff.org;
> patches@linaro.org;
> >> kgene.kim@samsung.com; Kyungmin Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter
> >> Korsgaard; Maurus Cuelenaere; Tushar Behera
> >> Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
> >>
> >> On Fri, Mar 16, 2012 at 3:39 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> >>
> >> it's not a bug, because it's working like it was intended to. it was
> >> reviewed and accepted by maintainers before merging to main line.
> >> you do not have rights to drop it because you don't like it.
> >> You are doing changes - please do it correctly and do not break
> >> existing functionality.
> >> btw, you still not answered my question: why these two s3c_fb_pd_win
> >> structures in mach-mini6410.c is a problem? They are declared on the
> >> stack, but platform data uses only single one. mini6410 rewrites its
> >> s3c-fb platform data with data from one of these two structures
> >> dynamically. Why it's a bug? You want to remove this dynamic selection
> >> which could be leaved there with reworked framework too.
> >
> >
> > It's just bug.
> >
> > struct s3c_fb_pd_win should be used for windows of FIMD IP.
> > It should not be used for selecting multi-LCDs.
>
> why not? s3c_fb_pd_win contains timing values which depend directly on
> LCD size and resolution.
> Please look into the code again - mini6410 doesn't use platform data
> so select different LCD sizes.
> It has only single window in platform data - exactly what you want.
> Now tell my, why you are arguing, that this platform data cannot be
> rewritten dynamically by board setup routines at kernel startup time?
> Many ARM architectures are doing the same - takes kernel command line
> argument to select LCD and passes necessary data for its fb drivers
> via platform data structures. It not a bug, it's correct approach to
> support different sizes of LCD display for the same board.
Hey, it is not my point.
To take kernel command line is not problem to support multiple LCDs.
My point is that s3c_fb_pd_win should not include variable LCD option.
So, if you want to support multiple LCDs, you should use another variable,
instead of using s3c_fb_pd_win[0] and s3c_fb_pd_win[1] as 4.3" LCD, 7.0" LCD.
You use s3c_fb_pd_win[0], s3c_fb_pd_win[1] as 4.3" LCD, 7.0" LCD.
However, s3c_fb_pd_win[0], s3c_fb_pd_win[1] means the windows of FIMD IP.
> You are simply lazy to deal with little bit different code of mini6410
> and real6410 because they have support for multiple LCDs and other
> s3c-fb boards doesn't.
> This is a reason why you want to drop it and not because it's a bug.
> Hope other maintainers and code reviewers understand this situation correctly.
>
> >
> > struct s3c_fb_pd_win should include information of windows of FIMD IP, s3c_fb_pd_win[0],
> s3c_fb_pd_win[1] represents window 0,
> > window 1 of FIMD IP. However, your code includes information of LCD, it means that struct
> s3c_fb_pd_win represents 4.3" LCD, 7.0"
> > LCD. It is wrong usage.
> >
> >
> >>
> >> > 'struct s3c_fb_pd_win' is used not for LCD, but for windows of FIMD IP. So, 'struct s3c_fb_pd_win'
> >> should not be used to select LCD.
> >> > The mach-aquila.c uses two 'struct s3c_fb_pd_win' like you, however 'struct s3c_fb_pd_win' don't
> make
> >> problem with Thomas's
> >> > patchset. It is because mach-aquila.c use 'struct s3c_fb_pd_win' for only windows of FIMD IP.
> >> > We don't have to responsibility to keep wrong code. If you have kept the rule, Thomas's patch would
> >> not make the compatibility
> >> > problem with your code. Why we should keep aberration such as your code? If you want to select two
> >> LCDs, please find other way.
> >> >
> >> >
> >> >>
> >> >> Darius A.
> >> >
> >
^ permalink raw reply
* Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
From: Darius Augulis @ 2012-03-16 10:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <002f01cd0359$eb718330$c2548990$%han@samsung.com>
On Fri, Mar 16, 2012 at 11:48 AM, Jingoo Han <jg1.han@samsung.com> wrote:
>> > It's just bug.
>> >
>> > struct s3c_fb_pd_win should be used for windows of FIMD IP.
>> > It should not be used for selecting multi-LCDs.
>>
>> why not? s3c_fb_pd_win contains timing values which depend directly on
>> LCD size and resolution.
>> Please look into the code again - mini6410 doesn't use platform data
>> so select different LCD sizes.
>> It has only single window in platform data - exactly what you want.
>> Now tell my, why you are arguing, that this platform data cannot be
>> rewritten dynamically by board setup routines at kernel startup time?
>> Many ARM architectures are doing the same - takes kernel command line
>> argument to select LCD and passes necessary data for its fb drivers
>> via platform data structures. It not a bug, it's correct approach to
>> support different sizes of LCD display for the same board.
>
>
> Hey, it is not my point.
> To take kernel command line is not problem to support multiple LCDs.
>
> My point is that s3c_fb_pd_win should not include variable LCD option.
> So, if you want to support multiple LCDs, you should use another variable,
> instead of using s3c_fb_pd_win[0] and s3c_fb_pd_win[1] as 4.3" LCD, 7.0" LCD.
But I use s3c_fb_pd_win only as container to hold timing values in
board setup code.
s3c-fb platform data has only single window and does not perform any
LCD selection.
What is wrong here? You suggest to declare some custom structure to
hold these timing values? What's the point?
>
> You use s3c_fb_pd_win[0], s3c_fb_pd_win[1] as 4.3" LCD, 7.0" LCD.
> However, s3c_fb_pd_win[0], s3c_fb_pd_win[1] means the windows of FIMD IP.
There is no multiple windows in platform data! driver sees only single
window. And this wont change in any way if you drop LCD selection from
board setup code.
Driver will see the same single window. So - your are arguing, that
board setup code sets platform data in bad way. I don't accept your
opinion, because tens of boards are doing the same and nobody call it
bug.
>
^ permalink raw reply
* Re: [PATCH 2/2] OMAPDSS: DISPC: Handle synclost errors in OMAP3
From: Tomi Valkeinen @ 2012-03-16 10:23 UTC (permalink / raw)
To: Ville Syrjälä, Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <20120315230446.GO17132@sci.fi>
On Fri, 2012-03-16 at 01:04 +0200, Ville Syrjälä wrote:
> On Thu, Mar 15, 2012 at 05:18:52PM +0530, Chandrabhanu Mahapatra wrote:
> > In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
> > and timing related limitations leading to SYNCLOST errors. Whenever the image
> > window is moved towards the right of the screen SYNCLOST errors become
> > frequent. Checks have been implemented to see that DISPC driver rejects
> > configuration exceeding above limitations.
> >
> > This code was successfully tested on OMAP3. This code is written based on code
> > written by Ville Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
> > Syrjälä <ville.syrjala@nokia.com> had added checks for video overlay horizontal
> > timing and DISPC horizontal blanking length limitations.
> >
> > Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> > ---
> > drivers/video/omap2/dss/dispc.c | 67 +++++++++++++++++++++++++++++++++++++--
> > 1 files changed, 64 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> > index 5a1963e..ebfa613 100644
> > --- a/drivers/video/omap2/dss/dispc.c
> > +++ b/drivers/video/omap2/dss/dispc.c
> > @@ -1622,6 +1622,58 @@ static void calc_dma_rotation_offset(u8 rotation, bool mirror,
> > }
> > }
> >
> > +static int check_horiz_timing(enum omap_channel channel, u16 pos_x,
> > + u16 width, u16 height, u16 out_width, u16 out_height)
> > +{
> > + int DS = DIV_ROUND_UP(height, out_height);
> > + struct omap_dss_device *dssdev = dispc_mgr_get_device(channel);
> > + struct omap_video_timings t = dssdev->panel.timings;
> > + int pcd = REG_GET(DISPC_DIVISORo(channel), 7, 0);
>
> pcd doesn't exist for TV out, which is the reason why you see
> 'lclk/pclk' ratio used instead in the harmattan kernel.
I'm surprised you still remember these things =). And even more
surprised that you actually bothered to review it, thanks for that.
Chandrabhanu, do you have an omap3 board with composite/svideo tv-out?
It'd be good to test on that also, as, like Ville points out, the above
code can't work for tv-out. Hmm, then again, it shouldn't work for HDMI
on OMAP4 either. Did you test with HDMI?
Or is the above code called for TV-manager at all? If not, perhaps it
could be named *_lcd or something. Or perhaps have a BUG_ON(channel !lcd1 or lcd2);
Tomi
^ permalink raw reply
* Re: [PATCH 1/2] OMAPDSS: DISPC: Enable predecimation
From: Mahapatra, Chandrabhanu @ 2012-03-16 12:11 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: linux-fbdev, linux-omap
In-Reply-To: <20120315222811.GN17132@sci.fi>
On Fri, Mar 16, 2012 at 3:58 AM, Ville Syrjälä <syrjala@sci.fi> wrote:
> On Thu, Mar 15, 2012 at 05:18:28PM +0530, Chandrabhanu Mahapatra wrote:
>> In OMAP3 and OMAP4, the DISPC Scaler can downscale an image up to 4 times, and
>> up to 2 times in OMAP2. However, with predecimation, the image can be reduced
>> to 16 times by fetching only the necessary pixels in memory. Then this
>> predecimated image can be downscaled further by the DISPC scaler.
>
> Now, where does that number 16 come from? IIRC the hardware can skip
> basically any number of pixels/rows. I certainly didn't add any such
> limit to the code in the harmattan kernel, and distinctly remember
> being able to downscale the N9/N950 UI even down to 1 pixel size :)
>
Yes, you are right. I have done vertical predecimation to a size of 2
lines beyond which it failed.
This limitation of 16 is actually on horizontal predecimation as was
mentioned by the hardware guys. Let me explain what we once discussed
with Archit.
The pipeline is configured to use a burst of size 8 * 128 bits and DSS
uses 8 L3 clock cycles to fetch it. So, burst consists of 8 mini
bursts of 16 bytes each.
Now, there are two possible situations:
1) Each mini burst has some valid pixels, so all 16 bytes are fetched
by DISPC DMA and the uneeded pixels are discarded. This case can be
shown as
pixelinc -1 + bpp =< 16
2) Some mini bursts don't have any valid pixels, so these will not be
fecthed by the DISPC DMA. So, L3 interconnect may handover the bus
over so some other initiator and inturn delay the fecthing of pixels
leading to UNDERFLOWS. This case can be shown as
pixellinc -1 + bpp > 16.
To avoid these UNDERFLOWS we better have a limitation of 16 on
horizontal predecimation. Anyways, 16 times predecimation followed by
2/4 times downscaling (may it be horizontal or vertical) by DISPC
SCALER is more than enough for any usecase required.
>> Based on the downscaling required, a prior calculation of predecimation values
>> for width and height of an image is done. Since, Predecimation reduces quality
>> of an image higher priorty is given to DISPC Scaler for downscaling.
>>
>> This code was successfully tested on OMAP2, OMAP3 and OMAP4. Horizontal and
>> vertical predecimation worked fine except for some synclost errors due to
>> undocumented errata in OMAP3 which are fixed later and skewed images were seen
>> on OMAP2 and OMAP3 during horizontal predecimation which will be addressed in
>> the future patches.
>
> All the rotation offset calculations still look suspiciously different
> to what is in the harmattan kernel. I remember that the original code
> was quite broken, and I fixed a lot of things when I was implementing
> pre-decimation and some rotation stuff for the N9/N950. Too bad I never
> managed to push that stuff upstream...
>
A number of variables are different in mainline kernel from that used
in your code. I did fix some of the rotation code to get the best
results. I will consider your suggestions for further improvement.
> --
> Ville Syrjälä
> syrjala@sci.fi
> http://www.sci.fi/~syrjala/
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH 2/2] OMAPDSS: DISPC: Handle synclost errors in OMAP3
From: Mahapatra, Chandrabhanu @ 2012-03-16 14:18 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: linux-fbdev, linux-omap
In-Reply-To: <20120315230446.GO17132@sci.fi>
On Fri, Mar 16, 2012 at 4:34 AM, Ville Syrjälä <syrjala@sci.fi> wrote:
> On Thu, Mar 15, 2012 at 05:18:52PM +0530, Chandrabhanu Mahapatra wrote:
>> In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
>> and timing related limitations leading to SYNCLOST errors. Whenever the image
>> window is moved towards the right of the screen SYNCLOST errors become
>> frequent. Checks have been implemented to see that DISPC driver rejects
>> configuration exceeding above limitations.
>>
>> This code was successfully tested on OMAP3. This code is written based on code
>> written by Ville Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
>> Syrjälä <ville.syrjala@nokia.com> had added checks for video overlay horizontal
>> timing and DISPC horizontal blanking length limitations.
>>
>> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>> ---
>> drivers/video/omap2/dss/dispc.c | 67 +++++++++++++++++++++++++++++++++++++--
>> 1 files changed, 64 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
>> index 5a1963e..ebfa613 100644
>> --- a/drivers/video/omap2/dss/dispc.c
>> +++ b/drivers/video/omap2/dss/dispc.c
>> @@ -1622,6 +1622,58 @@ static void calc_dma_rotation_offset(u8 rotation, bool mirror,
>> }
>> }
>>
>> +static int check_horiz_timing(enum omap_channel channel, u16 pos_x,
>> + u16 width, u16 height, u16 out_width, u16 out_height)
>> +{
>> + int DS = DIV_ROUND_UP(height, out_height);
>> + struct omap_dss_device *dssdev = dispc_mgr_get_device(channel);
>> + struct omap_video_timings t = dssdev->panel.timings;
>> + int pcd = REG_GET(DISPC_DIVISORo(channel), 7, 0);
>
> pcd doesn't exist for TV out, which is the reason why you see
> 'lclk/pclk' ratio used instead in the harmattan kernel.
>
Yes, you are right. It's a very important point which I had missed.
> Another thing which still seems to be wrong in the upstream code is the
> use of fclk in the scaling checks. It should be checking lclk instead.
Thanks for sighting this out. I will implement it in the future.
>> + unsigned long nonactive, val, blank;
>> + static const u8 limits[3] = { 8, 10, 20 };
>> + int i;
>> +
>> + nonactive = t.x_res + t.hfp + t.hsw + t.hbp - out_width;
>> +
>> + /*
>> + * Atleast DS-2 lines must have already been fetched
>> + * before the display active video period starts.
>> + */
>> + val = (nonactive - pos_x) * pcd;
>> + DSSDBG("(nonactive - pos_x) * pcd = %lu,"
>> + " max(0, DS - 2) * width = %d\n",
>> + val, max(0, DS - 2) * width);
>> + if (val < max(0, DS - 2) * width)
>> + return -EINVAL;
>> +
>> + /*
>> + * Only one line can be fetched during the overlay active
>> + * period, the rest have to be fetched during the inactive
>> + * period.
>> + */
>> + val = nonactive * pcd;
>> + DSSDBG("nonactive * pcd = %lu, max(0, DS - 1) * width = %d\n",
>> + val, max(0, DS - 1) * width);
>> + if (val < max(0, DS - 1) * width)
>> + return -EINVAL;
>> +
>> + /*
>> + * Atleast Ceil(DS) lines should have been loaded during
>> + * PPL (screen width) + blanking period.
>> + */
>> + i = 0;
>> + if (out_height < height)
>> + i++;
>> + if (out_width < width)
>> + i++;
>> + blank = (t.hbp + t.hsw + t.hfp) * pcd;
>> + DSSDBG("blanking period + ppl = %lu (limit = %u)\n", blank, limits[i]);
>> + if (blank <= limits[i])
>> + return -EINVAL;
>
> The comment and code here are totally out of sync. IIRC the hblank
> length check came directly from the TRM. I have no idea if it's correct
> for more recent OMAPs, or if it was just copy pasted from the
> old TRM when the TRM got adjusted for more recent chips. That is why I
> stuck it into a separate function in harmattan.
>
This patch is only for OMAP3 and this errata is present upto OMAP4 ES1
which we are currently not supporting.
> --
> Ville Syrjälä
> syrjala@sci.fi
> http://www.sci.fi/~syrjala/
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH 2/2] OMAPDSS: DISPC: Handle synclost errors in OMAP3
From: Mahapatra, Chandrabhanu @ 2012-03-16 14:27 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1331893411.1653.5.camel@lappy>
2012/3/16 Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On Fri, 2012-03-16 at 01:04 +0200, Ville Syrjälä wrote:
>> On Thu, Mar 15, 2012 at 05:18:52PM +0530, Chandrabhanu Mahapatra wrote:
>> > In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
>> > and timing related limitations leading to SYNCLOST errors. Whenever the image
>> > window is moved towards the right of the screen SYNCLOST errors become
>> > frequent. Checks have been implemented to see that DISPC driver rejects
>> > configuration exceeding above limitations.
>> >
>> > This code was successfully tested on OMAP3. This code is written based on code
>> > written by Ville Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
>> > Syrjälä <ville.syrjala@nokia.com> had added checks for video overlay horizontal
>> > timing and DISPC horizontal blanking length limitations.
>> >
>> > Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>> > ---
>> > drivers/video/omap2/dss/dispc.c | 67 +++++++++++++++++++++++++++++++++++++--
>> > 1 files changed, 64 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
>> > index 5a1963e..ebfa613 100644
>> > --- a/drivers/video/omap2/dss/dispc.c
>> > +++ b/drivers/video/omap2/dss/dispc.c
>> > @@ -1622,6 +1622,58 @@ static void calc_dma_rotation_offset(u8 rotation, bool mirror,
>> > }
>> > }
>> >
>> > +static int check_horiz_timing(enum omap_channel channel, u16 pos_x,
>> > + u16 width, u16 height, u16 out_width, u16 out_height)
>> > +{
>> > + int DS = DIV_ROUND_UP(height, out_height);
>> > + struct omap_dss_device *dssdev = dispc_mgr_get_device(channel);
>> > + struct omap_video_timings t = dssdev->panel.timings;
>> > + int pcd = REG_GET(DISPC_DIVISORo(channel), 7, 0);
>>
>> pcd doesn't exist for TV out, which is the reason why you see
>> 'lclk/pclk' ratio used instead in the harmattan kernel.
>
> I'm surprised you still remember these things =). And even more
> surprised that you actually bothered to review it, thanks for that.
>
> Chandrabhanu, do you have an omap3 board with composite/svideo tv-out?
> It'd be good to test on that also, as, like Ville points out, the above
> code can't work for tv-out. Hmm, then again, it shouldn't work for HDMI
> on OMAP4 either. Did you test with HDMI?
>
> Or is the above code called for TV-manager at all? If not, perhaps it
> could be named *_lcd or something. Or perhaps have a BUG_ON(channel !> lcd1 or lcd2);
>
Yes, I do have a Beagle board and I have tested on HDMI which works
without synclost errors upto 640 * 480 resolution.
The above errata exists only upto OMAP4430 ES1 which is currently not
supported. Please refer to errata 2.14 Downscaling Limitations in
errata sheet.
This patch is only for Omap3.
> Tomi
>
>
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Problem with framebuffer mmap on platforms with large addressing
From: Dmitry Eremin-Solenikov @ 2012-03-17 16:04 UTC (permalink / raw)
To: linux-fbdev-devel, linuxppc-dev, Florian Tobias Schandinat,
Josh Boyer, Matt Porter
Hello,
I'm trying to make framebuffer to work on PPC460EX board (canyonlands).
The peculiarity of this platform is the fact that it has
sizeof(unsigned long) = 4,
but physical address on it is 36 bits width. It is a common to various pieces
of the code to expect that unsigned long variable is able to contain physical
address. Most of those places are easy to fix.
The problem I'm stuck with is a fb_mmap() code. To find a right memory to map
it uses information from struct fb_fix_screeninfo provided by the driver.
This structure uses two unsigned long fields to hold physical addresses
(smem_start and mmio_start). It would be easy to change that structure
to use phys_addr_t instead of unsigned long, but this structure is a part
of userspace ABI. It is returned to userspace on FBIOGET_FSCREENINFO ioctl.
And now I'm stuck with it.
In my driver code I have just overwritten the fb_mmap function with
driver-private
fb_mmap callback supporting 64-bit addressing, but this doesn't look like
a generic and correct solution.
What is the best way to fix this problem? Should we break ABI with the goal
of correctness? Should we add new FBIOGET_FSCREENINFO2, which will
return a correct structure with phys_addr_t (or simply u64) fields and make
FBIOGET_FSCREENINFO a wrapper returning partially bogus structure
(with smem_start and mmio_start fields being truncated to just unsigned long)?
What would developers recommend?
Thank you.
--
With best wishes
Dmitry
^ permalink raw reply
* Fwd: Problem with framebuffer mmap on platforms with large addressing
From: Dmitry Eremin-Solenikov @ 2012-03-17 17:03 UTC (permalink / raw)
To: linux-fbdev
Hello,
I'm trying to make framebuffer to work on PPC460EX board (canyonlands).
The peculiarity of this platform is the fact that it has
sizeof(unsigned long) = 4,
but physical address on it is 36 bits width. It is a common to various pieces
of the code to expect that unsigned long variable is able to contain physical
address. Most of those places are easy to fix.
The problem I'm stuck with is a fb_mmap() code. To find a right memory to map
it uses information from struct fb_fix_screeninfo provided by the driver.
This structure uses two unsigned long fields to hold physical addresses
(smem_start and mmio_start). It would be easy to change that structure
to use phys_addr_t instead of unsigned long, but this structure is a part
of userspace ABI. It is returned to userspace on FBIOGET_FSCREENINFO ioctl.
And now I'm stuck with it.
In my driver code I have just overwritten the fb_mmap function with
driver-private
fb_mmap callback supporting 64-bit addressing, but this doesn't look like
a generic and correct solution.
What is the best way to fix this problem? Should we break ABI with the goal
of correctness? Should we add new FBIOGET_FSCREENINFO2, which will
return a correct structure with phys_addr_t (or simply u64) fields and make
FBIOGET_FSCREENINFO a wrapper returning partially bogus structure
(with smem_start and mmio_start fields being truncated to just unsigned long)?
What would developers recommend?
Thank you.
--
With best wishes
Dmitry
--
With best wishes
Dmitry
^ permalink raw reply
* Re: Problem with framebuffer mmap on platforms with large addressing
From: Benjamin Herrenschmidt @ 2012-03-18 0:46 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: linux-fbdev-devel, Florian Tobias Schandinat, Tony Breeds,
linuxppc-dev
In-Reply-To: <CALT56yO05UBY6=KqK94=_sC4hd3ranj5J5CXOAo5hSjVAdkgkA@mail.gmail.com>
On Sat, 2012-03-17 at 20:04 +0400, Dmitry Eremin-Solenikov wrote:
> Hello,
>
> I'm trying to make framebuffer to work on PPC460EX board (canyonlands).
>
> The peculiarity of this platform is the fact that it has
> sizeof(unsigned long) = 4,
> but physical address on it is 36 bits width. It is a common to various pieces
> of the code to expect that unsigned long variable is able to contain physical
> address. Most of those places are easy to fix.
Yes. In fact, Tony (CC) has some patches to fix a lot of the DRM
infrastructure (we have radeon KMS working on a similar platform).
> The problem I'm stuck with is a fb_mmap() code. To find a right memory to map
> it uses information from struct fb_fix_screeninfo provided by the driver.
> This structure uses two unsigned long fields to hold physical addresses
> (smem_start and mmio_start). It would be easy to change that structure
> to use phys_addr_t instead of unsigned long, but this structure is a part
> of userspace ABI. It is returned to userspace on FBIOGET_FSCREENINFO ioctl.
> And now I'm stuck with it.
It's an old problem, which I think we described a while back on the
list. Back then the conclusion was to make a new version with a proper
__u64, a new ioctl to access is, and a "compatible" ioctl that blanks
the address fields (or fails) if they contain a value >32-bit.
We just never got to actually implement it.
In fact, we could make the new structure such that it doesn't break
userspace compatibility with 64-bit architectures at all, ie, the "new"
and "compat" ioctl could remain entirely equivalent on 64-bit.
> In my driver code I have just overwritten the fb_mmap function with
> driver-private
> fb_mmap callback supporting 64-bit addressing, but this doesn't look like
> a generic and correct solution.
>
> What is the best way to fix this problem? Should we break ABI with the goal
> of correctness? Should we add new FBIOGET_FSCREENINFO2, which will
> return a correct structure with phys_addr_t (or simply u64) fields and make
> FBIOGET_FSCREENINFO a wrapper returning partially bogus structure
> (with smem_start and mmio_start fields being truncated to just unsigned long)?
> What would developers recommend?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH] Added backlight driver for Acer Aspire 4736
From: Pradeep Subrahmanion @ 2012-03-18 5:22 UTC (permalink / raw)
To: joeyli
Cc: Matthew Garrett, rpurdie, FlorianSchandinat, akpm, linux-fbdev,
linux-kernel
In-Reply-To: <1331798740.10557.245.camel@linux-s257.site>
On Thu, 2012-03-15 at 16:05 +0800, joeyli wrote:
> Hi Pradeep,
>
> 於 三，2012-03-14 於 11:47 +0530，Pradeep Subrahmanion 提到：
> > On Wed, Mar 14, 2012 at 11:21 AM, joeyli <jlee@suse.com> wrote:
> > > 於 三，2012-03-14 於 08:13 +0530，Pradeep Subrahmanion 提到：
> > >> Hi Joey ,
> > >>
> > >> > Per my understood, EC firmware should change brightness but didn't do
> > >> > that, another
> > >> > way is touch i915 register in _BCM.
> > >>
> > >> how do we do this ? you mean change the _BCM implementation ?
> > >
> > > "BIOS guy" should do something like this:
> > >
> > > Method (AINT, 2, NotSerialized)
> > > {
> > > ...
> > > If (LEqual (Arg0, One))
> > > {
> > > Store (Divide (Multiply (Arg1, 0xFF), 0x64, ), BCLP)
> > > Or (BCLP, 0x80000000, BCLP) <== touch BCLP register
> > > Store (0x02, ASLC)
> > > }
> > >
> > > Method (_BCM, 1, NotSerialized)
> > > {
> > > If (LAnd (LGreaterEqual (Arg0, Zero), LLessEqual (Arg0, 0x64)))
> > > {
> > > AINT (One, Arg0) <== call AINT method
> > > Store (Arg0, BRTL)
> > > }
> > > }
> > >
> > >
> > > Just for reference, they should do that when EC didn't wire to
> > > backlight.
> > >
> > >> >
> > >> > Acer machine provide a broken _BCM implementation and they didn't test
> > >> > it.
> > >> >
> > >> > > > > By ' ACPI interface' , I mean 'acpi_video0' inside the
> > >> > > > > /sys/class/backlight. I havn't tried the /sys/class/backlight interface
> > >> > > > > directly . I will try that also.
> > >> > > >
> > >> > > > So writing values into /sys/class/backlight/acpi_video0/brightness does
> > >> > > > nothing?
> > >> > >
> > >> > >
> > >> > > No change in value when writing
> > >> > > to /sys/class/backlight/acpi_video0/brightness.
> > >> > >
> > >> > > Another thing is that when i did boot with acpi_backlight = 'acer_wmi' ,
> > >> > > in new kernel (3.3.0-rc7) , it shows following messages ,
> > >> > >
> > >> > > [ 8.350825] wmi: Mapper loaded
> > >> > > [ 10.363975] acer_wmi: Acer Laptop ACPI-WMI Extras
> > >> > > [ 10.396186] acer_wmi: Function bitmap for Communication Device: 0x91
> > >> > > [ 10.396385] acer_wmi: Brightness must be controlled by generic video
> > >> > > driver
> > >> > >
> > >> > > Also there was no interface inside /sys/class/backlight for acer_wmi.
> > >> > >
> > >> >
> > >> > Yes, acer_wmi support backlight control with AMW0 interface, your
> > >> > machine didn't have AMW0 interface.
> > >> >
> > >> > Normally, backlight should control by standard acpi interface.
> > >> >
> > >> > > I also tried writing directly to Embedded controller register .But no
> > >> > > change.
> > >> >
> > >> > The machine has broken _BCM method, because EC should do something after
> > >> > _BCM changed EC register.
> > >>
> > >> Thanks ,
> > >>
> > >> Pradeep Subrahmanion
> > >
> > > Why they didn't find _BCM not work?
> > >
> > > My guess is:
> > >
> > > Because the backlight control is through WDDM driver on Windows platform
> > > but not through standard ACPI method _BCM. They only test Windows
> > > platform, so, they didn't find _BCM broken.
> > >
> > > And, they also didn't really follow Microsoft WDDM spec:
> > >
> > > http://msdn.microsoft.com/en-us/windows/hardware/gg487382.aspx
> > >
> > > Per spec,
> > > ODM should keep _BCM works fine for any other OS didn't support WDDM
> > > driver, but they didn't.
> > >
> > > At last year, I told Acer PM one time for this issue, they said will
> > > check but finally didn't response me.
> > >
> >
> > >
> > > Thanks
> > > Joey Lee
> > >
> >
> > So touching the PCI LBB register is the only feasible solution now
> > (even though it may not be a clean method) ?
> >
> > Thanks,
> > Pradeep Subrahmanion
>
> That will be better leave LBB register only touched by i915 driver.
>
> If 'acpi_backlight=vendor' works to you, maybe we can add a quirk to
> video_detect.c.
> You can try the following patch.
Thanks . I tried your patch .acpi_backlight=vendor allows me to control
brightness with hot key.But there are problems with it like increasing
brightness after maximum level causes blank screen.So I am trying it
sort it out.
Thank you ,
Pradeep Subrahmanion
>
>
> Thanks a lot!
> Joey Lee
>
>
> >From 038bd3c4e53b7195f34e9d46c999b8dcb279da5e Mon Sep 17 00:00:00 2001
> From: "Lee, Chun-Yi" <jlee@suse.com>
> Date: Thu, 15 Mar 2012 16:03:45 +0800
> Subject: [PATCH] acer-wmi: Add quirk table for video backlight vendor mode
>
> Add quirk table for video backlight vendor mode
>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> ---
> drivers/acpi/video_detect.c | 27 +++++++++++++++++++++++++++
> 1 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> index f3f0fe7..acb15d6 100644
> --- a/drivers/acpi/video_detect.c
> +++ b/drivers/acpi/video_detect.c
> @@ -132,6 +132,32 @@ find_video(acpi_handle handle, u32 lvl, void *context, void **rv)
> return AE_OK;
> }
>
> +static int video_set_backlight_vendor(const struct dmi_system_id *d)
> +{
> + acpi_video_support |= ACPI_VIDEO_BACKLIGHT_DMI_VENDOR;
> + return 0;
> +}
> +
> +static const struct dmi_system_id video_vendor_dmi_table[] = {
> + {
> + .callback = video_set_backlight_vendor,
> + .ident = "Acer Aspire 4736",
> + .matches = {
> + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 4736"),
> + },
> + },
> + {
> + .callback = video_set_backlight_vendor,
> + .ident = "Acer TravelMate 4750",
> + .matches = {
> + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 4750"),
> + },
> + },
> + {}
> +};
> +
> /*
> * Returns the video capabilities of a specific ACPI graphics device
> *
> @@ -164,6 +190,7 @@ long acpi_video_get_capabilities(acpi_handle graphics_handle)
> * ACPI_VIDEO_BACKLIGHT_DMI_VENDOR;
> *}
> */
> + dmi_check_system(video_vendor_dmi_table);
> } else {
> status = acpi_bus_get_device(graphics_handle, &tmp_dev);
> if (ACPI_FAILURE(status)) {
^ permalink raw reply
* Re: [PATCH] Added backlight driver for Acer Aspire 4736
From: Pradeep Subrahmanion @ 2012-03-18 5:24 UTC (permalink / raw)
To: joeyli
Cc: Matthew Garrett, rpurdie, FlorianSchandinat, akpm, linux-fbdev,
linux-kernel
In-Reply-To: <1331798740.10557.245.camel@linux-s257.site>
On Thu, 2012-03-15 at 16:05 +0800, joeyli wrote:
> Hi Pradeep,
>
> 於 三，2012-03-14 於 11:47 +0530，Pradeep Subrahmanion 提到：
> > On Wed, Mar 14, 2012 at 11:21 AM, joeyli <jlee@suse.com> wrote:
> > > 於 三，2012-03-14 於 08:13 +0530，Pradeep Subrahmanion 提到：
> > >> Hi Joey ,
> > >>
> > >> > Per my understood, EC firmware should change brightness but didn't do
> > >> > that, another
> > >> > way is touch i915 register in _BCM.
> > >>
> > >> how do we do this ? you mean change the _BCM implementation ?
> > >
> > > "BIOS guy" should do something like this:
> > >
> > > Method (AINT, 2, NotSerialized)
> > > {
> > > ...
> > > If (LEqual (Arg0, One))
> > > {
> > > Store (Divide (Multiply (Arg1, 0xFF), 0x64, ), BCLP)
> > > Or (BCLP, 0x80000000, BCLP) <== touch BCLP register
> > > Store (0x02, ASLC)
> > > }
> > >
> > > Method (_BCM, 1, NotSerialized)
> > > {
> > > If (LAnd (LGreaterEqual (Arg0, Zero), LLessEqual (Arg0, 0x64)))
> > > {
> > > AINT (One, Arg0) <== call AINT method
> > > Store (Arg0, BRTL)
> > > }
> > > }
> > >
> > >
> > > Just for reference, they should do that when EC didn't wire to
> > > backlight.
> > >
> > >> >
> > >> > Acer machine provide a broken _BCM implementation and they didn't test
> > >> > it.
> > >> >
> > >> > > > > By ' ACPI interface' , I mean 'acpi_video0' inside the
> > >> > > > > /sys/class/backlight. I havn't tried the /sys/class/backlight interface
> > >> > > > > directly . I will try that also.
> > >> > > >
> > >> > > > So writing values into /sys/class/backlight/acpi_video0/brightness does
> > >> > > > nothing?
> > >> > >
> > >> > >
> > >> > > No change in value when writing
> > >> > > to /sys/class/backlight/acpi_video0/brightness.
> > >> > >
> > >> > > Another thing is that when i did boot with acpi_backlight = 'acer_wmi' ,
> > >> > > in new kernel (3.3.0-rc7) , it shows following messages ,
> > >> > >
> > >> > > [ 8.350825] wmi: Mapper loaded
> > >> > > [ 10.363975] acer_wmi: Acer Laptop ACPI-WMI Extras
> > >> > > [ 10.396186] acer_wmi: Function bitmap for Communication Device: 0x91
> > >> > > [ 10.396385] acer_wmi: Brightness must be controlled by generic video
> > >> > > driver
> > >> > >
> > >> > > Also there was no interface inside /sys/class/backlight for acer_wmi.
> > >> > >
> > >> >
> > >> > Yes, acer_wmi support backlight control with AMW0 interface, your
> > >> > machine didn't have AMW0 interface.
> > >> >
> > >> > Normally, backlight should control by standard acpi interface.
> > >> >
> > >> > > I also tried writing directly to Embedded controller register .But no
> > >> > > change.
> > >> >
> > >> > The machine has broken _BCM method, because EC should do something after
> > >> > _BCM changed EC register.
> > >>
> > >> Thanks ,
> > >>
> > >> Pradeep Subrahmanion
> > >
> > > Why they didn't find _BCM not work?
> > >
> > > My guess is:
> > >
> > > Because the backlight control is through WDDM driver on Windows platform
> > > but not through standard ACPI method _BCM. They only test Windows
> > > platform, so, they didn't find _BCM broken.
> > >
> > > And, they also didn't really follow Microsoft WDDM spec:
> > >
> > > http://msdn.microsoft.com/en-us/windows/hardware/gg487382.aspx
> > >
> > > Per spec,
> > > ODM should keep _BCM works fine for any other OS didn't support WDDM
> > > driver, but they didn't.
> > >
> > > At last year, I told Acer PM one time for this issue, they said will
> > > check but finally didn't response me.
> > >
> >
> > >
> > > Thanks
> > > Joey Lee
> > >
> >
> > So touching the PCI LBB register is the only feasible solution now
> > (even though it may not be a clean method) ?
> >
> > Thanks,
> > Pradeep Subrahmanion
>
> That will be better leave LBB register only touched by i915 driver.
>
> If 'acpi_backlight=vendor' works to you, maybe we can add a quirk to
> video_detect.c.
> You can try the following patch.
Thanks . I tried your patch .acpi_backlight=vendor allows me to control
brightness with hot key.But there are problems with it like increasing
brightness after maximum level causes blank screen.So I am trying it
sort it out.
Thank you ,
Pradeep Subrahmanion
>
>
> Thanks a lot!
> Joey Lee
>
>
> >From 038bd3c4e53b7195f34e9d46c999b8dcb279da5e Mon Sep 17 00:00:00 2001
> From: "Lee, Chun-Yi" <jlee@suse.com>
> Date: Thu, 15 Mar 2012 16:03:45 +0800
> Subject: [PATCH] acer-wmi: Add quirk table for video backlight vendor mode
>
> Add quirk table for video backlight vendor mode
>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> ---
> drivers/acpi/video_detect.c | 27 +++++++++++++++++++++++++++
> 1 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> index f3f0fe7..acb15d6 100644
> --- a/drivers/acpi/video_detect.c
> +++ b/drivers/acpi/video_detect.c
> @@ -132,6 +132,32 @@ find_video(acpi_handle handle, u32 lvl, void *context, void **rv)
> return AE_OK;
> }
>
> +static int video_set_backlight_vendor(const struct dmi_system_id *d)
> +{
> + acpi_video_support |= ACPI_VIDEO_BACKLIGHT_DMI_VENDOR;
> + return 0;
> +}
> +
> +static const struct dmi_system_id video_vendor_dmi_table[] = {
> + {
> + .callback = video_set_backlight_vendor,
> + .ident = "Acer Aspire 4736",
> + .matches = {
> + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 4736"),
> + },
> + },
> + {
> + .callback = video_set_backlight_vendor,
> + .ident = "Acer TravelMate 4750",
> + .matches = {
> + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 4750"),
> + },
> + },
> + {}
> +};
> +
> /*
> * Returns the video capabilities of a specific ACPI graphics device
> *
> @@ -164,6 +190,7 @@ long acpi_video_get_capabilities(acpi_handle graphics_handle)
> * ACPI_VIDEO_BACKLIGHT_DMI_VENDOR;
> *}
> */
> + dmi_check_system(video_vendor_dmi_table);
> } else {
> status = acpi_bus_get_device(graphics_handle, &tmp_dev);
> if (ACPI_FAILURE(status)) {
^ permalink raw reply
* Re: Problem with framebuffer mmap on platforms with large addressing
From: Dmitry Eremin-Solenikov @ 2012-03-18 14:04 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linux-fbdev, Florian Tobias Schandinat, Tony Breeds, linuxppc-dev
In-Reply-To: <1332031585.3105.197.camel@pasglop>
On Sun, Mar 18, 2012 at 4:46 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Sat, 2012-03-17 at 20:04 +0400, Dmitry Eremin-Solenikov wrote:
>> Hello,
>>
>> I'm trying to make framebuffer to work on PPC460EX board (canyonlands).
>>
>> The peculiarity of this platform is the fact that it has
>> sizeof(unsigned long) = 4,
>> but physical address on it is 36 bits width. It is a common to various pieces
>> of the code to expect that unsigned long variable is able to contain physical
>> address. Most of those places are easy to fix.
>
> Yes. In fact, Tony (CC) has some patches to fix a lot of the DRM
> infrastructure (we have radeon KMS working on a similar platform).
That is interesting! Are those patches published or otherwise available
somewhere? We are also very interested in enabling Canyonlands
with Radeon KMS!
>
>> The problem I'm stuck with is a fb_mmap() code. To find a right memory to map
>> it uses information from struct fb_fix_screeninfo provided by the driver.
>> This structure uses two unsigned long fields to hold physical addresses
>> (smem_start and mmio_start). It would be easy to change that structure
>> to use phys_addr_t instead of unsigned long, but this structure is a part
>> of userspace ABI. It is returned to userspace on FBIOGET_FSCREENINFO ioctl.
>> And now I'm stuck with it.
>
> It's an old problem, which I think we described a while back on the
> list. Back then the conclusion was to make a new version with a proper
> __u64, a new ioctl to access is, and a "compatible" ioctl that blanks
> the address fields (or fails) if they contain a value >32-bit.
>
> We just never got to actually implement it.
I see. I will try to prepare patches.
>
> In fact, we could make the new structure such that it doesn't break
> userspace compatibility with 64-bit architectures at all, ie, the "new"
> and "compat" ioctl could remain entirely equivalent on 64-bit.
I remember stuff about compat_ioctl, but I have never used/implemented
that. Are there any details of requirements for the structures being passed?
--
With best wishes
Dmitry
^ permalink raw reply
* Re: Problem with framebuffer mmap on platforms with large addressing
From: Benjamin Herrenschmidt @ 2012-03-18 20:49 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: linux-fbdev, Florian Tobias Schandinat, Tony Breeds, linuxppc-dev
In-Reply-To: <CALT56yOaxfWYwxvBDbo9dpWo9y37dO0iGge4x6kciZp1oXXHVw@mail.gmail.com>
On Sun, 2012-03-18 at 18:04 +0400, Dmitry Eremin-Solenikov wrote:
> On Sun, Mar 18, 2012 at 4:46 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Sat, 2012-03-17 at 20:04 +0400, Dmitry Eremin-Solenikov wrote:
> >> Hello,
> >>
> >> I'm trying to make framebuffer to work on PPC460EX board (canyonlands).
> >>
> >> The peculiarity of this platform is the fact that it has
> >> sizeof(unsigned long) = 4,
> >> but physical address on it is 36 bits width. It is a common to various pieces
> >> of the code to expect that unsigned long variable is able to contain physical
> >> address. Most of those places are easy to fix.
> >
> > Yes. In fact, Tony (CC) has some patches to fix a lot of the DRM
> > infrastructure (we have radeon KMS working on a similar platform).
>
> That is interesting! Are those patches published or otherwise available
> somewhere? We are also very interested in enabling Canyonlands
> with Radeon KMS!
You will run into additional problems with 460 due to the fact that it's
not cache coherent for DMA. Tony patches don't address that part of the
problem (they were used on a 476 based platform).
> > In fact, we could make the new structure such that it doesn't break
> > userspace compatibility with 64-bit architectures at all, ie, the "new"
> > and "compat" ioctl could remain entirely equivalent on 64-bit.
>
> I remember stuff about compat_ioctl, but I have never used/implemented
> that. Are there any details of requirements for the structures being passed?
In that specific case, I meant something else. IE. The old ioctl could
remain unchanged, and the new ioctl make the same as the old one on
64-bit platforms.
Cheers,
Ben.
^ permalink raw reply
* RE: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
From: Jingoo Han @ 2012-03-18 23:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAEYLNCpJU_5uVa-jo5HUNY8Av6eYaiow1gEZtHCiQZ5oKkGoMQ@mail.gmail.com>
> -----Original Message-----
> From: linux-samsung-soc-owner@vger.kernel.org [mailto:linux-samsung-soc-owner@vger.kernel.org] On Behalf
> Of Darius Augulis
> Sent: Friday, March 16, 2012 7:19 PM
> To: Jingoo Han
> Cc: Thomas Abraham; linux-fbdev@vger.kernel.org; FlorianSchandinat@gmx.de; linux-arm-
> kernel@lists.infradead.org; linux-samsung-soc@vger.kernel.org; ben-linux@fluff.org; patches@linaro.org;
> kgene.kim@samsung.com; Kyungmin Park; JeongHyeon Kim; Heiko Stuebner; Kwangwoo Lee; Mark Brown; Peter
> Korsgaard; Maurus Cuelenaere; Tushar Behera
> Subject: Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
>
> On Fri, Mar 16, 2012 at 11:48 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> >> > It's just bug.
> >> >
> >> > struct s3c_fb_pd_win should be used for windows of FIMD IP.
> >> > It should not be used for selecting multi-LCDs.
> >>
> >> why not? s3c_fb_pd_win contains timing values which depend directly on
> >> LCD size and resolution.
> >> Please look into the code again - mini6410 doesn't use platform data
> >> so select different LCD sizes.
> >> It has only single window in platform data - exactly what you want.
> >> Now tell my, why you are arguing, that this platform data cannot be
> >> rewritten dynamically by board setup routines at kernel startup time?
> >> Many ARM architectures are doing the same - takes kernel command line
> >> argument to select LCD and passes necessary data for its fb drivers
> >> via platform data structures. It not a bug, it's correct approach to
> >> support different sizes of LCD display for the same board.
> >
> >
> > Hey, it is not my point.
> > To take kernel command line is not problem to support multiple LCDs.
> >
> > My point is that s3c_fb_pd_win should not include variable LCD option.
> > So, if you want to support multiple LCDs, you should use another variable,
> > instead of using s3c_fb_pd_win[0] and s3c_fb_pd_win[1] as 4.3" LCD, 7.0" LCD.
>
> But I use s3c_fb_pd_win only as container to hold timing values in
> board setup code.
> s3c-fb platform data has only single window and does not perform any
> LCD selection.
> What is wrong here? You suggest to declare some custom structure to
> hold these timing values? What's the point?
>
> >
> > You use s3c_fb_pd_win[0], s3c_fb_pd_win[1] as 4.3" LCD, 7.0" LCD.
> > However, s3c_fb_pd_win[0], s3c_fb_pd_win[1] means the windows of FIMD IP.
>
> There is no multiple windows in platform data! driver sees only single
> window. And this wont change in any way if you drop LCD selection from
> board setup code.
> Driver will see the same single window. So - your are arguing, that
> board setup code sets platform data in bad way. I don't accept your
> opinion, because tens of boards are doing the same and nobody call it
> bug.
Because there are not multiple windows and driver sees only single window as you mentiond,
Only 'struct s3c_fb_pd_win mini6410_fb_win[0]' should be used and
'struct s3c_fb_pd_win mini6410_fb_win[1]' should not be used.
s3c_fb_pd_win[0] means window 0 of FIMD IP, s3c_fb_pd_win[1] means window 1 of FIMD IP.
Therefore, if mini6410_fb_win[0] and mini6410_fb_win[1] are included, it means that
2 windows of FIMD IP are used in single LCD.
If you want to support multiple LCDs, you should use struct s3c_fb_platdata[0], struct s3c_fb_platdata[1],
because 'struct s3c_fb_platdata' means LCD panel.
If you understand the purpose of s3c_fb_platdata and s3c_fb_pd_win, you should do, at least, as below.
static struct s3c_fb_pd_win mini6410_fb_lcd0_win[] = {
{
.win_mode = { /* 4.3" 480x272 */
.left_margin = 3,
.right_margin = 2,
.upper_margin = 1,
.lower_margin = 1,
.hsync_len = 40,
.vsync_len = 1,
.xres = 480,
.yres = 272,
},
.max_bpp = 32,
.default_bpp = 16,
},
};
static struct s3c_fb_platdata mini6410_lcd_pdata[0] __initdata = {
.setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
.win[0] = &mini6410_fb_lcd0_win[0],
.vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
.vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
};
static struct s3c_fb_pd_win mini6410_fb_lcd1_win[] = {
{
.win_mode = { /* 7.0" 800x480 */
.left_margin = 8,
.right_margin = 13,
.upper_margin = 7,
.lower_margin = 5,
.hsync_len = 3,
.vsync_len = 1,
.xres = 800,
.yres = 480,
},
.max_bpp = 32,
.default_bpp = 16,
},
};
static struct s3c_fb_platdata mini6410_lcd_pdata[1] __initdata = {
.setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
.win[0] = &mini6410_fb_lcd1_win[0],
.vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
.vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
};
Each struct means:
mini6410_lcd_pdata[0]: 4.3" 480x272 LCD
mini6410_fb_lcd0_win[0]: window 0 used for 4.3" 480x272 LCD
mini6410_lcd_pdata[1]: 7.0" 800x480 LCD
mini6410_fb_lcd1_win[0]: window 0 used for 7.0" 800x480 LCD
In this case, 4.3" or 7.0" LCD can be selected using mini6410_lcd_pdata[], instead of selecting mini6410_fb_win[0] or
mini6410_fb_win[1].
My point is that 's3c_fb_platdata and s3c_fb_pd_win should be used exactly'.
>
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] x86: export 'pcibios_enabled'
From: Wang YanQing @ 2012-03-19 0:30 UTC (permalink / raw)
To: Randy Dunlap
Cc: Stephen Rothwell, linux-next, LKML, Michal Januszewski,
Florian Tobias Schandinat, linux-fbdev, x86, Andrew Morton
In-Reply-To: <4F5FAE63.3090908@xenotime.net>
On Tue, Mar 13, 2012 at 01:30:27PM -0700, Randy Dunlap wrote:
> From: Randy Dunlap <rdunlap@xenotime.net>
>
> Export 'pcibios_enabled' so that when uvesafb is built as a
> loadable module (on X86_32), the build will succeed.
>
> ERROR: "pcibios_enabled" [drivers/video/uvesafb.ko] undefined!
>
> Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
> Cc: Michal Januszewski <spock@gentoo.org>
> Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> Cc: linux-fbdev@vger.kernel.org
> Cc: x86@kernel.org
> ---
> Applies to mainline; found in linux-next.
>
> arch/x86/pci/pcbios.c | 1 +
> 1 file changed, 1 insertion(+)
>
> --- linux-next-20120313.orig/arch/x86/pci/pcbios.c
> +++ linux-next-20120313/arch/x86/pci/pcbios.c
> @@ -27,6 +27,7 @@
> #define PCIBIOS_HW_TYPE2_SPEC 0x20
>
> int pcibios_enabled;
> +EXPORT_SYMBOL(pcibios_enabled);
>
> /* According to the BIOS specification at:
> * http://members.datafast.net.au/dft0802/specs/bios21.pdf, we could
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Hi, could you resend this patch as EXPORT_SYMBOL_GPL to x86 architecture maintainer?
Looks at the MAINTAINERS for the below:
X86 ARCHITECTURE (32-BIT AND 64-BIT)
M: Thomas Gleixner <tglx@linutronix.de>
M: Ingo Molnar <mingo@redhat.com>
M: "H. Peter Anvin" <hpa@zytor.com>
M: x86@kernel.org
^ permalink raw reply
* [PATCH v2] x86: export 'pcibios_enabled' as GPL
From: Randy Dunlap @ 2012-03-19 1:03 UTC (permalink / raw)
To: Wang YanQing, Stephen Rothwell, linux-next, LKML,
Michal Januszewski, Florian Tobias Schandinat, linux-fbdev, x86,
Andrew Morton
In-Reply-To: <20120319003044.GA3744@udknight>
From: Randy Dunlap <rdunlap@xenotime.net>
Export 'pcibios_enabled' so that when uvesafb is built as a
loadable module (on X86_32), the build will succeed.
ERROR: "pcibios_enabled" [drivers/video/uvesafb.ko] undefined!
Patch v2 uses EXPORT_SYMBOL_GPL() as requested.
Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
Cc: Michal Januszewski <spock@gentoo.org>
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: linux-fbdev@vger.kernel.org
Cc: x86@kernel.org
---
arch/x86/pci/pcbios.c | 1 +
1 file changed, 1 insertion(+)
--- linux-next-20120313.orig/arch/x86/pci/pcbios.c
+++ linux-next-20120313/arch/x86/pci/pcbios.c
@@ -27,6 +27,7 @@
#define PCIBIOS_HW_TYPE2_SPEC 0x20
int pcibios_enabled;
+EXPORT_SYMBOL_GPL(pcibios_enabled);
/* According to the BIOS specification at:
* http://members.datafast.net.au/dft0802/specs/bios21.pdf, we could
^ permalink raw reply
* Re: [PATCH] Added backlight driver for Acer Aspire 4736
From: joeyli @ 2012-03-19 2:01 UTC (permalink / raw)
To: Pradeep Subrahmanion
Cc: Matthew Garrett, rpurdie, FlorianSchandinat, akpm, linux-fbdev,
linux-kernel
In-Reply-To: <1332047432.6304.13.camel@debian.Gayathri>
於 日，2012-03-18 於 10:40 +0530，Pradeep Subrahmanion 提到：
> On Thu, 2012-03-15 at 16:05 +0800, joeyli wrote:
> > Hi Pradeep,
> >
> > 於 三，2012-03-14 於 11:47 +0530，Pradeep Subrahmanion 提到：
> > > On Wed, Mar 14, 2012 at 11:21 AM, joeyli <jlee@suse.com> wrote:
> > > > 於 三，2012-03-14 於 08:13 +0530，Pradeep Subrahmanion 提到：
> > > >> Hi Joey ,
> > > >>
> > > >> > Per my understood, EC firmware should change brightness but didn't do
> > > >> > that, another
> > > >> > way is touch i915 register in _BCM.
> > > >>
> > > >> how do we do this ? you mean change the _BCM implementation ?
> > > >
> > > > "BIOS guy" should do something like this:
> > > >
> > > > Method (AINT, 2, NotSerialized)
> > > > {
> > > > ...
> > > > If (LEqual (Arg0, One))
> > > > {
> > > > Store (Divide (Multiply (Arg1, 0xFF), 0x64, ), BCLP)
> > > > Or (BCLP, 0x80000000, BCLP) <== touch BCLP register
> > > > Store (0x02, ASLC)
> > > > }
> > > >
> > > > Method (_BCM, 1, NotSerialized)
> > > > {
> > > > If (LAnd (LGreaterEqual (Arg0, Zero), LLessEqual (Arg0, 0x64)))
> > > > {
> > > > AINT (One, Arg0) <== call AINT method
> > > > Store (Arg0, BRTL)
> > > > }
> > > > }
> > > >
> > > >
> > > > Just for reference, they should do that when EC didn't wire to
> > > > backlight.
> > > >
> > > >> >
> > > >> > Acer machine provide a broken _BCM implementation and they didn't test
> > > >> > it.
> > > >> >
> > > >> > > > > By ' ACPI interface' , I mean 'acpi_video0' inside the
> > > >> > > > > /sys/class/backlight. I havn't tried the /sys/class/backlight interface
> > > >> > > > > directly . I will try that also.
> > > >> > > >
> > > >> > > > So writing values into /sys/class/backlight/acpi_video0/brightness does
> > > >> > > > nothing?
> > > >> > >
> > > >> > >
> > > >> > > No change in value when writing
> > > >> > > to /sys/class/backlight/acpi_video0/brightness.
> > > >> > >
> > > >> > > Another thing is that when i did boot with acpi_backlight = 'acer_wmi' ,
> > > >> > > in new kernel (3.3.0-rc7) , it shows following messages ,
> > > >> > >
> > > >> > > [ 8.350825] wmi: Mapper loaded
> > > >> > > [ 10.363975] acer_wmi: Acer Laptop ACPI-WMI Extras
> > > >> > > [ 10.396186] acer_wmi: Function bitmap for Communication Device: 0x91
> > > >> > > [ 10.396385] acer_wmi: Brightness must be controlled by generic video
> > > >> > > driver
> > > >> > >
> > > >> > > Also there was no interface inside /sys/class/backlight for acer_wmi.
> > > >> > >
> > > >> >
> > > >> > Yes, acer_wmi support backlight control with AMW0 interface, your
> > > >> > machine didn't have AMW0 interface.
> > > >> >
> > > >> > Normally, backlight should control by standard acpi interface.
> > > >> >
> > > >> > > I also tried writing directly to Embedded controller register .But no
> > > >> > > change.
> > > >> >
> > > >> > The machine has broken _BCM method, because EC should do something after
> > > >> > _BCM changed EC register.
> > > >>
> > > >> Thanks ,
> > > >>
> > > >> Pradeep Subrahmanion
> > > >
> > > > Why they didn't find _BCM not work?
> > > >
> > > > My guess is:
> > > >
> > > > Because the backlight control is through WDDM driver on Windows platform
> > > > but not through standard ACPI method _BCM. They only test Windows
> > > > platform, so, they didn't find _BCM broken.
> > > >
> > > > And, they also didn't really follow Microsoft WDDM spec:
> > > >
> > > > http://msdn.microsoft.com/en-us/windows/hardware/gg487382.aspx
> > > >
> > > > Per spec,
> > > > ODM should keep _BCM works fine for any other OS didn't support WDDM
> > > > driver, but they didn't.
> > > >
> > > > At last year, I told Acer PM one time for this issue, they said will
> > > > check but finally didn't response me.
> > > >
> > >
> > > >
> > > > Thanks
> > > > Joey Lee
> > > >
> > >
> > > So touching the PCI LBB register is the only feasible solution now
> > > (even though it may not be a clean method) ?
> > >
> > > Thanks,
> > > Pradeep Subrahmanion
> >
> > That will be better leave LBB register only touched by i915 driver.
> >
> > If 'acpi_backlight=vendor' works to you, maybe we can add a quirk to
> > video_detect.c.
> > You can try the following patch.
>
> Thanks . I tried your patch .acpi_backlight=vendor allows me to control
> brightness with hot key.But there are problems with it like increasing
OK, thanks for your testing, I will send out patch and add Cc. to you.
> brightness after maximum level causes blank screen.So I am trying it
> sort it out.
>
> Thank you ,
>
> Pradeep Subrahmanion
>
For the maximum level causes blank screen...
Please kindly help to identify which driver handle the brightness
change, run the following 2 commands:
# echo 5 > /sys/class/backlight/acer-wmi/brightness
# echo 5 > /sys/class/backlight/intel_backlight/brightness
Which one works to change brightness on your machine?
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
From: Darius Augulis @ 2012-03-19 7:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAJuYYwQmJvx_uGZKbAJ9uds-ciLs2VDqcifhoqB-P0M5uhO-nA@mail.gmail.com>
Hi,
On Mon, Mar 19, 2012 at 9:29 AM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> Hi Darius,
>
> On 19 March 2012 05:16, Jingoo Han <jg1.han@samsung.com> wrote:
> [...]
>
>>
>> Because there are not multiple windows and driver sees only single window as you mentiond,
>> Only 'struct s3c_fb_pd_win mini6410_fb_win[0]' should be used and
>> 'struct s3c_fb_pd_win mini6410_fb_win[1]' should not be used.
>> s3c_fb_pd_win[0] means window 0 of FIMD IP, s3c_fb_pd_win[1] means window 1 of FIMD IP.
>>
>> Therefore, if mini6410_fb_win[0] and mini6410_fb_win[1] are included, it means that
>> 2 windows of FIMD IP are used in single LCD.
>>
>> If you want to support multiple LCDs, you should use struct s3c_fb_platdata[0], struct s3c_fb_platdata[1],
>> because 'struct s3c_fb_platdata' means LCD panel.
>>
>> If you understand the purpose of s3c_fb_platdata and s3c_fb_pd_win, you should do, at least, as below.
>>
>> static struct s3c_fb_pd_win mini6410_fb_lcd0_win[] = {
>> {
>> .win_mode = { /* 4.3" 480x272 */
>> .left_margin = 3,
>> .right_margin = 2,
>> .upper_margin = 1,
>> .lower_margin = 1,
>> .hsync_len = 40,
>> .vsync_len = 1,
>> .xres = 480,
>> .yres = 272,
>> },
>> .max_bpp = 32,
>> .default_bpp = 16,
>> },
>> };
>>
>> static struct s3c_fb_platdata mini6410_lcd_pdata[0] __initdata = {
>> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
>> .win[0] = &mini6410_fb_lcd0_win[0],
>> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
>> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
>> };
>>
>> static struct s3c_fb_pd_win mini6410_fb_lcd1_win[] = {
>> {
>> .win_mode = { /* 7.0" 800x480 */
>> .left_margin = 8,
>> .right_margin = 13,
>> .upper_margin = 7,
>> .lower_margin = 5,
>> .hsync_len = 3,
>> .vsync_len = 1,
>> .xres = 800,
>> .yres = 480,
>> },
>> .max_bpp = 32,
>> .default_bpp = 16,
>> },
>> };
>>
>> static struct s3c_fb_platdata mini6410_lcd_pdata[1] __initdata = {
>> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
>> .win[0] = &mini6410_fb_lcd1_win[0],
>> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
>> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
>> };
>>
>> Each struct means:
>> mini6410_lcd_pdata[0]: 4.3" 480x272 LCD
>> mini6410_fb_lcd0_win[0]: window 0 used for 4.3" 480x272 LCD
>>
>> mini6410_lcd_pdata[1]: 7.0" 800x480 LCD
>> mini6410_fb_lcd1_win[0]: window 0 used for 7.0" 800x480 LCD
>>
>> In this case, 4.3" or 7.0" LCD can be selected using mini6410_lcd_pdata[], instead of selecting mini6410_fb_win[0] or
>> mini6410_fb_win[1].
>>
>> My point is that 's3c_fb_platdata and s3c_fb_pd_win should be used exactly'.
>
> I agree with solution which Jingoo Han has proposed. Instead of
> selecting one instance of s3c_fb_pd_win over the other at runtime, the
> features->lcd_index can be used to select one of the instances of
> platform data for s3c-fb driver.
I agree with it too. Would you make such patch based on your s3c-fb patch set?
Somebody should make it and definitely not to remove multiple LCD
suport for these boards.
>
> s3c_fb_pd_win is used to describe a window supported by the fimd
> controller. It is not meant to carry lcd timing information. The lcd
> panel timing remains the same irrespective of the window sizes. And
> this patchset is a step towards untangling window information and
> video timing.
Right now s3c_fb_pd_win contains exactly LCD timing values.
Your patch will move these timing values to struct fb_videomode.
I could make necessary patch for that too, but I think you will spend
less time doing it by yourself than syncing with me.
What do you think?
regards,
Darius
^ permalink raw reply
* Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
From: Thomas Abraham @ 2012-03-19 7:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <001501cd0561$57c642b0$0752c810$%han@samsung.com>
Hi Darius,
On 19 March 2012 05:16, Jingoo Han <jg1.han@samsung.com> wrote:
[...]
>
> Because there are not multiple windows and driver sees only single window as you mentiond,
> Only 'struct s3c_fb_pd_win mini6410_fb_win[0]' should be used and
> 'struct s3c_fb_pd_win mini6410_fb_win[1]' should not be used.
> s3c_fb_pd_win[0] means window 0 of FIMD IP, s3c_fb_pd_win[1] means window 1 of FIMD IP.
>
> Therefore, if mini6410_fb_win[0] and mini6410_fb_win[1] are included, it means that
> 2 windows of FIMD IP are used in single LCD.
>
> If you want to support multiple LCDs, you should use struct s3c_fb_platdata[0], struct s3c_fb_platdata[1],
> because 'struct s3c_fb_platdata' means LCD panel.
>
> If you understand the purpose of s3c_fb_platdata and s3c_fb_pd_win, you should do, at least, as below.
>
> static struct s3c_fb_pd_win mini6410_fb_lcd0_win[] = {
> {
> .win_mode = { /* 4.3" 480x272 */
> .left_margin = 3,
> .right_margin = 2,
> .upper_margin = 1,
> .lower_margin = 1,
> .hsync_len = 40,
> .vsync_len = 1,
> .xres = 480,
> .yres = 272,
> },
> .max_bpp = 32,
> .default_bpp = 16,
> },
> };
>
> static struct s3c_fb_platdata mini6410_lcd_pdata[0] __initdata = {
> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
> .win[0] = &mini6410_fb_lcd0_win[0],
> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
> };
>
> static struct s3c_fb_pd_win mini6410_fb_lcd1_win[] = {
> {
> .win_mode = { /* 7.0" 800x480 */
> .left_margin = 8,
> .right_margin = 13,
> .upper_margin = 7,
> .lower_margin = 5,
> .hsync_len = 3,
> .vsync_len = 1,
> .xres = 800,
> .yres = 480,
> },
> .max_bpp = 32,
> .default_bpp = 16,
> },
> };
>
> static struct s3c_fb_platdata mini6410_lcd_pdata[1] __initdata = {
> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
> .win[0] = &mini6410_fb_lcd1_win[0],
> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
> };
>
> Each struct means:
> mini6410_lcd_pdata[0]: 4.3" 480x272 LCD
> mini6410_fb_lcd0_win[0]: window 0 used for 4.3" 480x272 LCD
>
> mini6410_lcd_pdata[1]: 7.0" 800x480 LCD
> mini6410_fb_lcd1_win[0]: window 0 used for 7.0" 800x480 LCD
>
> In this case, 4.3" or 7.0" LCD can be selected using mini6410_lcd_pdata[], instead of selecting mini6410_fb_win[0] or
> mini6410_fb_win[1].
>
> My point is that 's3c_fb_platdata and s3c_fb_pd_win should be used exactly'.
I agree with solution which Jingoo Han has proposed. Instead of
selecting one instance of s3c_fb_pd_win over the other at runtime, the
features->lcd_index can be used to select one of the instances of
platform data for s3c-fb driver.
s3c_fb_pd_win is used to describe a window supported by the fimd
controller. It is not meant to carry lcd timing information. The lcd
panel timing remains the same irrespective of the window sizes. And
this patchset is a step towards untangling window information and
video timing.
(Apologizes for the delayed reply. I was out of office for last 4 days.)
Thanks,
Thomas.
^ permalink raw reply
* Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
From: Thomas Abraham @ 2012-03-19 7:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAEYLNCo0zyH9Nsf59UU-J4m=km+mKH1ym5a-9s7WthFpos6D+w@mail.gmail.com>
On 19 March 2012 13:10, Darius Augulis <augulis.darius@gmail.com> wrote:
> Hi,
>
> On Mon, Mar 19, 2012 at 9:29 AM, Thomas Abraham
> <thomas.abraham@linaro.org> wrote:
>> Hi Darius,
>>
>> On 19 March 2012 05:16, Jingoo Han <jg1.han@samsung.com> wrote:
>> [...]
>>
>>>
>>> Because there are not multiple windows and driver sees only single window as you mentiond,
>>> Only 'struct s3c_fb_pd_win mini6410_fb_win[0]' should be used and
>>> 'struct s3c_fb_pd_win mini6410_fb_win[1]' should not be used.
>>> s3c_fb_pd_win[0] means window 0 of FIMD IP, s3c_fb_pd_win[1] means window 1 of FIMD IP.
>>>
>>> Therefore, if mini6410_fb_win[0] and mini6410_fb_win[1] are included, it means that
>>> 2 windows of FIMD IP are used in single LCD.
>>>
>>> If you want to support multiple LCDs, you should use struct s3c_fb_platdata[0], struct s3c_fb_platdata[1],
>>> because 'struct s3c_fb_platdata' means LCD panel.
>>>
>>> If you understand the purpose of s3c_fb_platdata and s3c_fb_pd_win, you should do, at least, as below.
>>>
>>> static struct s3c_fb_pd_win mini6410_fb_lcd0_win[] = {
>>> {
>>> .win_mode = { /* 4.3" 480x272 */
>>> .left_margin = 3,
>>> .right_margin = 2,
>>> .upper_margin = 1,
>>> .lower_margin = 1,
>>> .hsync_len = 40,
>>> .vsync_len = 1,
>>> .xres = 480,
>>> .yres = 272,
>>> },
>>> .max_bpp = 32,
>>> .default_bpp = 16,
>>> },
>>> };
>>>
>>> static struct s3c_fb_platdata mini6410_lcd_pdata[0] __initdata = {
>>> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
>>> .win[0] = &mini6410_fb_lcd0_win[0],
>>> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
>>> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
>>> };
>>>
>>> static struct s3c_fb_pd_win mini6410_fb_lcd1_win[] = {
>>> {
>>> .win_mode = { /* 7.0" 800x480 */
>>> .left_margin = 8,
>>> .right_margin = 13,
>>> .upper_margin = 7,
>>> .lower_margin = 5,
>>> .hsync_len = 3,
>>> .vsync_len = 1,
>>> .xres = 800,
>>> .yres = 480,
>>> },
>>> .max_bpp = 32,
>>> .default_bpp = 16,
>>> },
>>> };
>>>
>>> static struct s3c_fb_platdata mini6410_lcd_pdata[1] __initdata = {
>>> .setup_gpio = s3c64xx_fb_gpio_setup_24bpp,
>>> .win[0] = &mini6410_fb_lcd1_win[0],
>>> .vidcon0 = VIDCON0_VIDOUT_RGB | VIDCON0_PNRMODE_RGB,
>>> .vidcon1 = VIDCON1_INV_HSYNC | VIDCON1_INV_VSYNC,
>>> };
>>>
>>> Each struct means:
>>> mini6410_lcd_pdata[0]: 4.3" 480x272 LCD
>>> mini6410_fb_lcd0_win[0]: window 0 used for 4.3" 480x272 LCD
>>>
>>> mini6410_lcd_pdata[1]: 7.0" 800x480 LCD
>>> mini6410_fb_lcd1_win[0]: window 0 used for 7.0" 800x480 LCD
>>>
>>> In this case, 4.3" or 7.0" LCD can be selected using mini6410_lcd_pdata[], instead of selecting mini6410_fb_win[0] or
>>> mini6410_fb_win[1].
>>>
>>> My point is that 's3c_fb_platdata and s3c_fb_pd_win should be used exactly'.
>>
>> I agree with solution which Jingoo Han has proposed. Instead of
>> selecting one instance of s3c_fb_pd_win over the other at runtime, the
>> features->lcd_index can be used to select one of the instances of
>> platform data for s3c-fb driver.
>
> I agree with it too. Would you make such patch based on your s3c-fb patch set?
> Somebody should make it and definitely not to remove multiple LCD
> suport for these boards.
Sure. I will make a patch for that. I did not notice the way two lcd's
were supported in your code while preparing this patchset. Thanks for
bringing up this point.
>
>>
>> s3c_fb_pd_win is used to describe a window supported by the fimd
>> controller. It is not meant to carry lcd timing information. The lcd
>> panel timing remains the same irrespective of the window sizes. And
>> this patchset is a step towards untangling window information and
>> video timing.
>
> Right now s3c_fb_pd_win contains exactly LCD timing values.
> Your patch will move these timing values to struct fb_videomode.
>
> I could make necessary patch for that too, but I think you will spend
> less time doing it by yourself than syncing with me.
> What do you think?
Yes, I will prepare an additional patch in this series that will
maintain backward compatibility for real6410 and mini6410 with the
approach suggested by Jingoo Han. As I do not have either of these
boards, an ack from you would be very helpful when the next version of
this patchset is posted.
Thanks,
Thomas.
^ permalink raw reply
* Re: [PATCH v2 3/3] ARM: Samsung: Rework platform data of s3c-fb driver
From: Darius Augulis @ 2012-03-19 7:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAJuYYwQY6C-+NUuYfmDyGoh0ahVa8_x1cai8MCfN_JjWh1k1Gg@mail.gmail.com>
On Mon, Mar 19, 2012 at 9:49 AM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> On 19 March 2012 13:10, Darius Augulis <augulis.darius@gmail.com> wrote:
>> Right now s3c_fb_pd_win contains exactly LCD timing values.
>> Your patch will move these timing values to struct fb_videomode.
>>
>> I could make necessary patch for that too, but I think you will spend
>> less time doing it by yourself than syncing with me.
>> What do you think?
>
> Yes, I will prepare an additional patch in this series that will
> maintain backward compatibility for real6410 and mini6410 with the
> approach suggested by Jingoo Han. As I do not have either of these
> boards, an ack from you would be very helpful when the next version of
> this patchset is posted.
It's fine - I will review and probably test it.
^ permalink raw reply
* [GIT PULL] OMAP DSS for v3.4
From: Tomi Valkeinen @ 2012-03-19 11:01 UTC (permalink / raw)
To: Florian Tobias Schandinat, Arnd Bergmann; +Cc: linux-omap, linux-fbdev
[-- Attachment #1: Type: text/plain, Size: 7590 bytes --]
Hi Florian, Arnd,
Here are the changes for OMAP DSS driver for v3.4.
There's an issue with the dss driver that appears on arm-soc/for-next
branch, which I'm still solving
(http://marc.info/?l=linux-omap&m=133214966902577&w=2). I hope to get
fix for that ready and merged for -rc1, but I'm not sure if I can make
it in time, so I wanted to sent this pull request already.
Arnd, as discussed before, you can also merge this branch to solve the
conflicts between arm-soc and omapdss trees.
Note that this is the first pull request I've sent with a branch that I
have not been rebasing, so please check that the branch looks sane.
Tomi
The following changes since commit c16fa4f2ad19908a47c63d8fa436a1178438c7e7:
Linux 3.3 (2012-03-18 16:15:34 -0700)
are available in the git repository at:
git://gitorious.org/linux-omap-dss2/linux.git for-3.4
Archit Taneja (2):
OMAPDSS: Features: Maintain dss_feats as a list
OMAPDSS: DISPC: Fix scaling constraints for OMAP4
Danny Kukawka (1):
OMAPDSS: VENC: fix NULL pointer dereference in DSS2 VENC sysfs debug attr on OMAP4
Grazvydas Ignotas (2):
OMAPDSS: TPO-TD03MTEA1: fix suspend hang
OMAPDSS: TPO-TD03MTEA1: update default gamma
Julia Lawall (1):
OMAPDSS: use devm_ functions
Lajos Molnar (3):
OMAPDSS: DISPC: Fix OMAP4 supported color formats
OMAPDSS: DISPC: Fix FIR coefficients
OMAPDSS: MANAGER/APPLY: Add runtime_pm protection around wait_for_go/vsync functions
Mythri P K (6):
OMAPDSS: HDMI: remove duplicate video interface code
OMAPDSS: HDMI: update static timing table
OMAPDSS: HDMI: change the timing match logic
OMAPDSS: HDMI: remove duplicate code and mode parameter
OMAPDSS: HDMI: Move Avi-infoframe struct to hdmi_ip_data
OMAPDSS: HDMI: Add M2 divider while calculating clkout
Ricardo Neri (4):
OMAPDSS: HDMI: Correct source of the pixel clock in ACR calculation
OMAPDSS: Add DSS feature for HDMI MCLK for audio
OMAPDSS: HDMI: Implement initialization of MCLK
OMAPDSS: HDMI: Modify logic to configure MCLK
Tomi Valkeinen (29):
OMAPDSS: FEAT: Add FIFO_MERGE feature
OMAPDSS: APPLY: add fifo merge support funcs
OMAPDSS: APPLY: add fifo-merge support
OMAPDSS: DISPC: print fifo threshold values in bytes
OMAPDSS: DISPC: move fifo threhold calc to dispc.c
OMAPDSS: DISPC: Add naive threshold calc for fifomerge
Merge branch 'work/fifomerge'
Merge commit 'v3.3-rc4'
OMAPDSS: cleanup probe functions
OMAPFB: remove old blizzard driver
OMAPFB: Remove OMAP2/3 support from old omapfb driver
OMAPDSS: Remove video SRAM support
OMAPFB: Remove video SRAM support (old omapfb)
OMAP2+: remove unneeded #include omapfb.h
OMAP: N770: remove HWA742 platform data
OAMPFB: remove unused omapfb_set_ctrl_platform_data()
OMAPFB: remove early mem alloc from old omapfb
OMAPFB: remove mem info from platform_data
OMAPFB: remove unused fb_format_to_dss_mode()
OMAPFB: Move old omapfb private structs to a private include file
OMAPFB: remove omapfb_set_platform_data()
OMAP1: pass LCD config with omapfb_set_lcd_config()
OMAP: Remove OMAP_TAG_LCD and OMAP_TAG_FBMEM
OMAP1: Remove unused LCD devices from board files
OMAPFB: remove remaining OMAP arch checks
Merge branch 'work/old-omapfb-removal'
Merge commit 'v3.3-rc6'
OMAPDSS: APPLY: print warning if wait_pending_extra_info_updates fails
OMAPDSS: APPLY: fix clearing shadow dirty flag with manual update
Tony Lindgren (1):
ARM: OMAP2+: Fix compile error when FB_OMAP2 is not set
Yegor Yefremov (1):
OMAPDSS: add Innolux AT080TN52 display support
arch/arm/mach-omap1/board-ams-delta.c | 9 +-
arch/arm/mach-omap1/board-fsample.c | 15 +-
arch/arm/mach-omap1/board-h2.c | 15 +-
arch/arm/mach-omap1/board-h3.c | 9 +-
arch/arm/mach-omap1/board-htcherald.c | 9 +-
arch/arm/mach-omap1/board-innovator.c | 11 +-
arch/arm/mach-omap1/board-nokia770.c | 19 +-
arch/arm/mach-omap1/board-osk.c | 14 +-
arch/arm/mach-omap1/board-palmte.c | 10 +-
arch/arm/mach-omap1/board-palmtt.c | 10 +-
arch/arm/mach-omap1/board-palmz71.c | 10 +-
arch/arm/mach-omap1/board-perseus2.c | 15 +-
arch/arm/mach-omap1/board-sx1.c | 16 +-
arch/arm/mach-omap2/io.c | 1 -
arch/arm/plat-omap/common.c | 2 -
arch/arm/plat-omap/fb.c | 334 +----
arch/arm/plat-omap/fb.h | 10 -
arch/arm/plat-omap/include/plat/blizzard.h | 12 -
arch/arm/plat-omap/include/plat/board.h | 2 -
arch/arm/plat-omap/include/plat/hwa742.h | 8 -
arch/arm/plat-omap/include/plat/vram.h | 21 +-
drivers/video/omap/Kconfig | 16 +-
drivers/video/omap/Makefile | 12 +-
drivers/video/omap/blizzard.c | 1648 --------------------
drivers/video/omap/dispc.c | 1547 ------------------
drivers/video/omap/dispc.h | 46 -
drivers/video/omap/hwa742.c | 21 +-
drivers/video/omap/omapfb.h | 25 +-
drivers/video/omap/omapfb_main.c | 30 +-
drivers/video/omap/rfbi.c | 598 -------
drivers/video/omap2/displays/panel-generic-dpi.c | 23 +
.../video/omap2/displays/panel-tpo-td043mtea1.c | 153 ++-
drivers/video/omap2/dss/apply.c | 275 +++-
drivers/video/omap2/dss/dispc.c | 121 +-
drivers/video/omap2/dss/dispc_coefs.c | 9 +-
drivers/video/omap2/dss/display.c | 10 -
drivers/video/omap2/dss/dsi.c | 65 +-
drivers/video/omap2/dss/dss.c | 17 +-
drivers/video/omap2/dss/dss.h | 10 +-
drivers/video/omap2/dss/dss_features.c | 181 ++-
drivers/video/omap2/dss/dss_features.h | 54 +-
drivers/video/omap2/dss/hdmi.c | 278 ++--
drivers/video/omap2/dss/manager.c | 12 +-
drivers/video/omap2/dss/rfbi.c | 36 +-
drivers/video/omap2/dss/ti_hdmi.h | 56 +-
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 94 +-
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h | 47 -
drivers/video/omap2/dss/venc.c | 32 +-
drivers/video/omap2/omapfb/omapfb-ioctl.c | 2 +-
drivers/video/omap2/omapfb/omapfb-main.c | 101 +--
drivers/video/omap2/vram.c | 99 +--
include/linux/omapfb.h | 32 +-
52 files changed, 1001 insertions(+), 5201 deletions(-)
delete mode 100644 arch/arm/plat-omap/fb.h
delete mode 100644 arch/arm/plat-omap/include/plat/blizzard.h
delete mode 100644 arch/arm/plat-omap/include/plat/hwa742.h
delete mode 100644 drivers/video/omap/blizzard.c
delete mode 100644 drivers/video/omap/dispc.c
delete mode 100644 drivers/video/omap/dispc.h
delete mode 100644 drivers/video/omap/rfbi.c
[-- 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