* Re: [PATCH 05/11] fblog: register one fblog object per framebuffer
From: Ryan Mallon @ 2012-08-15 0:17 UTC (permalink / raw)
To: David Herrmann
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <CANq1E4QP8PwQRFBr++b3cu1dM9NWf6+Y45rq4UAirjaAHyeM4Q@mail.gmail.com>
On 14/08/12 21:01, David Herrmann wrote:
> Hi Ryan
>
> On Mon, Aug 13, 2012 at 1:54 AM, Ryan Mallon <rmallon@gmail.com> wrote:
>> On 13/08/12 00:53, David Herrmann wrote:
>>> drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 195 insertions(+)
>>>
>>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>>> index fb39737..279f4d8 100644
>>> --- a/drivers/video/console/fblog.c
>>> +++ b/drivers/video/console/fblog.c
>>> @@ -23,15 +23,210 @@
>>> * all fblog instances before running other graphics applications.
>>> */
>>>
>>> +#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/fb.h>
>>> #include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +
>>> +enum fblog_flags {
>>> + FBLOG_KILLED,
>>> +};
>>> +
>>> +struct fblog_fb {
>>> + unsigned long flags;
>>
>> Are more flags added in later patches? If not, why not just have:
>>
>> bool is_killed;
>>
>> ?
>
> Yes, more are added in patch-6 and patch-8 which includes FBLOG_OPEN,
> FBLOG_SUSPENDED, FBLOG_BLANKED.
>
>>> +static void fblog_release(struct device *dev)
>>> +{
>>> + struct fblog_fb *fb = to_fblog_dev(dev);
>>> +
>>> + kfree(fb);
>>> + module_put(THIS_MODULE);
>>> +}
>>> +
>>> +static void fblog_do_unregister(struct fb_info *info)
>>> +{
>>> + struct fblog_fb *fb;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (!fb || fb->info != info)
>>> + return;
>>> +
>>> + fblog_fbs[info->node] = NULL;
>>> +
>>> + device_del(&fb->dev);
>>> + put_device(&fb->dev);
>>
>> device_unregister?
>
> Right, I will replace it.
>
>>> +}
>>> +
>>> +static void fblog_do_register(struct fb_info *info, bool force)
>>> +{
>>> + struct fblog_fb *fb;
>>> + int ret;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (fb && fb->info != info) {
>>> + if (!force)
>>> + return;
>>> +
>>> + fblog_do_unregister(fb->info);
>>> + }
>>> +
>>> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>>> + if (!fb)
>>> + return;
>>> +
>>> + fb->info = info;
>>> + __module_get(THIS_MODULE);
>>> + device_initialize(&fb->dev);
>>> + fb->dev.class = fb_class;
>>> + fb->dev.release = fblog_release;
>>> + dev_set_name(&fb->dev, "fblog%d", info->node);
>>> + fblog_fbs[info->node] = fb;
>>> +
>>> + ret = device_add(&fb->dev);
>>> + if (ret) {
>>> + fblog_fbs[info->node] = NULL;
>>> + set_bit(FBLOG_KILLED, &fb->flags);
>>> + put_device(&fb->dev);
>>
>> kfree(fb); ?
>
> No. See device_initialize() in ./drivers/base/core.c. After a call to
> device_initialize() the object is ref-counted so put_device() will
> invoke the fblog_release() callback which will call kfree(fb) itself.
>
>>> + return;
>>> + }
>>> +}
>>> +
>>> +static void fblog_register(struct fb_info *info, bool force)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_register(info, force);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>> +
>>> +static void fblog_unregister(struct fb_info *info)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_unregister(info);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>
>> This locking is needlessly heavy, and could easily pushed down into the
>> fb_do_(un)register functions. It would also help make it clear exactly
>> what the lock is protecting.
>
> I need to call fblog_do_unregister() from within fblog_do_register().
> I cannot release the locks while calling fblog_do_unregister() so I
> need the unlocked fblog_do_unregister() function. So the locking must
> be in a wrapper function.
>
> See below for an explanation of the locks.
I meant something like the below. It doesn't actually make the lock much
more fine-grained, but (IMHO) it does make it a bit more clear how the
lock is being used. I also don't think you need to split
device_initialize and device_add, which can make the code a bit simpler:
static void __fblog_unregister(struct fblog_fb *fb)
{
fblog_fbs[fb->info->node] = NULL;
device_unregister(&fb->dev);
}
static void fblog_unregister(struct fb_info *info)
{
struct fblog_fb *fb;
mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (!fb || fb->info != info) {
mutex_unlock(&fblog_registration_lock);
return;
}
__fblog_unregister(fb);
mutex_unlock(&fblog_registration_lock);
}
static int fblog_register(struct fb_info *info, bool force)
{
struct fblog_fb *fb;
int ret;
mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (fb && fb->info != info) {
if (!force) {
mutex_unlock(&fblog_registration_lock);
return -EEXIST;
}
__fblog_unregister(fb);
}
fb = kzalloc(sizeof(*fb), GFP_KERNEL);
if (!fb)
return;
fb->info = info;
__module_get(THIS_MODULE);
fb->dev.class = fb_class;
fb->dev.release = fblog_release;
dev_set_name(&fb->dev, "fblog%d", info->node);
ret = device_register(&fb->dev);
if (ret) {
mutex_unlock(&fblog_registeration_lock);
put_device(&fb->dev);
return ret;
}
fblog_fbs[info->node] = fb;
mutex_unlock(&fblog_registeration_lock);
return 0;
}
Functions which do:
foo() {
lock(some_lock);
do_foo();
unlock(some_lock);
}
can be a valid pattern for locked/unlocked versions (usually the
unlocked version do_foo will be called __foo). But other times it looks
lazy, where the lock is just serialising everthing and doesn't scale
well. Granted, in a case like this it probably doesn't matter, but it
still a good idea to try and make the locking as fine grained as
possible. It also helps when trying to determine what a lock is actually
protecting, since if do_foo is long, the lock may or may not be
protecting any number of things inside it.
>>> +static int fblog_event(struct notifier_block *self, unsigned long action,
>>> + void *data)
>>> +{
>>> + struct fb_event *event = data;
>>> + struct fb_info *info = event->info;
>>> +
>>> + switch(action) {
>>> + case FB_EVENT_FB_REGISTERED:
>>> + /* This is called when a low-level system driver registers a new
>>> + * framebuffer. The registration lock is held but the console
>>> + * lock might not be held when this is called. */
>>
>> Nitpick:
>>
>> /*
>> * The Linux kernel multi-line
>> * comment style looks like
>> * this.
>> */
>
> I was confused by a recent discussion on the LKML:
> http://comments.gmane.org/gmane.linux.kernel/1282421
> However, turns out they didn't add this to CodingStyle so I will adopt
> the old style again. Will be fixed in the next revision, thanks.
>
>>> + fblog_register(info, true);
>>> + break;
>>> + case FB_EVENT_FB_UNREGISTERED:
>>> + /* This is called when a low-level system driver unregisters a
>>> + * framebuffer. The registration lock is held but the console
>>> + * lock might not be held. */
>>> + fblog_unregister(info);
>>> + break;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void fblog_scan(void)
>>> +{
>>> + unsigned int i;
>>> + struct fb_info *info, *tmp;
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>
>> Nitpick:
>>
>> if (IS_ERR_OR_NULL(info))
>
> Didn't know of this macro, thanks.
>
>>> + continue;
>>> +
>>> + fblog_register(info, false);
>>
>> This function should really return a value to indicate if it failed.
>> There is no point continuing if it didn't register anything.
>
> Indeed.
>
>>> + /* There is a very subtle race-condition. Even though we might
>>> + * own a reference to the fb, it may still get unregistered
>>> + * between our call from get_fb_info() and fblog_register().
>>> + * Therefore, we simply check whether the same fb still is
>>> + * registered by calling get_fb_info() again. Only if they
>>> + * differ we know that it got unregistered, therefore, we
>>> + * call fblog_unregister() with the old pointer. */
>>> +
>>> + tmp = get_fb_info(i);
>>> + if (tmp && !IS_ERR(tmp))
>>> + put_fb_info(tmp);
>>> + if (tmp != info)
>>> + fblog_unregister(info);
>>
>> It would be better to fix this issue properly. Calling fblog_unregister
>> here also looks unsafe if the call to fblog_register above failed.
>
> fblog_unregister() does nothing if the passed "info" does not match
> the found "info" so this is safe. And as we are holding a reference to
> "info" here, the pointer is always valid and cannot be used by other
> FBs.
>
> Fixing this properly means changing the locking of fbdev. This can
> either be done by exporting the fbdev-registration-lock (which I want
> to avoid as locking should never be exported in an API) or by changing
> fbdev to provide an scan/enumeration function itself. However, in my
> opinion both would be uglier than using this race-condition-check
> here.
>
> The best way would be redesigning the fbdev in-kernel API. But that
> means checking that fbcon will not break and this is something I
> really don't want to touch.
Fair enough. It might be something to come back to once this gets merged.
>>> + /* Here we either called fblog_unregister() and therefore do not
>>> + * need any reference to the fb, or we can be sure that the FB
>>> + * is registered and FB_EVENT_FB_UNREGISTERED will be called
>>> + * before the last reference is dropped. Hence, we can drop our
>>> + * reference here. */
>>
>> This seems a slightly odd reasoning. Why would you not hold a reference
>> to something you are using?
>
> It's because get_fb_info() takes the fbdev-registration-lock so we
> cannot call it from fblog_event().
That could possibly be fixed by providing an unlocked __get_fb_info
function. Then the ref-counting could be done by calling __get_fb_info
in fblog_register and __put_fb_info in fblog_unregister, so that you
hold the ref-count for the lifetime of the fblog object which I think
makes a bit more sense.
> And fblog_event() calls
> fblog_register() for hotplugged framebuffers so we cannot get a refcnt
> for hotplugged framebuffers.
> Now I can either increase the fbdev-refcount manually without calling
> get_fb_info() in fblog_register() or I can simply drop the framebuffer
> when the fbdev-core notifies me that it is gone. I chose the latter
> one.
>
>>> + put_fb_info(info);
>>> + }
>>> +}
>>> +
>>> +static struct notifier_block fblog_notifier = {
>>> + .notifier_call = fblog_event,
>>> +};
>>>
>>> static int __init fblog_init(void)
>>> {
>>> + int ret;
>>> +
>>> + ret = fb_register_client(&fblog_notifier);
>>> + if (ret) {
>>> + pr_err("cannot register framebuffer notifier\n");
>>> + return ret;
>>> + }
>>> +
>>> + fblog_scan();
>>> +
>>> return 0;
>>> }
>>>
>>> static void __exit fblog_exit(void)
>>> {
>>> + unsigned int i;
>>> + struct fb_info *info;
>>> +
>>> + fb_unregister_client(&fblog_notifier);
>>> +
>>> + /* We scan through the whole registered_fb array here instead of
>>> + * fblog_fbs because we need to get the device lock _before_ the
>>> + * fblog-registration-lock. */
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>> + continue;
>>> +
>>> + fblog_unregister(info);
>>
>> Given the description of the get_fb_info/fblog_register race above, can
>> this unregister the wrong framebuffer?
>
> No. fblog_unregister() will do nothing if the "info" pointers do not
> match. Moreover, this unregisters _all_ framebuffers, so I don't
> understand how this can unregister the "wrong" framebuffer?
>
> But I just noticed a race here. If the fbdev core unregisters a
> framebuffer during my loop, I will not call fblog_unregister() on it,
> as it does not exist anymore. Therefore, I will not free it in my
> fblog_fbs array as the fblog_notifier has already been unregistered.
> So I need to set a global "exiting" flag and fblog_event() shall only
> handle the UNBIND/UNREGISTER events during exit. Then I simply move
> the fb_unregister_client() call below this loop.
>
>>> + put_fb_info(info);
>>> + }
>>> }
>>>
>>> module_init(fblog_init);
>>>
>>
>
> A short explanation for all locks:
> First of all, I spent hours getting this right. The easy way would be
> removing all locks and relying on the fbdev locking (fbcon does this).
> But obviously, that doesn't work as fbdev has no safe way of
> scanning/enumerating all existing devices. And this is needed as fblog
> can be compiled as a module.
> fbdev has a core registration-lock and each framebuffer has its own
> fb-lock. fblog_event() is sometimes called with these locks held,
> sometimes without any locks held (see the comments in this function)
> and I need to work around this.
> So fblog_event() as entry point for hotplugging is called with the
> fbdev-registration-lock held. And it calls fblog_(un)register() which
> uses its own locks. So when calling this from fblog_scan(), I need to
> make sure to guarantee that the locks are acquired in the same order
> as in fblog_event(), otherwise I might have deadlocks. But I have no
> outside access to the fbdev-registration-lock, therefore, the
> fblog-registration-lock is needed and fblog_scan() needs to check for
> those ugly races.
Right, I think providing unlocked versions of get/put_fb_info will help
fix part of this.
> As you mentioned that this locking is needlessly complex, I have to
> disagree.
Sorry, poor choice of words. I meant 'coarse-grained', not complex.
However, some documentation in the code explaining how the locking
works, and what the locking order is never goes amiss.
> I use one lock to protect the registration
> (fblog_registration_lock) and one lock to protect each registered
> framebuffer from concurrent access (struct fblog_fb->lock). This is
> the most common way to protect hotplugged devices and I don't see how
> this can be done with less locks? (these are the only locks that are
> added by fblog).
I was only referring to the 'heavy' usage of the registration lock by
just acquiring it for the whole register/unregister functions. I was
skimming through the code and was assuming that the actual concurrent
part would just be the addition/removal in the fblog_fbs array, and
therefore the lock was being held for much longer than it needed to be.
As shown above, it isn't as bad as I thought it was.
> Normally, this would be all locks I have to access. However, the
> fbdev-core wasn't designed as in-kernel API and thus has very
> inconsistent locking. And all entry points to fblog that are not
> through fbdev-notifier-callbacks (eg, fblog_scan) need to make sure to
> acquire the same locks as the fbdev-core to avoid races with the
> fbdev-core. As this is not possible, because the fbdev-locks are not
> exported, I need to carefully use fbdev-functions that guarantee that
> I have no races. And I think I found the only way to guarantee this.
> If anyone has other ideas, I would be glad to hear them.
Yeah, this makes sense. It would be good, as you say, to not export the
locks for fbmem. I think adding a couple of functions to fbmem.c for
doing unlocked access might help a lot though.
~Ryan
^ permalink raw reply
* Re: [PATCH] video/mx3fb: remove stray l from debug output
From: Geert Uytterhoeven @ 2012-08-14 19:30 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344843875-8174-1-git-send-email-u.kleine-koenig@pengutronix.de>
On Mon, Aug 13, 2012 at 7:09 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Aug 13, 2012 at 9:44 AM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>> drivers/video/mx3fb.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
>> index c89f8a8..1dfdeb1 100644
>> --- a/drivers/video/mx3fb.c
>> +++ b/drivers/video/mx3fb.c
>> @@ -771,7 +771,7 @@ static int __set_par(struct fb_info *fbi, bool lock)
>> if (fbi->var.sync & FB_SYNC_SHARP_MODE)
>> mode = IPU_PANEL_SHARP_TFT;
>>
>> - dev_dbg(fbi->device, "pixclock = %ul Hz\n",
>> + dev_dbg(fbi->device, "pixclock = %u Hz\n",
>> (u32) (PICOS2KHZ(fbi->var.pixclock) * 1000UL));
>
> I think a better fix is to change the dyslectic "%ul" to "%lu", and
> drop the cast to
> u32, which was probably only added to kill the compiler warning caused by the
> dyslectia issue.
BTW, 'git grep "%ul"' shows a few more of these dyslectia issues.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Rob Clark @ 2012-08-14 19:26 UTC (permalink / raw)
To: Archit Taneja; +Cc: Tomi Valkeinen, linux-fbdev, linux-omap
In-Reply-To: <502A8322.6000309@ti.com>
On Tue, Aug 14, 2012 at 11:56 AM, Archit Taneja <archit@ti.com> wrote:
> On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
>>
>> On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
>>>
>>> Create function omapdss_sdi_set_timings(). Configuring new timings is
>>> done the
>>> same way as before, SDI is disabled, and re-enabled with the new timings
>>> in
>>> dssdev. This just moves the code from the panel drivers to the SDI
>>> driver.
>>>
>>> The panel drivers shouldn't be aware of how SDI manages to configure a
>>> new set
>>> of timings. This should be taken care of by the SDI driver itself.
>>
>>
>> I'm not sure about this one. Although I see that dpi.c does currently
>> the same thing as you're doing in your patch.
>
>
> Even HDMI does the same thing.
>
>
>>
>> One thing is that we should try to remove dssdev uses from the output
>> drivers, including use of dssdev->state.
>
>
> Yes, we could do that by keeping a state of the output(and also checking
> state of the manager)
>
>
>>
>> The other thing is that I don't think the output driver should disable &
>> enable the output during set timings. I think sdi's set_timings should
>> return EBUSY if the output is enabled. The same way as other
>> configuration functions should (like dpi_set_data_lines or such).
>>
>> I'm actually not sure if even the panel driver should disable & enable
>> the output during set_timings. Perhaps it should be the caller's
>> (omapdrm or such) responsibility....
>>
>> My reasoning here is that disabling & enabling the video output is not
>> invisible to the upper layers, so doing it "in secret" may be bad.
>>
>> Then again, perhaps timings can be changed freely on some other
>> platforms, and then it'd be nice if the panel driver wouldn't disable &
>> enable the output.
>>
>> So I'm again not quite sure what's the best way to handle this... (of
>> the dssdev->state I'm sure, its use should be removed from omapdss). Any
>> thoughts?
>
>
> I guess it depends on how drm/fb want to use it. I guess an output should
> have a set_timings() kind of op if it can do it seamlessly. I guess we can
> do that easily in DPI, for example, we could reduce the fps from 60 to 30
> without causing an artefacts(I think). For outputs which can't do it, we
> could remove the set_timings totally.
fwiw, drm wouldn't try to change timings on the fly.. or at least it
is bracketed by a call to the driver's crtc->prepare() and
crtc->commit() fxns (which in our case disable/enable output).
I haven't seen much userspace that tries to do things like this,
except maybe some apps like xbmc which seem to have some options to
attempt to change timings to align w/ video playback framerate. I am
a bit curious how many tv's and drivers could support this in a
glitch-free way.
BR,
-R
> However, it'll be kind of inconsistent for some outputs to set timings, and
> for others to not, and if in the future drm/fb gets exposed to ops too, we
> may have dirty checks to see if set_timings is populated or not.
>
> The easiest way would be to make all set_timings just update the copy of the
> timings output has, and expect drm/fb to disable and re enable the panel. We
> may end up doing unnecessary gpio resets and configuration of the panels
> though.
>
> Archit
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" 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 v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Archit Taneja @ 2012-08-14 19:20 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Rob Clark, linux-fbdev, linux-omap
In-Reply-To: <1344965600.2345.70.camel@deskari>
On Tuesday 14 August 2012 11:03 PM, Tomi Valkeinen wrote:
> On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote:
>> On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
>
>> I guess it depends on how drm/fb want to use it. I guess an output
>> should have a set_timings() kind of op if it can do it seamlessly. I
>> guess we can do that easily in DPI, for example, we could reduce the fps
>> from 60 to 30 without causing an artefacts(I think). For outputs which
>
> Yes, that kind of thing is easy to do by just changing the pck divider,
> which is in a shadow register. I mean, "easy" in theory, at least. Our
> clock calculation doesn't work like that currently, though, so it could
> end up changing DSS fck.
>
>> can't do it, we could remove the set_timings totally.
>
> But we do need set_timings for other outputs also (like sdi). We just
> can't change them just like that. Only outputs that do not need timings
> are DSI command mode and rfbi.
>
>> However, it'll be kind of inconsistent for some outputs to set timings,
>> and for others to not, and if in the future drm/fb gets exposed to ops
>> too, we may have dirty checks to see if set_timings is populated or not.
>>
>> The easiest way would be to make all set_timings just update the copy of
>> the timings output has, and expect drm/fb to disable and re enable the
>> panel. We may end up doing unnecessary gpio resets and configuration of
>> the panels though.
>
> I think changing things like timings is quite a rare operation. The only
> case it'd be necessary to change timings often, with speed, and without
> artifacts would be the fps drop you mentioned, for lower power use with
> panels that don't mind the fps drop.
>
> If I understood correctly, Rob said that drm already disables the output
> when changing the mode, when I asked if it's ok for the apply's
> set_timings to require the output to be off.
>
> In any case this is not a big issue, I mean, it's not causing any
> problems. Somebody is going to disable the output anyway when changing
> the timings. Perhaps even these patches are good, because they make the
> set_timings consistent across the output drivers (don't they?).
Yes, they do, there isn't a set_timings for RFBI though, only a
set_size, and DSI has set_timings for video mode and a set_size for
command mode, I haven't put checks in the ops for a panel driver to
wrongly call set_timings in commmand mode, and set_size in video mode.
I haven't done that yet because a future patch of mine will have a DSI
specific op called set_operation_mode() to make us independent of
dssdev->panel.dsi_mode, there is no guarantee that the panel driver to
first call set_operation_mode(), and then set_timings(), so I'm not sure
yet how to deal with that. Probably having a mode/state which says that
a field is unintialized might help, but that would overcomplicate things.
Archit
^ permalink raw reply
* Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Tomi Valkeinen @ 2012-08-14 17:33 UTC (permalink / raw)
To: Archit Taneja, Rob Clark; +Cc: linux-fbdev, linux-omap
In-Reply-To: <502A8322.6000309@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]
On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote:
> On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
> I guess it depends on how drm/fb want to use it. I guess an output
> should have a set_timings() kind of op if it can do it seamlessly. I
> guess we can do that easily in DPI, for example, we could reduce the fps
> from 60 to 30 without causing an artefacts(I think). For outputs which
Yes, that kind of thing is easy to do by just changing the pck divider,
which is in a shadow register. I mean, "easy" in theory, at least. Our
clock calculation doesn't work like that currently, though, so it could
end up changing DSS fck.
> can't do it, we could remove the set_timings totally.
But we do need set_timings for other outputs also (like sdi). We just
can't change them just like that. Only outputs that do not need timings
are DSI command mode and rfbi.
> However, it'll be kind of inconsistent for some outputs to set timings,
> and for others to not, and if in the future drm/fb gets exposed to ops
> too, we may have dirty checks to see if set_timings is populated or not.
>
> The easiest way would be to make all set_timings just update the copy of
> the timings output has, and expect drm/fb to disable and re enable the
> panel. We may end up doing unnecessary gpio resets and configuration of
> the panels though.
I think changing things like timings is quite a rare operation. The only
case it'd be necessary to change timings often, with speed, and without
artifacts would be the fps drop you mentioned, for lower power use with
panels that don't mind the fps drop.
If I understood correctly, Rob said that drm already disables the output
when changing the mode, when I asked if it's ok for the apply's
set_timings to require the output to be off.
In any case this is not a big issue, I mean, it's not causing any
problems. Somebody is going to disable the output anyway when changing
the timings. Perhaps even these patches are good, because they make the
set_timings consistent across the output drivers (don't they?).
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Tejun Heo @ 2012-08-14 17:31 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel, Jiri Kosina, linux-fbdev
In-Reply-To: <20120814083044.78b401d2@pluto.restena.lu>
Hello,
On Tue, Aug 14, 2012 at 08:30:44AM +0200, Bruno Prémont wrote:
> > I'm kinda shooting in the dark but who flushes / cancels
> > fb_info->deferred_work?
>
> fb_deferred_io_cleanup() does so and is called by destroy fbops
> (when last reference to fb_info is returned):
I see. Sorry but just from glancing it I can't really tell what's
going wrong.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 07/13] OMAPDSS: HDMI: Add a get_timing function for HDMI interface
From: Archit Taneja @ 2012-08-14 17:28 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344953420.4845.72.camel@lappyti>
On Tuesday 14 August 2012 07:40 PM, Tomi Valkeinen wrote:
> On Tue, 2012-08-14 at 18:45 +0530, Archit Taneja wrote:
>> On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
>>>> Add function omapdss_hdmi_display_get_timing() which returns the timings
>>>> maintained by the HDMI interface driver in it's hdmi_config field. This
>>>> prevents the need for the panel driver to configure default timings in it's
>>>> probe.
>>>>
>>>> This function is just intended to be used once during the panel driver's probe.
>>>> It makes sense for those interfaces which can be configured to a default timing.
>>>
>>> I'm not sure about this patch. So I think the basic idea here is that
>>> HDMI will use VGA video mode if, for whatever reason, no better mode is
>>> found out.
>>>
>>> After this change, the panel driver doesn't seem to do that. Instead it
>>> uses whatever mode was used previously by the hdmi output driver.
>>>
>>> Is the only reason for this patch to clean up the hdmi_panel driver? We
>>> could have a dss helper function which returns the timings for VGA
>>> (well, just a public const variable would be enough), and the panel
>>> driver could use that to initialize the timings struct.
>>
>> Yes, that's the only reason, basically, to sync the panel with the
>> timings to which the hdmi output driver configured itself during it's
>> probe. We don't use hdmi_get_timing anywhere apart from here.
>
> Does the hdmi output driver even need to configure any defaults?
> Wouldn't it be ok to presume that the panel driver configures the
> timings?
>
> I don't think it's sensible to think about default values in the output
> driver generally (well, at least for things like timings). Although in
> HDMI's case VGA is quite sensible default, but that's not the case for
> any other output.
>
> So I think generally we should just trust the panel driver to tell the
> output driver what configuration should be used before the panel driver
> enables the output.
>
> That said, it doesn't hurt that the output drivers initialize their own
> datastructures to something relatively sane to avoid any BUG() or WARN()
> calls in the omapdss. Although we could also somehow track if the
> timings has been set, and return an error when enabling the display if
> timings hasn't been set. But that requires an extra flag, so perhaps
> it's simpler to have some initial values in the output driver also.
>
>> I thought it would be clean to retrieve the timings by taking whatever
>> is stored in the hdmi output driver, but we could leave it to a
>> hardcoded VGA as before.
>
> My main worry is that it's not easily clear what is going on if you look
> at the panel code. It looks that the panel just uses whatever is in the
> output driver. It's not clear that it is always VGA. What if the
> previous mode was something else?
>
> I think it's much clearer if the panel driver sets the timings
> explicitly before enabling the output. It doesn't have to be in panel's
> probe, although that's perhaps the easiest place for it.
>
>> I have done the same for venc, let me know if you think these should be
>> removed.
>
> Well, I agree that the initial timings code in hdmi and venc is a bit
> ugly. I don't think it's bad as such, just that the timings are standard
> ones, and instead of having all the timings numbers there, we should
> have a common place for them.
Okay, I'll remove the get_timing ops, keep defaults in the panel
driver's probe, have a const variable for the defaults to make it look
less ugly, and make sure that there is a set_timings for the output in
the hdmi and venc panel drivers before they are enabled, it sort of okay
for hdmi and venc panel drivers as there is going to be only one of them
and be in our control.
Archit
^ permalink raw reply
* Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Archit Taneja @ 2012-08-14 17:08 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344951845.4845.58.camel@lappyti>
On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
> On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
>> Create function omapdss_sdi_set_timings(). Configuring new timings is done the
>> same way as before, SDI is disabled, and re-enabled with the new timings in
>> dssdev. This just moves the code from the panel drivers to the SDI driver.
>>
>> The panel drivers shouldn't be aware of how SDI manages to configure a new set
>> of timings. This should be taken care of by the SDI driver itself.
>
> I'm not sure about this one. Although I see that dpi.c does currently
> the same thing as you're doing in your patch.
Even HDMI does the same thing.
>
> One thing is that we should try to remove dssdev uses from the output
> drivers, including use of dssdev->state.
Yes, we could do that by keeping a state of the output(and also checking
state of the manager)
>
> The other thing is that I don't think the output driver should disable &
> enable the output during set timings. I think sdi's set_timings should
> return EBUSY if the output is enabled. The same way as other
> configuration functions should (like dpi_set_data_lines or such).
>
> I'm actually not sure if even the panel driver should disable & enable
> the output during set_timings. Perhaps it should be the caller's
> (omapdrm or such) responsibility....
>
> My reasoning here is that disabling & enabling the video output is not
> invisible to the upper layers, so doing it "in secret" may be bad.
>
> Then again, perhaps timings can be changed freely on some other
> platforms, and then it'd be nice if the panel driver wouldn't disable &
> enable the output.
>
> So I'm again not quite sure what's the best way to handle this... (of
> the dssdev->state I'm sure, its use should be removed from omapdss). Any
> thoughts?
I guess it depends on how drm/fb want to use it. I guess an output
should have a set_timings() kind of op if it can do it seamlessly. I
guess we can do that easily in DPI, for example, we could reduce the fps
from 60 to 30 without causing an artefacts(I think). For outputs which
can't do it, we could remove the set_timings totally.
However, it'll be kind of inconsistent for some outputs to set timings,
and for others to not, and if in the future drm/fb gets exposed to ops
too, we may have dirty checks to see if set_timings is populated or not.
The easiest way would be to make all set_timings just update the copy of
the timings output has, and expect drm/fb to disable and re enable the
panel. We may end up doing unnecessary gpio resets and configuration of
the panels though.
Archit
^ permalink raw reply
* Re: [PATCH 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-14 14:34 UTC (permalink / raw)
To: Mahapatra, Chandrabhanu; +Cc: linux-omap, linux-fbdev
In-Reply-To: <CAF0AtAsgTHwnUKaU8AzO5b7sThQTaocDGdb1Ebknuev_DgC0hg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4096 bytes --]
On Tue, 2012-08-14 at 18:00 +0530, Mahapatra, Chandrabhanu wrote:
> On Tue, Aug 14, 2012 at 3:18 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Mon, 2012-08-13 at 17:29 +0530, Chandrabhanu Mahapatra wrote:
> >> All the cpu_is checks have been moved to dss_init_features function providing a
> >> much more generic and cleaner interface. The OMAP version and revision specific
> >> initializations in various functions are cleaned and the necessary data are
> >> moved to dss_features structure which is local to dss.c.
> >
> > It'd be good to have some version information here. Even just [PATCH v3
> > 3/6] in the subject is good, but of course the best is if there's a
> > short change log
> >
>
> Is it ok to have short change log in the individual patch description
> itself. I normally prefer the cover letter for this.
No, the patch description is added to git repository, and shouldn't
contain that kind of "extra" information. But you can add the version
information to the posted patch.
The diff stats below, after the "---" line, are discarded by git when
applying the patch. So I think you can just insert any text below ---
(but before the actual diffs) and they do not appear in the final git
commit.
I've not actually used that myself, and I can't find notes about it in
the documentation, so I'm not sure if there are any restrictions about
it. I've seen it used, though.
> >> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> >> ---
> >> drivers/video/omap2/dss/dss.c | 115 ++++++++++++++++++++++++++---------------
> >> 1 file changed, 74 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
> >> index 7b1c6ac..6ab236e 100644
> >> --- a/drivers/video/omap2/dss/dss.c
> >> +++ b/drivers/video/omap2/dss/dss.c
> >> @@ -31,6 +31,7 @@
> >> #include <linux/clk.h>
> >> #include <linux/platform_device.h>
> >> #include <linux/pm_runtime.h>
> >> +#include <linux/dma-mapping.h>
> >
> > Hmm, what is this for?
> >
>
> I should have included "include/linux/gfp.h" instead. It defines GFP_KERNEL.
>
> >> #include <video/omapdss.h>
> >>
> >> @@ -65,6 +66,13 @@ struct dss_reg {
> >> static int dss_runtime_get(void);
> >> static void dss_runtime_put(void);
> >>
> >> +struct dss_features {
> >> + u16 fck_div_max;
> >> + int factor;
> >> + char *clk_name;
> >> + bool (*check_cinfo_fck) (void);
> >> +};
> >
> > Is the check_cinfo_fck a leftover from previous versions?
> >
>
> Sorry, to have skipped that.
>
> > I think "factor" name is too vague. If I remember right, it's some kind
> > of implicit multiplier for the dss fck. So "dss_fck_multiplier"? You
> > could check the clock trees to verify what the multiplier exactly was,
> > and see if you can come up with a better name =).
> >
>
> dss_fck_multiplier sounds good to me. DSS clocks trees do not seem to
Yes, it's not really DSS thing, but PRCM. If things were perfect, DSS
wouldn't even need to know about the clock.
> mention anything about this multiplier. I found clock "dpll4_mx4_clk"
> in omap3 trm but nothing called "dpll_per_m5x2_clk" in omap4 trm. Any
> references please you know about?
Hmm, I think that's the linux's name for it. TRM seems to call it
CLKOUTX2_M5 (in the power, reset and clock management section).
> >> + if (cpu_is_omap24xx())
> >> + dss.feat = &omap2_dss_features;
> >> + else if (cpu_is_omap34xx())
> >> + dss.feat = &omap34_dss_features;
> >> + else if (cpu_is_omap3630())
> >> + dss.feat = &omap36_dss_features;
> >> + else
> >> + dss.feat = &omap4_dss_features;
> >
> > Check for omap4 also, and if the cpu is none of the above, return error.
> >
>
> I was wondering what error value to return. I was considering EINVAL
> (error in value) and ENODEV (no such device). But nothing seems to fit
> this case.
ENODEV sounds fine to me. I think EINVAL should be used when some given
parameter was wrong.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 07/13] OMAPDSS: HDMI: Add a get_timing function for HDMI interface
From: Tomi Valkeinen @ 2012-08-14 14:10 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap
In-Reply-To: <502A4F62.5050400@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3399 bytes --]
On Tue, 2012-08-14 at 18:45 +0530, Archit Taneja wrote:
> On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote:
> > Hi,
> >
> > On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
> >> Add function omapdss_hdmi_display_get_timing() which returns the timings
> >> maintained by the HDMI interface driver in it's hdmi_config field. This
> >> prevents the need for the panel driver to configure default timings in it's
> >> probe.
> >>
> >> This function is just intended to be used once during the panel driver's probe.
> >> It makes sense for those interfaces which can be configured to a default timing.
> >
> > I'm not sure about this patch. So I think the basic idea here is that
> > HDMI will use VGA video mode if, for whatever reason, no better mode is
> > found out.
> >
> > After this change, the panel driver doesn't seem to do that. Instead it
> > uses whatever mode was used previously by the hdmi output driver.
> >
> > Is the only reason for this patch to clean up the hdmi_panel driver? We
> > could have a dss helper function which returns the timings for VGA
> > (well, just a public const variable would be enough), and the panel
> > driver could use that to initialize the timings struct.
>
> Yes, that's the only reason, basically, to sync the panel with the
> timings to which the hdmi output driver configured itself during it's
> probe. We don't use hdmi_get_timing anywhere apart from here.
Does the hdmi output driver even need to configure any defaults?
Wouldn't it be ok to presume that the panel driver configures the
timings?
I don't think it's sensible to think about default values in the output
driver generally (well, at least for things like timings). Although in
HDMI's case VGA is quite sensible default, but that's not the case for
any other output.
So I think generally we should just trust the panel driver to tell the
output driver what configuration should be used before the panel driver
enables the output.
That said, it doesn't hurt that the output drivers initialize their own
datastructures to something relatively sane to avoid any BUG() or WARN()
calls in the omapdss. Although we could also somehow track if the
timings has been set, and return an error when enabling the display if
timings hasn't been set. But that requires an extra flag, so perhaps
it's simpler to have some initial values in the output driver also.
> I thought it would be clean to retrieve the timings by taking whatever
> is stored in the hdmi output driver, but we could leave it to a
> hardcoded VGA as before.
My main worry is that it's not easily clear what is going on if you look
at the panel code. It looks that the panel just uses whatever is in the
output driver. It's not clear that it is always VGA. What if the
previous mode was something else?
I think it's much clearer if the panel driver sets the timings
explicitly before enabling the output. It doesn't have to be in panel's
probe, although that's perhaps the easiest place for it.
> I have done the same for venc, let me know if you think these should be
> removed.
Well, I agree that the initial timings code in hdmi and venc is a bit
ugly. I don't think it's bad as such, just that the timings are standard
ones, and instead of having all the timings numbers there, we should
have a common place for them.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Tomi Valkeinen @ 2012-08-14 13:44 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344512989-4071-10-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
> Create function omapdss_sdi_set_timings(). Configuring new timings is done the
> same way as before, SDI is disabled, and re-enabled with the new timings in
> dssdev. This just moves the code from the panel drivers to the SDI driver.
>
> The panel drivers shouldn't be aware of how SDI manages to configure a new set
> of timings. This should be taken care of by the SDI driver itself.
I'm not sure about this one. Although I see that dpi.c does currently
the same thing as you're doing in your patch.
One thing is that we should try to remove dssdev uses from the output
drivers, including use of dssdev->state.
The other thing is that I don't think the output driver should disable &
enable the output during set timings. I think sdi's set_timings should
return EBUSY if the output is enabled. The same way as other
configuration functions should (like dpi_set_data_lines or such).
I'm actually not sure if even the panel driver should disable & enable
the output during set_timings. Perhaps it should be the caller's
(omapdrm or such) responsibility....
My reasoning here is that disabling & enabling the video output is not
invisible to the upper layers, so doing it "in secret" may be bad.
Then again, perhaps timings can be changed freely on some other
platforms, and then it'd be nice if the panel driver wouldn't disable &
enable the output.
So I'm again not quite sure what's the best way to handle this... (of
the dssdev->state I'm sure, its use should be removed from omapdss). Any
thoughts?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH v3] da8xx-fb: allow frame to complete after disabling LCDC
From: Manjunathappa, Prakash @ 2012-08-14 13:35 UTC (permalink / raw)
To: linux-fbdev
Wait for active frame transfer to complete after disabling LCDC.
At the same this wait is not be required when there are sync and
underflow errors.
More information on disable and reset sequence can be found in
section 13.4.6 of AM335x TRM @www.ti.com/am335x.
Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
---
Applies on top of fbdev-next of Florian Tobias Schandinat's tree.
Since v2:
Optimized the lcd_disable_raster function.
Since v1:
Changed the commit message, also added link to hardware specification.
drivers/video/da8xx-fb.c | 49 ++++++++++++++++++++++++++++++++++++---------
1 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 7ae9d53..cb696ff 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -48,6 +48,7 @@
#define LCD_PL_LOAD_DONE BIT(6)
#define LCD_FIFO_UNDERFLOW BIT(5)
#define LCD_SYNC_LOST BIT(2)
+#define LCD_FRAME_DONE BIT(0)
/* LCD DMA Control Register */
#define LCD_DMA_BURST_SIZE(x) ((x) << 4)
@@ -288,13 +289,41 @@ static inline void lcd_enable_raster(void)
}
/* Disable the Raster Engine of the LCD Controller */
-static inline void lcd_disable_raster(void)
+static inline void lcd_disable_raster(bool wait_for_frame_done)
{
u32 reg;
+ u32 stat_reg = LCD_STAT_REG;
+ u32 loop_cnt = 0;
reg = lcdc_read(LCD_RASTER_CTRL_REG);
if (reg & LCD_RASTER_ENABLE)
lcdc_write(reg & ~LCD_RASTER_ENABLE, LCD_RASTER_CTRL_REG);
+
+ if (lcd_revision = LCD_VERSION_2)
+ stat_reg = LCD_RAW_STAT_REG;
+
+ if (wait_for_frame_done) {
+ /*
+ * 50 milli seconds should be sufficient for a frame to
+ * complete
+ */
+ loop_cnt = 50;
+ while (!(lcdc_read(stat_reg) & LCD_FRAME_DONE)) {
+ /* Handle timeout */
+ if (unlikely(0 = --loop_cnt)) {
+ pr_err("LCD Controller timed out\n");
+ break;
+ }
+ mdelay(1);
+ }
+ }
+
+ /* clear asserted interrupts */
+ reg = lcdc_read(stat_reg);
+ if (lcd_revision = LCD_VERSION_1)
+ lcdc_write(reg, LCD_STAT_REG);
+ else
+ lcdc_write(reg, LCD_MASKED_STAT_REG);
}
static void lcd_blit(int load_mode, struct da8xx_fb_par *par)
@@ -638,7 +667,7 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
static void lcd_reset(struct da8xx_fb_par *par)
{
/* Disable the Raster if previously Enabled */
- lcd_disable_raster();
+ lcd_disable_raster(false);
/* DMA has to be disabled */
lcdc_write(0, LCD_DMA_CTRL_REG);
@@ -734,7 +763,7 @@ static irqreturn_t lcdc_irq_handler_rev02(int irq, void *arg)
u32 stat = lcdc_read(LCD_MASKED_STAT_REG);
if ((stat & LCD_SYNC_LOST) && (stat & LCD_FIFO_UNDERFLOW)) {
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_MASKED_STAT_REG);
lcd_enable_raster();
} else if (stat & LCD_PL_LOAD_DONE) {
@@ -744,7 +773,7 @@ static irqreturn_t lcdc_irq_handler_rev02(int irq, void *arg)
* interrupt via the following write to the status register. If
* this is done after then one gets multiple PL done interrupts.
*/
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_MASKED_STAT_REG);
@@ -789,7 +818,7 @@ static irqreturn_t lcdc_irq_handler_rev01(int irq, void *arg)
u32 reg_ras;
if ((stat & LCD_SYNC_LOST) && (stat & LCD_FIFO_UNDERFLOW)) {
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_STAT_REG);
lcd_enable_raster();
} else if (stat & LCD_PL_LOAD_DONE) {
@@ -799,7 +828,7 @@ static irqreturn_t lcdc_irq_handler_rev01(int irq, void *arg)
* interrupt via the following write to the status register. If
* this is done after then one gets multiple PL done interrupts.
*/
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_STAT_REG);
@@ -898,7 +927,7 @@ static int lcd_da8xx_cpufreq_transition(struct notifier_block *nb,
if (val = CPUFREQ_POSTCHANGE) {
if (par->lcd_fck_rate != clk_get_rate(par->lcdc_clk)) {
par->lcd_fck_rate = clk_get_rate(par->lcdc_clk);
- lcd_disable_raster();
+ lcd_disable_raster(true);
lcd_calc_clk_divider(par);
lcd_enable_raster();
}
@@ -935,7 +964,7 @@ static int __devexit fb_remove(struct platform_device *dev)
if (par->panel_power_ctrl)
par->panel_power_ctrl(0);
- lcd_disable_raster();
+ lcd_disable_raster(true);
lcdc_write(0, LCD_RASTER_CTRL_REG);
/* disable DMA */
@@ -1051,7 +1080,7 @@ static int cfb_blank(int blank, struct fb_info *info)
if (par->panel_power_ctrl)
par->panel_power_ctrl(0);
- lcd_disable_raster();
+ lcd_disable_raster(true);
break;
default:
ret = -EINVAL;
@@ -1411,7 +1440,7 @@ static int fb_suspend(struct platform_device *dev, pm_message_t state)
par->panel_power_ctrl(0);
fb_set_suspend(info, 1);
- lcd_disable_raster();
+ lcd_disable_raster(true);
clk_disable(par->lcdc_clk);
console_unlock();
--
1.7.1
^ permalink raw reply related
* [PATCH v5] da8xx-fb: add 24bpp LCD configuration support
From: Manjunathappa, Prakash @ 2012-08-14 13:33 UTC (permalink / raw)
To: linux-fbdev
LCD controller on am335x supports 24bpp raster configuration in addition
to ones on da850. LCDC also supports 24bpp in unpacked format having
ARGB:8888 32bpp format data in DDR, but it doesn't interpret alpha
component of the data.
Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
Cc: Anatolij Gustschin <agust@denx.de>
---
Applies on top of fbdev-next of Florian Tobias Schandinat's tree.
Since v4:
Re-define CNVT_TOHW macro.
Since v3:
Minor nit, declare pseudo_palette as u32 type.
Since v2:
Fixed additional configurations for 24bpp support.
Since v1:
Simplified calculation of pseudopalette for FB_VISUAL_TRUECOLOR type.
drivers/video/da8xx-fb.c | 132 +++++++++++++++++++++++++++++++++------------
1 files changed, 97 insertions(+), 35 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index cb696ff..5c6df7b 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -87,6 +87,8 @@
#define LCD_V2_LIDD_CLK_EN BIT(1)
#define LCD_V2_CORE_CLK_EN BIT(0)
#define LCD_V2_LPP_B10 26
+#define LCD_V2_TFT_24BPP_MODE BIT(25)
+#define LCD_V2_TFT_24BPP_UNPACK BIT(26)
/* LCD Raster Timing 2 Register */
#define LCD_AC_BIAS_TRANSITIONS_PER_INT(x) ((x) << 16)
@@ -157,7 +159,6 @@ struct da8xx_fb_par {
unsigned int dma_end;
struct clk *lcdc_clk;
int irq;
- unsigned short pseudo_palette[16];
unsigned int palette_sz;
unsigned int pxl_clk;
int blank;
@@ -176,6 +177,7 @@ struct da8xx_fb_par {
unsigned int lcd_fck_rate;
#endif
void (*panel_power_ctrl)(int);
+ u32 pseudo_palette[16];
};
/* Variable Screen Information */
@@ -528,6 +530,9 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
{
u32 reg;
+ if (bpp > 16 && lcd_revision = LCD_VERSION_1)
+ return -EINVAL;
+
/* Set the Panel Width */
/* Pixels per line = (PPL + 1)*16 */
if (lcd_revision = LCD_VERSION_1) {
@@ -571,14 +576,19 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
reg = lcdc_read(LCD_RASTER_CTRL_REG) & ~(1 << 8);
if (raster_order)
reg |= LCD_RASTER_ORDER;
- lcdc_write(reg, LCD_RASTER_CTRL_REG);
+
+ par->palette_sz = 16 * 2;
switch (bpp) {
case 1:
case 2:
case 4:
case 16:
- par->palette_sz = 16 * 2;
+ break;
+ case 24:
+ reg |= LCD_V2_TFT_24BPP_MODE;
+ case 32:
+ reg |= LCD_V2_TFT_24BPP_UNPACK;
break;
case 8:
@@ -589,9 +599,12 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
return -EINVAL;
}
+ lcdc_write(reg, LCD_RASTER_CTRL_REG);
+
return 0;
}
+#define CNVT_TOHW(val, width) ((((val) << (width)) + 0x7FFF - (val)) >> 16)
static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
unsigned blue, unsigned transp,
struct fb_info *info)
@@ -607,13 +620,38 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
if (info->fix.visual = FB_VISUAL_DIRECTCOLOR)
return 1;
- if (info->var.bits_per_pixel = 4) {
- if (regno > 15)
- return 1;
+ if (info->var.bits_per_pixel > 16 && lcd_revision = LCD_VERSION_1)
+ return -EINVAL;
- if (info->var.grayscale) {
- pal = regno;
- } else {
+ switch (info->fix.visual) {
+ case FB_VISUAL_TRUECOLOR:
+ red = CNVT_TOHW(red, info->var.red.length);
+ green = CNVT_TOHW(green, info->var.green.length);
+ blue = CNVT_TOHW(blue, info->var.blue.length);
+ break;
+ case FB_VISUAL_PSEUDOCOLOR:
+ switch (info->var.bits_per_pixel) {
+ case 4:
+ if (regno > 15)
+ return -EINVAL;
+
+ if (info->var.grayscale) {
+ pal = regno;
+ } else {
+ red >>= 4;
+ green >>= 8;
+ blue >>= 12;
+
+ pal = red & 0x0f00;
+ pal |= green & 0x00f0;
+ pal |= blue & 0x000f;
+ }
+ if (regno = 0)
+ pal |= 0x2000;
+ palette[regno] = pal;
+ break;
+
+ case 8:
red >>= 4;
green >>= 8;
blue >>= 12;
@@ -621,36 +659,36 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
pal = (red & 0x0f00);
pal |= (green & 0x00f0);
pal |= (blue & 0x000f);
- }
- if (regno = 0)
- pal |= 0x2000;
- palette[regno] = pal;
- } else if (info->var.bits_per_pixel = 8) {
- red >>= 4;
- green >>= 8;
- blue >>= 12;
-
- pal = (red & 0x0f00);
- pal |= (green & 0x00f0);
- pal |= (blue & 0x000f);
-
- if (palette[regno] != pal) {
- update_hw = 1;
- palette[regno] = pal;
+ if (palette[regno] != pal) {
+ update_hw = 1;
+ palette[regno] = pal;
+ }
+ break;
}
- } else if ((info->var.bits_per_pixel = 16) && regno < 16) {
- red >>= (16 - info->var.red.length);
- red <<= info->var.red.offset;
+ break;
+ }
- green >>= (16 - info->var.green.length);
- green <<= info->var.green.offset;
+ /* Truecolor has hardware independent palette */
+ if (info->fix.visual = FB_VISUAL_TRUECOLOR) {
+ u32 v;
- blue >>= (16 - info->var.blue.length);
- blue <<= info->var.blue.offset;
+ if (regno > 15)
+ return -EINVAL;
- par->pseudo_palette[regno] = red | green | blue;
+ v = (red << info->var.red.offset) |
+ (green << info->var.green.offset) |
+ (blue << info->var.blue.offset);
+ switch (info->var.bits_per_pixel) {
+ case 16:
+ ((u16 *) (info->pseudo_palette))[regno] = v;
+ break;
+ case 24:
+ case 32:
+ ((u32 *) (info->pseudo_palette))[regno] = v;
+ break;
+ }
if (palette[0] != 0x4000) {
update_hw = 1;
palette[0] = 0x4000;
@@ -663,6 +701,7 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
return 0;
}
+#undef CNVT_TOHW
static void lcd_reset(struct da8xx_fb_par *par)
{
@@ -871,6 +910,9 @@ static int fb_check_var(struct fb_var_screeninfo *var,
{
int err = 0;
+ if (var->bits_per_pixel > 16 && lcd_revision = LCD_VERSION_1)
+ return -EINVAL;
+
switch (var->bits_per_pixel) {
case 1:
case 8:
@@ -906,6 +948,26 @@ static int fb_check_var(struct fb_var_screeninfo *var,
var->transp.length = 0;
var->nonstd = 0;
break;
+ case 24:
+ var->red.offset = 16;
+ var->red.length = 8;
+ var->green.offset = 8;
+ var->green.length = 8;
+ var->blue.offset = 0;
+ var->blue.length = 8;
+ var->nonstd = 0;
+ break;
+ case 32:
+ var->transp.offset = 24;
+ var->transp.length = 8;
+ var->red.offset = 16;
+ var->red.length = 8;
+ var->green.offset = 8;
+ var->green.length = 8;
+ var->blue.offset = 0;
+ var->blue.length = 8;
+ var->nonstd = 0;
+ break;
default:
err = -EINVAL;
}
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v2 07/13] OMAPDSS: HDMI: Add a get_timing function for HDMI interface
From: Archit Taneja @ 2012-08-14 13:27 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344949348.4845.46.camel@lappyti>
On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote:
> Hi,
>
> On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
>> Add function omapdss_hdmi_display_get_timing() which returns the timings
>> maintained by the HDMI interface driver in it's hdmi_config field. This
>> prevents the need for the panel driver to configure default timings in it's
>> probe.
>>
>> This function is just intended to be used once during the panel driver's probe.
>> It makes sense for those interfaces which can be configured to a default timing.
>
> I'm not sure about this patch. So I think the basic idea here is that
> HDMI will use VGA video mode if, for whatever reason, no better mode is
> found out.
>
> After this change, the panel driver doesn't seem to do that. Instead it
> uses whatever mode was used previously by the hdmi output driver.
>
> Is the only reason for this patch to clean up the hdmi_panel driver? We
> could have a dss helper function which returns the timings for VGA
> (well, just a public const variable would be enough), and the panel
> driver could use that to initialize the timings struct.
Yes, that's the only reason, basically, to sync the panel with the
timings to which the hdmi output driver configured itself during it's
probe. We don't use hdmi_get_timing anywhere apart from here.
I thought it would be clean to retrieve the timings by taking whatever
is stored in the hdmi output driver, but we could leave it to a
hardcoded VGA as before.
I have done the same for venc, let me know if you think these should be
removed.
Archit
^ permalink raw reply
* Re: [PATCH v2 07/13] OMAPDSS: HDMI: Add a get_timing function for HDMI interface
From: Tomi Valkeinen @ 2012-08-14 13:02 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344512989-4071-8-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]
Hi,
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
> Add function omapdss_hdmi_display_get_timing() which returns the timings
> maintained by the HDMI interface driver in it's hdmi_config field. This
> prevents the need for the panel driver to configure default timings in it's
> probe.
>
> This function is just intended to be used once during the panel driver's probe.
> It makes sense for those interfaces which can be configured to a default timing.
I'm not sure about this patch. So I think the basic idea here is that
HDMI will use VGA video mode if, for whatever reason, no better mode is
found out.
After this change, the panel driver doesn't seem to do that. Instead it
uses whatever mode was used previously by the hdmi output driver.
Is the only reason for this patch to clean up the hdmi_panel driver? We
could have a dss helper function which returns the timings for VGA
(well, just a public const variable would be enough), and the panel
driver could use that to initialize the timings struct.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Mahapatra, Chandrabhanu @ 2012-08-14 12:42 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1344937705.2345.17.camel@deskari>
On Tue, Aug 14, 2012 at 3:18 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-08-13 at 17:29 +0530, Chandrabhanu Mahapatra wrote:
>> All the cpu_is checks have been moved to dss_init_features function providing a
>> much more generic and cleaner interface. The OMAP version and revision specific
>> initializations in various functions are cleaned and the necessary data are
>> moved to dss_features structure which is local to dss.c.
>
> It'd be good to have some version information here. Even just [PATCH v3
> 3/6] in the subject is good, but of course the best is if there's a
> short change log
>
Is it ok to have short change log in the individual patch description
itself. I normally prefer the cover letter for this.
>> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>> ---
>> drivers/video/omap2/dss/dss.c | 115 ++++++++++++++++++++++++++---------------
>> 1 file changed, 74 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
>> index 7b1c6ac..6ab236e 100644
>> --- a/drivers/video/omap2/dss/dss.c
>> +++ b/drivers/video/omap2/dss/dss.c
>> @@ -31,6 +31,7 @@
>> #include <linux/clk.h>
>> #include <linux/platform_device.h>
>> #include <linux/pm_runtime.h>
>> +#include <linux/dma-mapping.h>
>
> Hmm, what is this for?
>
I should have included "include/linux/gfp.h" instead. It defines GFP_KERNEL.
>> #include <video/omapdss.h>
>>
>> @@ -65,6 +66,13 @@ struct dss_reg {
>> static int dss_runtime_get(void);
>> static void dss_runtime_put(void);
>>
>> +struct dss_features {
>> + u16 fck_div_max;
>> + int factor;
>> + char *clk_name;
>> + bool (*check_cinfo_fck) (void);
>> +};
>
> Is the check_cinfo_fck a leftover from previous versions?
>
Sorry, to have skipped that.
> I think "factor" name is too vague. If I remember right, it's some kind
> of implicit multiplier for the dss fck. So "dss_fck_multiplier"? You
> could check the clock trees to verify what the multiplier exactly was,
> and see if you can come up with a better name =).
>
dss_fck_multiplier sounds good to me. DSS clocks trees do not seem to
mention anything about this multiplier. I found clock "dpll4_mx4_clk"
in omap3 trm but nothing called "dpll_per_m5x2_clk" in omap4 trm. Any
references please you know about?
>> +
>> static struct {
>> struct platform_device *pdev;
>> void __iomem *base;
>> @@ -83,6 +91,8 @@ static struct {
>>
>> bool ctx_valid;
>> u32 ctx[DSS_SZ_REGS / sizeof(u32)];
>> +
>> + const struct dss_features *feat;
>> } dss;
>>
>> static const char * const dss_generic_clk_source_names[] = {
>> @@ -91,6 +101,30 @@ static const char * const dss_generic_clk_source_names[] = {
>> [OMAP_DSS_CLK_SRC_FCK] = "DSS_FCK",
>> };
>>
>> +static const struct __initdata dss_features omap2_dss_features = {
>> + .fck_div_max = 16,
>> + .factor = 2,
>> + .clk_name = NULL,
>> +};
>
> include/linux/init.h says says __initdata should be used like:
>
> static int init_variable __initdata = 0;
>
> And there actually seems to be __initconst also, which I think is the
> correct one to use here. Didn't know about that =):
>
> static const char linux_logo[] __initconst = { 0x32, 0x36, ... };
Thanks for mentioning.
>
>> +static const struct __initdata dss_features omap34_dss_features = {
>
> Perhaps the names could be more consistent. "omap34" doesn't sound like
> any omap we have ;). So maybe just omap2xxx, omap34xx, etc, the same way
> we have in the cpu_is calls.
>
ok.
>> + .fck_div_max = 16,
>> + .factor = 2,
>> + .clk_name = "dpll4_m4_ck",
>> +};
>> +
>> +static const struct __initdata dss_features omap36_dss_features = {
>> + .fck_div_max = 32,
>> + .factor = 1,
>> + .clk_name = "dpll4_m4_ck",
>> +};
>> +
>> +static const struct __initdata dss_features omap4_dss_features = {
>> + .fck_div_max = 32,
>> + .factor = 1,
>> + .clk_name = "dpll_per_m5x2_ck",
>> +};
>> +
>> static inline void dss_write_reg(const struct dss_reg idx, u32 val)
>> {
>> __raw_writel(val, dss.base + idx.idx);
>> @@ -236,7 +270,6 @@ const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src)
>> return dss_generic_clk_source_names[clk_src];
>> }
>>
>> -
>> void dss_dump_clocks(struct seq_file *s)
>> {
>> unsigned long dpll4_ck_rate;
>> @@ -259,18 +292,10 @@ void dss_dump_clocks(struct seq_file *s)
>>
>> seq_printf(s, "dpll4_ck %lu\n", dpll4_ck_rate);
>>
>> - if (cpu_is_omap3630() || cpu_is_omap44xx())
>> - seq_printf(s, "%s (%s) = %lu / %lu = %lu\n",
>> - fclk_name, fclk_real_name,
>> - dpll4_ck_rate,
>> - dpll4_ck_rate / dpll4_m4_ck_rate,
>> - fclk_rate);
>> - else
>> - seq_printf(s, "%s (%s) = %lu / %lu * 2 = %lu\n",
>> - fclk_name, fclk_real_name,
>> - dpll4_ck_rate,
>> - dpll4_ck_rate / dpll4_m4_ck_rate,
>> - fclk_rate);
>> + seq_printf(s, "%s (%s) = %lu / %lu * %d = %lu\n",
>> + fclk_name, fclk_real_name, dpll4_ck_rate,
>> + dpll4_ck_rate / dpll4_m4_ck_rate,
>> + dss.feat->factor, fclk_rate);
>> } else {
>> seq_printf(s, "%s (%s) = %lu\n",
>> fclk_name, fclk_real_name,
>> @@ -470,7 +495,7 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
>>
>> unsigned long fck, max_dss_fck;
>>
>> - u16 fck_div, fck_div_max = 16;
>> + u16 fck_div;
>>
>> int match = 0;
>> int min_fck_per_pck;
>> @@ -480,9 +505,8 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
>> max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
>>
>> fck = clk_get_rate(dss.dss_clk);
>> - if (req_pck = dss.cache_req_pck &&
>> - ((cpu_is_omap34xx() && prate = dss.cache_prate) ||
>> - dss.cache_dss_cinfo.fck = fck)) {
>> + if (req_pck = dss.cache_req_pck && prate = dss.cache_prate &&
>> + dss.cache_dss_cinfo.fck = fck) {
>> DSSDBG("dispc clock info found from cache.\n");
>> *dss_cinfo = dss.cache_dss_cinfo;
>> *dispc_cinfo = dss.cache_dispc_cinfo;
>> @@ -519,16 +543,10 @@ retry:
>>
>> goto found;
>> } else {
>> - if (cpu_is_omap3630() || cpu_is_omap44xx())
>> - fck_div_max = 32;
>> -
>> - for (fck_div = fck_div_max; fck_div > 0; --fck_div) {
>> + for (fck_div = dss.feat->fck_div_max; fck_div > 0; --fck_div) {
>> struct dispc_clock_info cur_dispc;
>>
>> - if (fck_div_max = 32)
>> - fck = prate / fck_div;
>> - else
>> - fck = prate / fck_div * 2;
>> + fck = prate / fck_div * dss.feat->factor;
>>
>> if (fck > max_dss_fck)
>> continue;
>> @@ -633,22 +651,11 @@ static int dss_get_clocks(void)
>>
>> dss.dss_clk = clk;
>>
>> - if (cpu_is_omap34xx()) {
>> - clk = clk_get(NULL, "dpll4_m4_ck");
>> - if (IS_ERR(clk)) {
>> - DSSERR("Failed to get dpll4_m4_ck\n");
>> - r = PTR_ERR(clk);
>> - goto err;
>> - }
>> - } else if (cpu_is_omap44xx()) {
>> - clk = clk_get(NULL, "dpll_per_m5x2_ck");
>> - if (IS_ERR(clk)) {
>> - DSSERR("Failed to get dpll_per_m5x2_ck\n");
>> - r = PTR_ERR(clk);
>> - goto err;
>> - }
>> - } else { /* omap24xx */
>> - clk = NULL;
>> + clk = clk_get(NULL, dss.feat->clk_name);
>> + if (IS_ERR(clk)) {
>> + DSSERR("Failed to get %s\n", dss.feat->clk_name);
>> + r = PTR_ERR(clk);
>> + goto err;
>> }
>>
>> dss.dpll4_m4_ck = clk;
>> @@ -704,6 +711,26 @@ void dss_debug_dump_clocks(struct seq_file *s)
>> }
>> #endif
>>
>> +static int __init dss_init_features(struct device *dev)
>> +{
>> + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL);
>> + if (!dss.feat) {
>> + dev_err(dev, "Failed to allocate local DSS Features\n");
>> + return -ENOMEM;
>> + }
>> +
>> + if (cpu_is_omap24xx())
>> + dss.feat = &omap2_dss_features;
>> + else if (cpu_is_omap34xx())
>> + dss.feat = &omap34_dss_features;
>> + else if (cpu_is_omap3630())
>> + dss.feat = &omap36_dss_features;
>> + else
>> + dss.feat = &omap4_dss_features;
>
> Check for omap4 also, and if the cpu is none of the above, return error.
>
I was wondering what error value to return. I was considering EINVAL
(error in value) and ENODEV (no such device). But nothing seems to fit
this case.
>> +
>> + return 0;
>> +}
>> +
>> /* DSS HW IP initialisation */
>> static int __init omap_dsshw_probe(struct platform_device *pdev)
>> {
>> @@ -750,6 +777,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
>> dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
>> dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
>>
>> + r = dss_init_features(&dss.pdev->dev);
>> + if (r)
>> + return r;
>> +
>> rev = dss_read_reg(DSS_REVISION);
>> printk(KERN_INFO "OMAP DSS rev %d.%d\n",
>> FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
>> @@ -772,6 +803,8 @@ static int __exit omap_dsshw_remove(struct platform_device *pdev)
>>
>> dss_put_clocks();
>>
>> + devm_kfree(&dss.pdev->dev, (void *)dss.feat);
>
> You don't need to free the memory allocated with devm_kzalloc. The
> framework will free it automatically on unload (that's the idea of
> devm_* functions).
>
Ok.
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-14 12:16 UTC (permalink / raw)
To: Mahapatra, Chandrabhanu; +Cc: linux-omap, linux-fbdev
In-Reply-To: <CAF0AtAt9SakEnavHMFXYZ2+nPXXreGcQNyXiJ7TuarRa1tATXA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2590 bytes --]
On Tue, 2012-08-14 at 17:33 +0530, Mahapatra, Chandrabhanu wrote:
> > +static const struct __initdata dispc_features omap2_dispc_features = {
> > + .hp_max = 256,
> > + .vp_max = 255,
> > + .sw_max = 64,
> > + .sw_start = 5,
> > + .fp_start = 15,
> > + .bp_start = 27,
> > + .calc_scaling = dispc_ovl_calc_scaling_24xx,
> > + .calc_core_clk = calc_core_clk_24xx,
> > +};
> > +
> > +static const struct __initdata dispc_features omap3_2_1_dispc_features = {
> > + .hp_max = 256,
> > + .vp_max = 255,
> > + .sw_max = 64,
> > + .sw_start = 5,
> > + .fp_start = 15,
> > + .bp_start = 27,
> > + .calc_scaling = dispc_ovl_calc_scaling_34xx,
> > + .calc_core_clk = calc_core_clk_34xx,
> > +};
> > +
> > +static const struct __initdata dispc_features omap3_3_0_dispc_features = {
> > + .hp_max = 4096,
> > + .vp_max = 4095,
> > + .sw_max = 256,
> > + .sw_start = 7,
> > + .fp_start = 19,
> > + .bp_start = 31,
> > + .calc_scaling = dispc_ovl_calc_scaling_34xx,
> > + .calc_core_clk = calc_core_clk_34xx,
> > +};
> > +
> > +static const struct __initdata dispc_features omap4_dispc_features = {
> > + .hp_max = 4096,
> > + .vp_max = 4095,
> > + .sw_max = 256,
> > + .sw_start = 7,
> > + .fp_start = 19,
> > + .bp_start = 31,
> > + .calc_scaling = dispc_ovl_calc_scaling_44xx,
> > + .calc_core_clk = calc_core_clk_44xx,
> > +};
> > +
>
> Here the dispc_features not only mention the omap name but also the
> revision like omap3_3_0_dispc_features which initializes data for
> OMAP3430_REV_ES3_0 and higher. May be omap34xx_rev3_0_dispc_features
> is a better name for this. For others omap44xx_dispc_features kind of
> name should be ok without revision number being mentioned. What d you
> say?
Sounds ok to me.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
From: Mahapatra, Chandrabhanu @ 2012-08-14 12:15 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1344859135-4470-1-git-send-email-cmahapatra@ti.com>
> +static const struct __initdata dispc_features omap2_dispc_features = {
> + .hp_max = 256,
> + .vp_max = 255,
> + .sw_max = 64,
> + .sw_start = 5,
> + .fp_start = 15,
> + .bp_start = 27,
> + .calc_scaling = dispc_ovl_calc_scaling_24xx,
> + .calc_core_clk = calc_core_clk_24xx,
> +};
> +
> +static const struct __initdata dispc_features omap3_2_1_dispc_features = {
> + .hp_max = 256,
> + .vp_max = 255,
> + .sw_max = 64,
> + .sw_start = 5,
> + .fp_start = 15,
> + .bp_start = 27,
> + .calc_scaling = dispc_ovl_calc_scaling_34xx,
> + .calc_core_clk = calc_core_clk_34xx,
> +};
> +
> +static const struct __initdata dispc_features omap3_3_0_dispc_features = {
> + .hp_max = 4096,
> + .vp_max = 4095,
> + .sw_max = 256,
> + .sw_start = 7,
> + .fp_start = 19,
> + .bp_start = 31,
> + .calc_scaling = dispc_ovl_calc_scaling_34xx,
> + .calc_core_clk = calc_core_clk_34xx,
> +};
> +
> +static const struct __initdata dispc_features omap4_dispc_features = {
> + .hp_max = 4096,
> + .vp_max = 4095,
> + .sw_max = 256,
> + .sw_start = 7,
> + .fp_start = 19,
> + .bp_start = 31,
> + .calc_scaling = dispc_ovl_calc_scaling_44xx,
> + .calc_core_clk = calc_core_clk_44xx,
> +};
> +
Here the dispc_features not only mention the omap name but also the
revision like omap3_3_0_dispc_features which initializes data for
OMAP3430_REV_ES3_0 and higher. May be omap34xx_rev3_0_dispc_features
is a better name for this. For others omap44xx_dispc_features kind of
name should be ok without revision number being mentioned. What d you
say?
> +static int __init dispc_init_features(struct device *dev)
> +{
> + dispc.feat = devm_kzalloc(dev, sizeof(*dispc.feat), GFP_KERNEL);
> + if (!dispc.feat) {
> + dev_err(dev, "Failed to allocate DISPC Features\n");
> + return -ENOMEM;
> + }
> +
> + if (cpu_is_omap24xx()) {
> + dispc.feat = &omap2_dispc_features;
> + } else if (cpu_is_omap34xx()) {
> + if (omap_rev() < OMAP3430_REV_ES3_0)
> + dispc.feat = &omap3_2_1_dispc_features;
> + else
> + dispc.feat = &omap3_3_0_dispc_features;
> + } else {
> + dispc.feat = &omap4_dispc_features;
> + }
> +
> + return 0;
> +}
> +
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH 07/11] fblog: allow selecting fbs via sysfs and module-parameters
From: David Herrmann @ 2012-08-14 11:07 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <5028447E.7000208@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 2:04 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> +static ssize_t fblog_dev_active_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> +
>> + return snprintf(buf, PAGE_SIZE, "%d\n",
>> + !!test_bit(FBLOG_OPEN, &fb->flags));
>
> Nitpick. sprintf is okay here, %d is rarely longer than PAGE_SIZE :-).
>
>> +}
>> +
>> +static ssize_t fblog_dev_active_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf,
>> + size_t count)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> + unsigned long num;
>> + int ret = 0;
>> +
>> + num = simple_strtoul(buf, NULL, 10);
>
> kstrtoul is preferred these days I think, it also catches errors.
>
>> +
>> + mutex_lock(&fb->info->lock);
>> + if (num)
>> + ret = fblog_open(fb);
>> + else
>> + fblog_close(fb, false);
>> + mutex_unlock(&fb->info->lock);
>> +
>> + return ret ? ret : count;
>
> Nitpick, you can use gcc's shortcut form of the ? operator here:
>
> return ret ?: count;
>
>> @@ -186,7 +228,20 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> return;
>> }
>>
>> - fblog_open(fb);
>> + ret = device_create_file(&fb->dev, &dev_attr_active);
>> + if (ret) {
>> + pr_err("fblog: cannot create sysfs entry");
>
> Shouldn't need the "fblog: " prefix, since you have pr_fmt defined.
>
>> + /* do not open fb if we cannot create control file */
>> + do_open = false;
>> + }
>> +
>> + if (!activate_on_hotplug)
>> + do_open = false;
>> + if (main_only && info->node != 0)
>> + do_open = false;
>> +
>> + if (do_open)
>> + fblog_open(fb);
>> }
>>
>> static void fblog_register(struct fb_info *info, bool force)
All four fixes make sense, thanks!
David
^ permalink raw reply
* Re: [PATCH 06/11] fblog: open fb on registration
From: David Herrmann @ 2012-08-14 11:05 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <50284383.7020100@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 2:00 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> /*
>> + * fblog_open/close()
>> + * These functions manage access to the underlying framebuffer. While opened, we
>> + * have a valid reference to the fb and can use it for drawing operations. When
>> + * the fb is unregistered, we drop our reference and close the fb so it can get
>> + * deleted properly. We also mark it as dead so no further fblog_open() call
>> + * will succeed.
>> + * Both functions must be called with the fb->info->lock mutex held! But make
>> + * sure to lock it _before_ locking the fblog-registration-lock. Otherwise, we
>> + * will dead-lock with fb-registration.
>> + */
>> +
>> +static int fblog_open(struct fblog_fb *fb)
>> +{
>> + int ret;
>> +
>> + mutex_lock(&fb->lock);
>> +
>> + if (test_bit(FBLOG_KILLED, &fb->flags)) {
>> + ret = -ENODEV;
>> + goto unlock;
>> + }
>> +
>> + if (test_bit(FBLOG_OPEN, &fb->flags)) {
>> + ret = 0;
>> + goto unlock;
>> + }
>> +
>> + if (!try_module_get(fb->info->fbops->owner)) {
>> + ret = -ENODEV;
>> + goto out_killed;
>> + }
>> +
>> + if (fb->info->fbops->fb_open && fb->info->fbops->fb_open(fb->info, 0)) {
>
> Should propagate the error here:
>
> if (fb->info->fbops->fb_open) {
> ret = fb->info->fbops->fb_open(fb->info, 0);
> if (ret)
> gotou out_unref;
> }
Nice catch, thanks.
>> +/*
>> * fblog framebuffer list
>> * The fblog_fbs[] array contains all currently registered framebuffers. If a
>> * framebuffer is in that list, we always must make sure that we own a reference
>> @@ -77,6 +147,7 @@ static void fblog_do_unregister(struct fb_info *info)
>>
>> fblog_fbs[info->node] = NULL;
>>
>> + fblog_close(fb, true);
>> device_del(&fb->dev);
>> put_device(&fb->dev);
>
> device_unregister?
Heh, you already mentioned this in the previous patch. I definitely
need to fix that.
>> }
>> @@ -99,6 +170,7 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> return;
>>
>> fb->info = info;
>> + mutex_init(&fb->lock);
>> __module_get(THIS_MODULE);
>> device_initialize(&fb->dev);
>> fb->dev.class = fb_class;
>> @@ -113,6 +185,8 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> put_device(&fb->dev);
>> return;
>> }
>> +
>> + fblog_open(fb);
>> }
>
> This function should really return an errno value.
I never used the return value in my code but as mentioned in the
previous patches, fblog_scan() should check them. So yes, I will add
this.
Thanks!
David
^ permalink raw reply
* Re: [PATCH 05/11] fblog: register one fblog object per framebuffer
From: David Herrmann @ 2012-08-14 11:01 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <50284234.40306@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 1:54 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 195 insertions(+)
>>
>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>> index fb39737..279f4d8 100644
>> --- a/drivers/video/console/fblog.c
>> +++ b/drivers/video/console/fblog.c
>> @@ -23,15 +23,210 @@
>> * all fblog instances before running other graphics applications.
>> */
>>
>> +#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
>> +
>> +#include <linux/device.h>
>> +#include <linux/fb.h>
>> #include <linux/module.h>
>> +#include <linux/mutex.h>
>> +
>> +enum fblog_flags {
>> + FBLOG_KILLED,
>> +};
>> +
>> +struct fblog_fb {
>> + unsigned long flags;
>
> Are more flags added in later patches? If not, why not just have:
>
> bool is_killed;
>
> ?
Yes, more are added in patch-6 and patch-8 which includes FBLOG_OPEN,
FBLOG_SUSPENDED, FBLOG_BLANKED.
>> +static void fblog_release(struct device *dev)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> +
>> + kfree(fb);
>> + module_put(THIS_MODULE);
>> +}
>> +
>> +static void fblog_do_unregister(struct fb_info *info)
>> +{
>> + struct fblog_fb *fb;
>> +
>> + fb = fblog_fbs[info->node];
>> + if (!fb || fb->info != info)
>> + return;
>> +
>> + fblog_fbs[info->node] = NULL;
>> +
>> + device_del(&fb->dev);
>> + put_device(&fb->dev);
>
> device_unregister?
Right, I will replace it.
>> +}
>> +
>> +static void fblog_do_register(struct fb_info *info, bool force)
>> +{
>> + struct fblog_fb *fb;
>> + int ret;
>> +
>> + fb = fblog_fbs[info->node];
>> + if (fb && fb->info != info) {
>> + if (!force)
>> + return;
>> +
>> + fblog_do_unregister(fb->info);
>> + }
>> +
>> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>> + if (!fb)
>> + return;
>> +
>> + fb->info = info;
>> + __module_get(THIS_MODULE);
>> + device_initialize(&fb->dev);
>> + fb->dev.class = fb_class;
>> + fb->dev.release = fblog_release;
>> + dev_set_name(&fb->dev, "fblog%d", info->node);
>> + fblog_fbs[info->node] = fb;
>> +
>> + ret = device_add(&fb->dev);
>> + if (ret) {
>> + fblog_fbs[info->node] = NULL;
>> + set_bit(FBLOG_KILLED, &fb->flags);
>> + put_device(&fb->dev);
>
> kfree(fb); ?
No. See device_initialize() in ./drivers/base/core.c. After a call to
device_initialize() the object is ref-counted so put_device() will
invoke the fblog_release() callback which will call kfree(fb) itself.
>> + return;
>> + }
>> +}
>> +
>> +static void fblog_register(struct fb_info *info, bool force)
>> +{
>> + mutex_lock(&fblog_registration_lock);
>> + fblog_do_register(info, force);
>> + mutex_unlock(&fblog_registration_lock);
>> +}
>> +
>> +static void fblog_unregister(struct fb_info *info)
>> +{
>> + mutex_lock(&fblog_registration_lock);
>> + fblog_do_unregister(info);
>> + mutex_unlock(&fblog_registration_lock);
>> +}
>
> This locking is needlessly heavy, and could easily pushed down into the
> fb_do_(un)register functions. It would also help make it clear exactly
> what the lock is protecting.
I need to call fblog_do_unregister() from within fblog_do_register().
I cannot release the locks while calling fblog_do_unregister() so I
need the unlocked fblog_do_unregister() function. So the locking must
be in a wrapper function.
See below for an explanation of the locks.
>> +static int fblog_event(struct notifier_block *self, unsigned long action,
>> + void *data)
>> +{
>> + struct fb_event *event = data;
>> + struct fb_info *info = event->info;
>> +
>> + switch(action) {
>> + case FB_EVENT_FB_REGISTERED:
>> + /* This is called when a low-level system driver registers a new
>> + * framebuffer. The registration lock is held but the console
>> + * lock might not be held when this is called. */
>
> Nitpick:
>
> /*
> * The Linux kernel multi-line
> * comment style looks like
> * this.
> */
I was confused by a recent discussion on the LKML:
http://comments.gmane.org/gmane.linux.kernel/1282421
However, turns out they didn't add this to CodingStyle so I will adopt
the old style again. Will be fixed in the next revision, thanks.
>> + fblog_register(info, true);
>> + break;
>> + case FB_EVENT_FB_UNREGISTERED:
>> + /* This is called when a low-level system driver unregisters a
>> + * framebuffer. The registration lock is held but the console
>> + * lock might not be held. */
>> + fblog_unregister(info);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void fblog_scan(void)
>> +{
>> + unsigned int i;
>> + struct fb_info *info, *tmp;
>> +
>> + for (i = 0; i < FB_MAX; ++i) {
>> + info = get_fb_info(i);
>> + if (!info || IS_ERR(info))
>
> Nitpick:
>
> if (IS_ERR_OR_NULL(info))
Didn't know of this macro, thanks.
>> + continue;
>> +
>> + fblog_register(info, false);
>
> This function should really return a value to indicate if it failed.
> There is no point continuing if it didn't register anything.
Indeed.
>> + /* There is a very subtle race-condition. Even though we might
>> + * own a reference to the fb, it may still get unregistered
>> + * between our call from get_fb_info() and fblog_register().
>> + * Therefore, we simply check whether the same fb still is
>> + * registered by calling get_fb_info() again. Only if they
>> + * differ we know that it got unregistered, therefore, we
>> + * call fblog_unregister() with the old pointer. */
>> +
>> + tmp = get_fb_info(i);
>> + if (tmp && !IS_ERR(tmp))
>> + put_fb_info(tmp);
>> + if (tmp != info)
>> + fblog_unregister(info);
>
> It would be better to fix this issue properly. Calling fblog_unregister
> here also looks unsafe if the call to fblog_register above failed.
fblog_unregister() does nothing if the passed "info" does not match
the found "info" so this is safe. And as we are holding a reference to
"info" here, the pointer is always valid and cannot be used by other
FBs.
Fixing this properly means changing the locking of fbdev. This can
either be done by exporting the fbdev-registration-lock (which I want
to avoid as locking should never be exported in an API) or by changing
fbdev to provide an scan/enumeration function itself. However, in my
opinion both would be uglier than using this race-condition-check
here.
The best way would be redesigning the fbdev in-kernel API. But that
means checking that fbcon will not break and this is something I
really don't want to touch.
>> + /* Here we either called fblog_unregister() and therefore do not
>> + * need any reference to the fb, or we can be sure that the FB
>> + * is registered and FB_EVENT_FB_UNREGISTERED will be called
>> + * before the last reference is dropped. Hence, we can drop our
>> + * reference here. */
>
> This seems a slightly odd reasoning. Why would you not hold a reference
> to something you are using?
It's because get_fb_info() takes the fbdev-registration-lock so we
cannot call it from fblog_event(). And fblog_event() calls
fblog_register() for hotplugged framebuffers so we cannot get a refcnt
for hotplugged framebuffers.
Now I can either increase the fbdev-refcount manually without calling
get_fb_info() in fblog_register() or I can simply drop the framebuffer
when the fbdev-core notifies me that it is gone. I chose the latter
one.
>> + put_fb_info(info);
>> + }
>> +}
>> +
>> +static struct notifier_block fblog_notifier = {
>> + .notifier_call = fblog_event,
>> +};
>>
>> static int __init fblog_init(void)
>> {
>> + int ret;
>> +
>> + ret = fb_register_client(&fblog_notifier);
>> + if (ret) {
>> + pr_err("cannot register framebuffer notifier\n");
>> + return ret;
>> + }
>> +
>> + fblog_scan();
>> +
>> return 0;
>> }
>>
>> static void __exit fblog_exit(void)
>> {
>> + unsigned int i;
>> + struct fb_info *info;
>> +
>> + fb_unregister_client(&fblog_notifier);
>> +
>> + /* We scan through the whole registered_fb array here instead of
>> + * fblog_fbs because we need to get the device lock _before_ the
>> + * fblog-registration-lock. */
>> +
>> + for (i = 0; i < FB_MAX; ++i) {
>> + info = get_fb_info(i);
>> + if (!info || IS_ERR(info))
>> + continue;
>> +
>> + fblog_unregister(info);
>
> Given the description of the get_fb_info/fblog_register race above, can
> this unregister the wrong framebuffer?
No. fblog_unregister() will do nothing if the "info" pointers do not
match. Moreover, this unregisters _all_ framebuffers, so I don't
understand how this can unregister the "wrong" framebuffer?
But I just noticed a race here. If the fbdev core unregisters a
framebuffer during my loop, I will not call fblog_unregister() on it,
as it does not exist anymore. Therefore, I will not free it in my
fblog_fbs array as the fblog_notifier has already been unregistered.
So I need to set a global "exiting" flag and fblog_event() shall only
handle the UNBIND/UNREGISTER events during exit. Then I simply move
the fb_unregister_client() call below this loop.
>> + put_fb_info(info);
>> + }
>> }
>>
>> module_init(fblog_init);
>>
>
A short explanation for all locks:
First of all, I spent hours getting this right. The easy way would be
removing all locks and relying on the fbdev locking (fbcon does this).
But obviously, that doesn't work as fbdev has no safe way of
scanning/enumerating all existing devices. And this is needed as fblog
can be compiled as a module.
fbdev has a core registration-lock and each framebuffer has its own
fb-lock. fblog_event() is sometimes called with these locks held,
sometimes without any locks held (see the comments in this function)
and I need to work around this.
So fblog_event() as entry point for hotplugging is called with the
fbdev-registration-lock held. And it calls fblog_(un)register() which
uses its own locks. So when calling this from fblog_scan(), I need to
make sure to guarantee that the locks are acquired in the same order
as in fblog_event(), otherwise I might have deadlocks. But I have no
outside access to the fbdev-registration-lock, therefore, the
fblog-registration-lock is needed and fblog_scan() needs to check for
those ugly races.
As you mentioned that this locking is needlessly complex, I have to
disagree. I use one lock to protect the registration
(fblog_registration_lock) and one lock to protect each registered
framebuffer from concurrent access (struct fblog_fb->lock). This is
the most common way to protect hotplugged devices and I don't see how
this can be done with less locks? (these are the only locks that are
added by fblog).
Normally, this would be all locks I have to access. However, the
fbdev-core wasn't designed as in-kernel API and thus has very
inconsistent locking. And all entry points to fblog that are not
through fbdev-notifier-callbacks (eg, fblog_scan) need to make sure to
acquire the same locks as the fbdev-core to avoid races with the
fbdev-core. As this is not possible, because the fbdev-locks are not
exported, I need to carefully use fbdev-functions that guarantee that
I have no races. And I think I found the only way to guarantee this.
If anyone has other ideas, I would be glad to hear them.
Thanks for reviewing the code!
Regards
David
^ permalink raw reply
* Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-14 9:58 UTC (permalink / raw)
To: Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1344859135-4470-1-git-send-email-cmahapatra@ti.com>
[-- Attachment #1: Type: text/plain, Size: 498 bytes --]
On Mon, 2012-08-13 at 17:28 +0530, Chandrabhanu Mahapatra wrote:
> All the cpu_is checks have been moved to dispc_init_features function providing
> a much more generic and cleaner interface. The OMAP version and revision
> specific functions and data are initialized by dispc_features structure which is
> local to dispc.c.
The comments I gave for the dss.c patch apply to this one also, about
devm_free, __initdata, and the names of the feat arrays.
Otherwise, looks good.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-14 9:48 UTC (permalink / raw)
To: Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1344859176-4512-1-git-send-email-cmahapatra@ti.com>
[-- Attachment #1: Type: text/plain, Size: 8283 bytes --]
On Mon, 2012-08-13 at 17:29 +0530, Chandrabhanu Mahapatra wrote:
> All the cpu_is checks have been moved to dss_init_features function providing a
> much more generic and cleaner interface. The OMAP version and revision specific
> initializations in various functions are cleaned and the necessary data are
> moved to dss_features structure which is local to dss.c.
It'd be good to have some version information here. Even just [PATCH v3
3/6] in the subject is good, but of course the best is if there's a
short change log
> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> ---
> drivers/video/omap2/dss/dss.c | 115 ++++++++++++++++++++++++++---------------
> 1 file changed, 74 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
> index 7b1c6ac..6ab236e 100644
> --- a/drivers/video/omap2/dss/dss.c
> +++ b/drivers/video/omap2/dss/dss.c
> @@ -31,6 +31,7 @@
> #include <linux/clk.h>
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> +#include <linux/dma-mapping.h>
Hmm, what is this for?
> #include <video/omapdss.h>
>
> @@ -65,6 +66,13 @@ struct dss_reg {
> static int dss_runtime_get(void);
> static void dss_runtime_put(void);
>
> +struct dss_features {
> + u16 fck_div_max;
> + int factor;
> + char *clk_name;
> + bool (*check_cinfo_fck) (void);
> +};
Is the check_cinfo_fck a leftover from previous versions?
I think "factor" name is too vague. If I remember right, it's some kind
of implicit multiplier for the dss fck. So "dss_fck_multiplier"? You
could check the clock trees to verify what the multiplier exactly was,
and see if you can come up with a better name =).
> +
> static struct {
> struct platform_device *pdev;
> void __iomem *base;
> @@ -83,6 +91,8 @@ static struct {
>
> bool ctx_valid;
> u32 ctx[DSS_SZ_REGS / sizeof(u32)];
> +
> + const struct dss_features *feat;
> } dss;
>
> static const char * const dss_generic_clk_source_names[] = {
> @@ -91,6 +101,30 @@ static const char * const dss_generic_clk_source_names[] = {
> [OMAP_DSS_CLK_SRC_FCK] = "DSS_FCK",
> };
>
> +static const struct __initdata dss_features omap2_dss_features = {
> + .fck_div_max = 16,
> + .factor = 2,
> + .clk_name = NULL,
> +};
include/linux/init.h says says __initdata should be used like:
static int init_variable __initdata = 0;
And there actually seems to be __initconst also, which I think is the
correct one to use here. Didn't know about that =):
static const char linux_logo[] __initconst = { 0x32, 0x36, ... };
> +static const struct __initdata dss_features omap34_dss_features = {
Perhaps the names could be more consistent. "omap34" doesn't sound like
any omap we have ;). So maybe just omap2xxx, omap34xx, etc, the same way
we have in the cpu_is calls.
> + .fck_div_max = 16,
> + .factor = 2,
> + .clk_name = "dpll4_m4_ck",
> +};
> +
> +static const struct __initdata dss_features omap36_dss_features = {
> + .fck_div_max = 32,
> + .factor = 1,
> + .clk_name = "dpll4_m4_ck",
> +};
> +
> +static const struct __initdata dss_features omap4_dss_features = {
> + .fck_div_max = 32,
> + .factor = 1,
> + .clk_name = "dpll_per_m5x2_ck",
> +};
> +
> static inline void dss_write_reg(const struct dss_reg idx, u32 val)
> {
> __raw_writel(val, dss.base + idx.idx);
> @@ -236,7 +270,6 @@ const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src)
> return dss_generic_clk_source_names[clk_src];
> }
>
> -
> void dss_dump_clocks(struct seq_file *s)
> {
> unsigned long dpll4_ck_rate;
> @@ -259,18 +292,10 @@ void dss_dump_clocks(struct seq_file *s)
>
> seq_printf(s, "dpll4_ck %lu\n", dpll4_ck_rate);
>
> - if (cpu_is_omap3630() || cpu_is_omap44xx())
> - seq_printf(s, "%s (%s) = %lu / %lu = %lu\n",
> - fclk_name, fclk_real_name,
> - dpll4_ck_rate,
> - dpll4_ck_rate / dpll4_m4_ck_rate,
> - fclk_rate);
> - else
> - seq_printf(s, "%s (%s) = %lu / %lu * 2 = %lu\n",
> - fclk_name, fclk_real_name,
> - dpll4_ck_rate,
> - dpll4_ck_rate / dpll4_m4_ck_rate,
> - fclk_rate);
> + seq_printf(s, "%s (%s) = %lu / %lu * %d = %lu\n",
> + fclk_name, fclk_real_name, dpll4_ck_rate,
> + dpll4_ck_rate / dpll4_m4_ck_rate,
> + dss.feat->factor, fclk_rate);
> } else {
> seq_printf(s, "%s (%s) = %lu\n",
> fclk_name, fclk_real_name,
> @@ -470,7 +495,7 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
>
> unsigned long fck, max_dss_fck;
>
> - u16 fck_div, fck_div_max = 16;
> + u16 fck_div;
>
> int match = 0;
> int min_fck_per_pck;
> @@ -480,9 +505,8 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
> max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
>
> fck = clk_get_rate(dss.dss_clk);
> - if (req_pck == dss.cache_req_pck &&
> - ((cpu_is_omap34xx() && prate == dss.cache_prate) ||
> - dss.cache_dss_cinfo.fck == fck)) {
> + if (req_pck == dss.cache_req_pck && prate == dss.cache_prate &&
> + dss.cache_dss_cinfo.fck == fck) {
> DSSDBG("dispc clock info found from cache.\n");
> *dss_cinfo = dss.cache_dss_cinfo;
> *dispc_cinfo = dss.cache_dispc_cinfo;
> @@ -519,16 +543,10 @@ retry:
>
> goto found;
> } else {
> - if (cpu_is_omap3630() || cpu_is_omap44xx())
> - fck_div_max = 32;
> -
> - for (fck_div = fck_div_max; fck_div > 0; --fck_div) {
> + for (fck_div = dss.feat->fck_div_max; fck_div > 0; --fck_div) {
> struct dispc_clock_info cur_dispc;
>
> - if (fck_div_max == 32)
> - fck = prate / fck_div;
> - else
> - fck = prate / fck_div * 2;
> + fck = prate / fck_div * dss.feat->factor;
>
> if (fck > max_dss_fck)
> continue;
> @@ -633,22 +651,11 @@ static int dss_get_clocks(void)
>
> dss.dss_clk = clk;
>
> - if (cpu_is_omap34xx()) {
> - clk = clk_get(NULL, "dpll4_m4_ck");
> - if (IS_ERR(clk)) {
> - DSSERR("Failed to get dpll4_m4_ck\n");
> - r = PTR_ERR(clk);
> - goto err;
> - }
> - } else if (cpu_is_omap44xx()) {
> - clk = clk_get(NULL, "dpll_per_m5x2_ck");
> - if (IS_ERR(clk)) {
> - DSSERR("Failed to get dpll_per_m5x2_ck\n");
> - r = PTR_ERR(clk);
> - goto err;
> - }
> - } else { /* omap24xx */
> - clk = NULL;
> + clk = clk_get(NULL, dss.feat->clk_name);
> + if (IS_ERR(clk)) {
> + DSSERR("Failed to get %s\n", dss.feat->clk_name);
> + r = PTR_ERR(clk);
> + goto err;
> }
>
> dss.dpll4_m4_ck = clk;
> @@ -704,6 +711,26 @@ void dss_debug_dump_clocks(struct seq_file *s)
> }
> #endif
>
> +static int __init dss_init_features(struct device *dev)
> +{
> + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL);
> + if (!dss.feat) {
> + dev_err(dev, "Failed to allocate local DSS Features\n");
> + return -ENOMEM;
> + }
> +
> + if (cpu_is_omap24xx())
> + dss.feat = &omap2_dss_features;
> + else if (cpu_is_omap34xx())
> + dss.feat = &omap34_dss_features;
> + else if (cpu_is_omap3630())
> + dss.feat = &omap36_dss_features;
> + else
> + dss.feat = &omap4_dss_features;
Check for omap4 also, and if the cpu is none of the above, return error.
> +
> + return 0;
> +}
> +
> /* DSS HW IP initialisation */
> static int __init omap_dsshw_probe(struct platform_device *pdev)
> {
> @@ -750,6 +777,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
> dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
> dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
>
> + r = dss_init_features(&dss.pdev->dev);
> + if (r)
> + return r;
> +
> rev = dss_read_reg(DSS_REVISION);
> printk(KERN_INFO "OMAP DSS rev %d.%d\n",
> FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
> @@ -772,6 +803,8 @@ static int __exit omap_dsshw_remove(struct platform_device *pdev)
>
> dss_put_clocks();
>
> + devm_kfree(&dss.pdev->dev, (void *)dss.feat);
You don't need to free the memory allocated with devm_kzalloc. The
framework will free it automatically on unload (that's the idea of
devm_* functions).
Otherwise looks good, cleans up nicely the cpu_is_* mess.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 03/11] fblog: new framebuffer kernel log dummy driver
From: David Herrmann @ 2012-08-14 9:42 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <50283D7A.4090309@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 1:34 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> > config VGA_CONSOLE
> > bool "VGA text console" if EXPERT || !X86
> > - depends on !4xx && !8xx && !SPARC && !M68K && !PARISC && !FRV &&
> > !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM || ARCH_FOOTBRIDGE ||
> > ARCH_INTEGRATOR || ARCH_NETWINDER)
> > + depends on VT && !4xx && !8xx && !SPARC && !M68K && !PARISC &&
> > !FRV && !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM ||
> > ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER)
> > default y
> > help
> > Saying Y here will allow you to use Linux in text mode through a
> > @@ -45,7 +45,7 @@ config VGACON_SOFT_SCROLLBACK_SIZE
> > screenfuls of scrollback buffer
>
> You could just place all of the CONFIG_VT options inside an if VT and
> then the new fblog options in the else case rather than modifying all of
> the (already overly long) depends statements.
Indeed, that sounds better.
> > +config FBLOG
> > + tristate "Framebuffer Kernel Log Driver"
> > + depends on !VT && FB
> > + default n
>
> Default n is not required, all options are default n unless otherwise
> specified. I don't think you need the dependency on FB either, since
> this file should only get included if CONFIG_FB is set?
I will remove the default-line, but the FB-dependency is required. The
"console/" directory includes also drivers like vgacon which do not
depend on FB so it gets always included.
> > + help
> > + This driver displays all kernel log messages on all connected
> > + framebuffers. It is mutually exclusive with
> > CONFIG_FRAMEBUFFER_CONSOLE
> > + and CONFIG_VT. It was mainly created for debugging purposes when
> > + CONFIG_VT is not selected but you still want kernel boot
> > messages on
> > + the screen.
>
> Do command line options exist to specify screens to write the debug log
> to? It might be a useful feature to have.
See patch-7. However, I am not entirely happy with it. I will probably
have to rethink the parameters.
Thanks for reviewing. I will fix all these in the next series.
Regards
David
^ permalink raw reply
* [PATCH RESEND] video/udlfb: fix line counting in fb_write
From: Alexander Holler @ 2012-08-14 7:11 UTC (permalink / raw)
To: linux-kernel; +Cc: Bernie Thompson, Alexander Holler, linux-fbdev
Line 0 and 1 were both written to line 0 (on the display) and all subsequent
lines had an offset of -1. The result was that the last line on the display
was never overwritten by writes to /dev/fbN.
Cc: stable@vger.kernel.org
Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
drivers/video/udlfb.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
index a159b63..85d8110 100644
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -647,7 +647,7 @@ static ssize_t dlfb_ops_write(struct fb_info *info,
const char __user *buf,
result = fb_sys_write(info, buf, count, ppos);
if (result > 0) {
- int start = max((int)(offset / info->fix.line_length) - 1, 0);
+ int start = max((int)(offset / info->fix.line_length), 0);
int lines = min((u32)((result / info->fix.line_length) + 1),
(u32)info->var.yres);
-- 1.7.6.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox