* Re: [RFC 0/4] Use the Common Display Framework in tegra-drm
From: Sascha Hauer @ 2013-01-30 8:38 UTC (permalink / raw)
To: Alex Courbot
Cc: Thierry Reding, Laurent Pinchart, Stephen Warren, Mark Zhang,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
linux-tegra@vger.kernel.org, gnurou@gmail.com
In-Reply-To: <5108D865.3070301@nvidia.com>
On Wed, Jan 30, 2013 at 05:23:01PM +0900, Alex Courbot wrote:
> On 01/30/2013 04:40 PM, Thierry Reding wrote:
> >Thanks *a lot* for taking care of this Alexandre! From a quick look at
> >the patches they seem generally fine. I'll go over them in a bit more
> >detail though.
>
> Glad you like it better than my previous attempts at controlling
> Tegra's panels and backlights. ;)
>
> >>1) The CDF has a get_modes() hook, but this is already implemented by
> >>tegra_connector_get_modes(). Ideally everything should be moved to the CDF hook,
> >>but Tegra's implementation uses DRM functions to retrieve the EDID and CDF
> >>should, AFAIK, remain DRM-agnostic.
> >
> >Maybe a good option would be to just not implement get_modes() if the
> >same information can be retrieved via EDID. That is, the DRM driver
> >could just go and fetch EDID when the nvidia,ddc-i2c-bus is available
> >(or parse the nvidia,edid blob) and only rely on CDF otherwise.
>
> Since EDID information is per-panel I'd intuitively say it should be
> provided by the panel driver.
Hm. If you know from the devicetree that you have a CLAA101WA01A then
you won't need EDID data at all. If instead you have EDID data you don't
have to put the information that this is a CLAA101WA01A into devicetree.
I think the next thing that will happen is that you want to use the EDID
data to 1) pick display timings 2) pick the right power sequence that
fits the panel found in EDID. You can't archieve 2) by hardcoding the
panel in the devicetree.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [RFC 1/4] video: panel: add CLAA101WA01A panel support
From: Alex Courbot @ 2013-01-30 8:28 UTC (permalink / raw)
To: Thierry Reding
Cc: Mark Zhang, Laurent Pinchart, Stephen Warren, Mark Zhang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
In-Reply-To: <20130130074852.GB17547-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org>
On 01/30/2013 04:48 PM, Thierry Reding wrote:
> I already said this in another thread. I think we should only be using
> the CDF .get_modes() for static modes that cannot be obtained from EDID.
> Thinking about it, I'm not quite sure why EDID would be needed for this
> kind of panel anyway. Ventana probably has it because it keeps us from
> having to hardcode things, but if we provide drivers for the panel
> anyway, we can just as well hard-code the supported mode(s) in those
> drivers.
>
> What I'm trying to say is that the existence of EDID is board-specific,
> so boards other than Ventana may not have an EDID EEPROM. Or perhaps
> this particular display has the EEPROM integrated? Even in that case,
> some boards may use this panel and simply not connect the EEPROM to an
> I2C controller.
Actually this display has its own EEPROM and pins for the I2C bus.
That's why it would seems out-of-place to have EDID taking place outside
its driver.
But you are right that we should also handle the case where the I2C bus
is not connected and provide a static list of videomodes (that could be
an "offline" dump of the EDID data). However, the driver could take the
decision to use it if the EDID bus is not specified in the DT or if
transfer failed for some reason.
But then as you mention we might as well save time and directly serve
that offline version, since we know the content of the EEPROM anyway.
Alex.
^ permalink raw reply
* Re: [RFC 0/4] Use the Common Display Framework in tegra-drm
From: Alex Courbot @ 2013-01-30 8:23 UTC (permalink / raw)
To: Thierry Reding
Cc: Laurent Pinchart, Stephen Warren, Mark Zhang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
In-Reply-To: <20130130074020.GA17547-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org>
On 01/30/2013 04:40 PM, Thierry Reding wrote:
> Thanks *a lot* for taking care of this Alexandre! From a quick look at
> the patches they seem generally fine. I'll go over them in a bit more
> detail though.
Glad you like it better than my previous attempts at controlling Tegra's
panels and backlights. ;)
>> 1) The CDF has a get_modes() hook, but this is already implemented by
>> tegra_connector_get_modes(). Ideally everything should be moved to the CDF hook,
>> but Tegra's implementation uses DRM functions to retrieve the EDID and CDF
>> should, AFAIK, remain DRM-agnostic.
>
> Maybe a good option would be to just not implement get_modes() if the
> same information can be retrieved via EDID. That is, the DRM driver
> could just go and fetch EDID when the nvidia,ddc-i2c-bus is available
> (or parse the nvidia,edid blob) and only rely on CDF otherwise.
Since EDID information is per-panel I'd intuitively say it should be
provided by the panel driver.
> So for Ventana the only reason why we need CDF is basically the power
> sequencing, right?
As of now, yes.
> I definitely think that we should aim for correct panel and backlight
> interaction. Perhaps this could work by looking up the real backlight
> via it's phandle and have the CDF driver use the backlight API to
> disable or enable the backlight as part of the power sequencing.
I have just written a bit of code that does that. It works well and
seems like a natural way to operate the backlight. However...
> I'm not sure what you mean by "cannot ignore FB events"? Can you provide
> a concrete problematic use-case?
... that's where thing stop looking nice. The backlight framework
forcibly registers a framebuffer notifier callback and switches the
backlight on and off on blank/unblank events, effectively duplicating
what the panel driver does. There is no way to disable this behavior at
the moment.
This could be solved by introducing a new function for controlling the
"ownership" of the backlight that would unregister the notifier when the
panel driver takes its reference to the backlight.
> Hijacking .update_status() sounds a bit risky. But perhaps you could
> wrap the real backlight in a CDF backlight to receive notifications.
> Obviously you'd get two backlight devices in sysfs, but that turn out
> not to be a problem.
For cosmetic reasons I'd prefer to avoid having two backlight devices
(which ended up in that terrible PWM backlight subdriver thing). Maybe
we can engineer the backlight framework to make such customizations easier?
Alex.
^ permalink raw reply
* Re: [RFC 1/4] video: panel: add CLAA101WA01A panel support
From: Mark Zhang @ 2013-01-30 8:08 UTC (permalink / raw)
To: Thierry Reding
Cc: Alex Courbot, Laurent Pinchart, Stephen Warren,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
linux-tegra@vger.kernel.org, gnurou@gmail.com
In-Reply-To: <20130130074852.GB17547@avionic-0098.mockup.avionic-design.de>
On 01/30/2013 03:48 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Jan 30, 2013 at 04:27:11PM +0900, Alex Courbot wrote:
>> On 01/30/2013 04:20 PM, Mark Zhang wrote:
> [...]
>>>> +static int panel_claa101_get_modes(struct display_entity *entity,
>>>> + const struct videomode **modes)
>>>> +{
>>>> + /* TODO get modes from EDID? */
>>>
>>> Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
>>> case, you can get EDID here. I know drm has some helpers to fetch EDID
>>> but I recall there are some other functions which has no drm
>>> dependencies which may be suitable for you.
>>
>> I explained this in the cover letter - I'm not sure we want to have
>> a dependency on DRM here, as CDF entities could also be connected to
>> other subsystems. That's something we need to figure out. But yes,
>> ultimately this should be the place where EDID is retrieved.
>
> I already said this in another thread. I think we should only be using
> the CDF .get_modes() for static modes that cannot be obtained from EDID.
> Thinking about it, I'm not quite sure why EDID would be needed for this
> kind of panel anyway. Ventana probably has it because it keeps us from
> having to hardcode things, but if we provide drivers for the panel
> anyway, we can just as well hard-code the supported mode(s) in those
> drivers.
I don't think so. I think it's better that we only have one entry for
getting video modes. Since CDF already provides ".get_modes" for us, we
can rely on that. And according to my understanding, get video modes in
panel driver makes sense than getting it in crtc.
In this way, panel driver may get video modes from EDID or from
hard-coded display timing infos, but other modules such as crtc don't
need to care about that.
Mark
>
> What I'm trying to say is that the existence of EDID is board-specific,
> so boards other than Ventana may not have an EDID EEPROM. Or perhaps
> this particular display has the EEPROM integrated? Even in that case,
> some boards may use this panel and simply not connect the EEPROM to an
> I2C controller.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>
^ permalink raw reply
* Re: [PATCH] pwm-backlight: handle BL_CORE_FBBLANK state
From: Thierry Reding @ 2013-01-30 7:58 UTC (permalink / raw)
To: Alexandre Courbot; +Cc: Richard Purdie, linux-kernel, linux-fbdev, gnurou
In-Reply-To: <1359526755-21145-1-git-send-email-acourbot@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 550 bytes --]
On Wed, Jan 30, 2013 at 03:19:15PM +0900, Alexandre Courbot wrote:
> According to include/linux/backlight.h, the fb_blank field is to be
> removed and blank status should preferably be set by setting the
> BL_CORE_FBBLANK bit of the state field. This patch ensures this
> condition is also taken into account when updating the backlight state.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> drivers/video/backlight/pwm_bl.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
Applied, thanks.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 1/4] video: panel: add CLAA101WA01A panel support
From: Thierry Reding @ 2013-01-30 7:48 UTC (permalink / raw)
To: Alex Courbot
Cc: Mark Zhang, Laurent Pinchart, Stephen Warren, Mark Zhang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
In-Reply-To: <5108CB4F.7000103-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]
On Wed, Jan 30, 2013 at 04:27:11PM +0900, Alex Courbot wrote:
> On 01/30/2013 04:20 PM, Mark Zhang wrote:
[...]
> >>+static int panel_claa101_get_modes(struct display_entity *entity,
> >>+ const struct videomode **modes)
> >>+{
> >>+ /* TODO get modes from EDID? */
> >
> >Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
> >case, you can get EDID here. I know drm has some helpers to fetch EDID
> >but I recall there are some other functions which has no drm
> >dependencies which may be suitable for you.
>
> I explained this in the cover letter - I'm not sure we want to have
> a dependency on DRM here, as CDF entities could also be connected to
> other subsystems. That's something we need to figure out. But yes,
> ultimately this should be the place where EDID is retrieved.
I already said this in another thread. I think we should only be using
the CDF .get_modes() for static modes that cannot be obtained from EDID.
Thinking about it, I'm not quite sure why EDID would be needed for this
kind of panel anyway. Ventana probably has it because it keeps us from
having to hardcode things, but if we provide drivers for the panel
anyway, we can just as well hard-code the supported mode(s) in those
drivers.
What I'm trying to say is that the existence of EDID is board-specific,
so boards other than Ventana may not have an EDID EEPROM. Or perhaps
this particular display has the EEPROM integrated? Even in that case,
some boards may use this panel and simply not connect the EEPROM to an
I2C controller.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 3/4] drm: tegra: use the Common Display Framework
From: Mark Zhang @ 2013-01-30 7:46 UTC (permalink / raw)
To: Alex Courbot
Cc: Laurent Pinchart, Thierry Reding, Stephen Warren, Mark Zhang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
In-Reply-To: <5108C55C.30104-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 01/30/2013 03:01 PM, Alex Courbot wrote:
> On 01/30/2013 03:50 PM, Mark Zhang wrote:
[...]
>
>>> + /* register display notifier */
>>> + output->display_notifier.dev = NULL;
>>
>> Set "display_notifier.dev" to NULL makes we have to compare with every
>> display entity, just like what you do in "display_notify_callback":
>>
>> entity->dev && entity->dev->of_node = pnode
>>
>> So can we get the "struct device *" of panel here? Seems currently the
>> "of" framework doesn't allow "device_node -> device".
>
> Nope. AFAICT the device might not be instanciated at this point. We
> become aware of it for the first time in the callback function. We also
> don't want to defer probing until the panel is parsed first, since the
> panel might also depend on resources of the display device.
>
Agree.
> Thanks,
> Alex.
>
^ permalink raw reply
* Re: [RFC 0/4] Use the Common Display Framework in tegra-drm
From: Thierry Reding @ 2013-01-30 7:40 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Laurent Pinchart, Stephen Warren, Mark Zhang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, gnurou-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1359514939-15653-1-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3715 bytes --]
On Wed, Jan 30, 2013 at 12:02:15PM +0900, Alexandre Courbot wrote:
> This series leverages the (still work-in-progress) Common Display Framework to
> add panel support to the tegra-drm driver. It also adds a driver for the
> CLAA101WA01A panel used on the Ventana board.
>
> The CDF is a moving target but Tegra needs some sort of display framework and
> even in its current state the CDF seems to be the best candidate. Besides, by
> using the CDF from now on we hope to provide useful feedback to Laurent and the
> other CDF designers.
>
> The changes to tegra-drm are rather minimal. Panels are referenced from Tegra DC
> output nodes through the "nvidia,panel" property. This property is looked up
> when a display connect notification is received in order to see if it points to
> the connected display entity. If it does, the entity is then used for output.
>
> The DPMS states are then propagated to the output entity, which is then supposed
> to call back into the set_stream() hook in order to enable/disable the output
> stream as needed.
Thanks *a lot* for taking care of this Alexandre! From a quick look at
the patches they seem generally fine. I'll go over them in a bit more
detail though.
> Although the overall design seems to work ok, a few specific issues need to be
> addressed:
>
> 1) The CDF has a get_modes() hook, but this is already implemented by
> tegra_connector_get_modes(). Ideally everything should be moved to the CDF hook,
> but Tegra's implementation uses DRM functions to retrieve the EDID and CDF
> should, AFAIK, remain DRM-agnostic.
Maybe a good option would be to just not implement get_modes() if the
same information can be retrieved via EDID. That is, the DRM driver
could just go and fetch EDID when the nvidia,ddc-i2c-bus is available
(or parse the nvidia,edid blob) and only rely on CDF otherwise.
So for Ventana the only reason why we need CDF is basically the power
sequencing, right?
> 2) There is currently no panel/backlight interaction, e.g. backlight status is
> controlled through FB events, independantly from the panel state. It could make
> sense to have the panel DT node reference the backlight and control it as part
> of its own power on/off sequence. Right now however, a backlight device cannot
> ignore FB events.
I definitely think that we should aim for correct panel and backlight
interaction. Perhaps this could work by looking up the real backlight
via it's phandle and have the CDF driver use the backlight API to
disable or enable the backlight as part of the power sequencing.
I'm not sure what you mean by "cannot ignore FB events"? Can you provide
a concrete problematic use-case?
> 3) Probably related to 2), now the backlight's power controls are part of the
> panel driver, because the pwm-backlight driver cannot control the power
> regulator and enable GPIO. This means that the backlight power is not turned off
> when its brightness is set to 0 through sysfs. Once again this speaks in favor
> of having stronger panel/backlight interaction: for instance, the panel driver
> could reference the backlight and hijack its update_status() op to replace it by
> one that does the correct power sequencing before calling the original function.
> This would require some extra infrastructure though. Another possibility would
> be to have a dedicated backlight driver for each panel, with its own
> "compatible" string.
Hijacking .update_status() sounds a bit risky. But perhaps you could
wrap the real backlight in a CDF backlight to receive notifications.
Obviously you'd get two backlight devices in sysfs, but that turn out
not to be a problem.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] pwm_backlight: Fix PWM levels support in non DT case
From: Peter Ujfalusi @ 2013-01-30 7:34 UTC (permalink / raw)
To: Thierry Reding
Cc: Richard Purdie, Grant Likely, Rob Landley,
Florian Tobias Schandinat, Andrew Morton, devicetree-discuss,
linux-doc, linux-kernel, linux-fbdev
In-Reply-To: <20130129123025.GA20166@avionic-0098.mockup.avionic-design.de>
On 01/29/2013 01:30 PM, Thierry Reding wrote:
>> Right. Now I know the background.
>> However I do not agree with the conclusion. Probably it is fine in some cases
>> to limit the number of configurable duty cycles to have only distinct steps.
>> To not go too far, on my laptop I have:
>> # cat /sys/class/backlight/acpi_video0/max_brightness
>> 15
>> # cat /sys/class/backlight/intel_backlight/max_brightness
>> 93
>
> FWIW, I have hardware with an Intel chipset that has max_brightness
> 13666. That doesn't mean all of these are necessarily valid or useful.
For sure this range is overkill, but if you reduce it to let's say 15 would it
be better? I don't think. It is up to the userspace to decide how to use it.
User should have full control over the HW in hand. In this particular case I
would expect userspace to give you around 20 steps to change brightness and
when you change it would step between those so you will have nice, smooth
changes and not big jumps.
> That's not true. The duty-cycle merely defines a percentage of how long
> the PWM signal will be high. You can choose an arbitrary number of
> subdivisions.
Sure all HW has limitation. The HW I have either have 127 or 255 time slots.
Probably the best thing way to deal with the backlight is to have a range of 0
- 100% Nothing else.
We should have an API to PWMs so user drivers could get the duty cycle from
the HW drivers. In this way backlight would only expose percentage and the
backlight driver would get the number of time slots from the PWM core to
calculate the actual value for the selected percentage.
> Since the brightness of a display isn't linearly proportional to the
> duty cycle of the PWM driving it, representing that brightness by a
> linear range is misleading. Furthermore some panels have regions where
> the backlight isn't lit at all or where changes in the PWM duty-cycle
> don't make any difference.
and all of this also depend on the surrounding light conditions as well. If
the same device is used in low light condition you care about the lower light
ranges more compared to when the same device is used in bright environment. In
these case the user space has to be adopted to the conditions and one should
not need to recompile the kernel/dtb just because the device is used in
different environment.
--
Péter
^ permalink raw reply
* Re: [RFC 3/4] drm: tegra: use the Common Display Framework
From: Alex Courbot @ 2013-01-30 7:30 UTC (permalink / raw)
To: Thierry Reding
Cc: Mark Zhang, Laurent Pinchart, Stephen Warren, Mark Zhang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
In-Reply-To: <20130130072406.GA17128-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org>
On 01/30/2013 04:24 PM, Thierry Reding wrote:
>>> Could you pick up a somewhat meaningful name? You know, there are too
>>> many variables with name "drm/connector/output/encoder"... :)
>>
>> Well, it's supposed to be abstract. From the CDF point of view it
>> could be anything besides a panel. I know this makes it an output of
>> an output, but I can't think of anything better right now.
>
> How about renaming "this" to stream to match with what the output is in
> CDF speak.
Good idea.
> And the output's output is the panel, right? Why not just
> call it that? Even if it isn't directly connected to a panel entity but
> has indeed a whole pipeline in between, for tegra-drm it is still a
> panel.
Makes sense indeed. Besides the DT refer it as "panel" already.
Thanks,
Alex.
^ permalink raw reply
* Re: [RFC 1/4] video: panel: add CLAA101WA01A panel support
From: Alex Courbot @ 2013-01-30 7:27 UTC (permalink / raw)
To: Mark Zhang
Cc: Laurent Pinchart, Thierry Reding, Stephen Warren, Mark Zhang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
In-Reply-To: <5108C9C1.1090707-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 01/30/2013 04:20 PM, Mark Zhang wrote:
>> + /* OFF and STANDBY are equivalent to us */
>> + if (state = DISPLAY_ENTITY_STATE_STANDBY)
>> + state = DISPLAY_ENTITY_STATE_OFF;
>
> Do we need this? The "switch" below handles the same thing already.
Indeed, I have rewritten this part actually.
>> +static int panel_claa101_get_modes(struct display_entity *entity,
>> + const struct videomode **modes)
>> +{
>> + /* TODO get modes from EDID? */
>
> Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
> case, you can get EDID here. I know drm has some helpers to fetch EDID
> but I recall there are some other functions which has no drm
> dependencies which may be suitable for you.
I explained this in the cover letter - I'm not sure we want to have a
dependency on DRM here, as CDF entities could also be connected to other
subsystems. That's something we need to figure out. But yes, ultimately
this should be the place where EDID is retrieved.
>> +static struct of_device_id panel_claa101_of_match[] = {
>> + { .compatible = "chunghwa,claa101wa01a", },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, pwm_backlight_of_match);
>
> What does this mean? Why we need this?
Well, now you know where I copy my code from. :)
Thanks,
Alex.
^ permalink raw reply
* Re: [RFC 3/4] drm: tegra: use the Common Display Framework
From: Thierry Reding @ 2013-01-30 7:24 UTC (permalink / raw)
To: Alex Courbot
Cc: Mark Zhang, Laurent Pinchart, Stephen Warren, Mark Zhang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
In-Reply-To: <5108C55C.30104-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 968 bytes --]
On Wed, Jan 30, 2013 at 04:01:48PM +0900, Alex Courbot wrote:
> On 01/30/2013 03:50 PM, Mark Zhang wrote:
> >>@@ -147,6 +148,9 @@ struct tegra_output {
> >>
> >> struct drm_encoder encoder;
> >> struct drm_connector connector;
> >>+ struct display_entity this;
> >>+ struct display_entity *output;
> >
> >Could you pick up a somewhat meaningful name? You know, there are too
> >many variables with name "drm/connector/output/encoder"... :)
>
> Well, it's supposed to be abstract. From the CDF point of view it
> could be anything besides a panel. I know this makes it an output of
> an output, but I can't think of anything better right now.
How about renaming "this" to stream to match with what the output is in
CDF speak. And the output's output is the panel, right? Why not just
call it that? Even if it isn't directly connected to a panel entity but
has indeed a whole pipeline in between, for tegra-drm it is still a
panel.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 1/4] video: panel: add CLAA101WA01A panel support
From: Mark Zhang @ 2013-01-30 7:20 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Laurent Pinchart, Thierry Reding, Stephen Warren, Mark Zhang,
linux-kernel, linux-fbdev, linux-tegra, gnurou
In-Reply-To: <1359514939-15653-2-git-send-email-acourbot@nvidia.com>
On 01/30/2013 11:02 AM, Alexandre Courbot wrote:
> Add support for the Chunghwa CLAA101WA01A display panel.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
[...]
> +
> +#include <video/display.h>
> +
> +#define CLAA101WA01A_WIDTH 223
> +#define CLAA101WA01A_HEIGHT 125
If I remember correct, the physical size of the panel can be fetched in
EDID. If this is correct, we don't have to hard-code here.
> +
> +struct panel_claa101 {
> + struct display_entity entity;
> + struct regulator *vdd_pnl;
> + struct regulator *vdd_bl;
> + /* Enable GPIOs */
> + int pnl_enable;
> + int bl_enable;
> +};
> +
> +#define to_panel_claa101(p) container_of(p, struct panel_claa101, entity)
> +
> +static int panel_claa101_set_state(struct display_entity *entity,
> + enum display_entity_state state)
> +{
> + struct panel_claa101 *panel = to_panel_claa101(entity);
> +
> + /* OFF and STANDBY are equivalent to us */
> + if (state = DISPLAY_ENTITY_STATE_STANDBY)
> + state = DISPLAY_ENTITY_STATE_OFF;
Do we need this? The "switch" below handles the same thing already.
> +
> + switch (state) {
> + case DISPLAY_ENTITY_STATE_OFF:
> + case DISPLAY_ENTITY_STATE_STANDBY:
> + if (entity->source)
> + display_entity_set_stream(entity->source,
> + DISPLAY_ENTITY_STREAM_STOPPED);
> +
> + /* TODO error checking? */
> + gpio_set_value_cansleep(panel->bl_enable, 0);
> + usleep_range(10000, 10000);
> + regulator_disable(panel->vdd_bl);
> + usleep_range(200000, 200000);
> + gpio_set_value_cansleep(panel->pnl_enable, 0);
> + regulator_disable(panel->vdd_pnl);
> + break;
> +
> + case DISPLAY_ENTITY_STATE_ON:
> + regulator_enable(panel->vdd_pnl);
> + gpio_set_value_cansleep(panel->pnl_enable, 1);
> + usleep_range(200000, 200000);
> + regulator_enable(panel->vdd_bl);
> + usleep_range(10000, 10000);
> + gpio_set_value_cansleep(panel->bl_enable, 1);
> +
> + if (entity->source)
> + display_entity_set_stream(entity->source,
> + DISPLAY_ENTITY_STREAM_CONTINUOUS);
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int panel_claa101_get_modes(struct display_entity *entity,
> + const struct videomode **modes)
> +{
> + /* TODO get modes from EDID? */
Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
case, you can get EDID here. I know drm has some helpers to fetch EDID
but I recall there are some other functions which has no drm
dependencies which may be suitable for you.
> + return 0;
> +}
[...]
> +
> +static int __exit panel_claa101_remove(struct platform_device *pdev)
> +{
> + struct panel_claa101 *panel = platform_get_drvdata(pdev);
> +
> + display_entity_unregister(&panel->entity);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
We don't need this kind of "ifdef" in upstream kernel.
> +static struct of_device_id panel_claa101_of_match[] = {
> + { .compatible = "chunghwa,claa101wa01a", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, pwm_backlight_of_match);
What does this mean? Why we need this?
> +#else
> +#endif
> +
> +static const struct dev_pm_ops panel_claa101_dev_pm_ops = {
> +};
> +
> +static struct platform_driver panel_claa101_driver = {
> + .probe = panel_claa101_probe,
> + .remove = panel_claa101_remove,
> + .driver = {
> + .name = "panel_claa101wa01a",
> + .owner = THIS_MODULE,
> +#ifdef CONFIG_PM
> + .pm = &panel_claa101_dev_pm_ops,
If you haven't implemented this in this patch set, I suggest removing this.
> +#endif
> +#ifdef CONFIG_OF
> + .of_match_table = of_match_ptr(panel_claa101_of_match),
> +#endif
> + },
> +};
> +
> +module_platform_driver(panel_claa101_driver);
> +
> +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
> +MODULE_DESCRIPTION("Chunghwa CLAA101WA01A Display Panel");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply
* Re: [RFC 3/4] drm: tegra: use the Common Display Framework
From: Alex Courbot @ 2013-01-30 7:01 UTC (permalink / raw)
To: Mark Zhang
Cc: Laurent Pinchart, Thierry Reding, Stephen Warren, Mark Zhang,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
linux-tegra@vger.kernel.org, gnurou@gmail.com
In-Reply-To: <5108C298.1000500@gmail.com>
On 01/30/2013 03:50 PM, Mark Zhang wrote:
>> @@ -147,6 +148,9 @@ struct tegra_output {
>>
>> struct drm_encoder encoder;
>> struct drm_connector connector;
>> + struct display_entity this;
>> + struct display_entity *output;
>
> Could you pick up a somewhat meaningful name? You know, there are too
> many variables with name "drm/connector/output/encoder"... :)
Well, it's supposed to be abstract. From the CDF point of view it could
be anything besides a panel. I know this makes it an output of an
output, but I can't think of anything better right now.
>> + if (entity->dev && entity->dev->of_node = pnode) {
>> + dev_dbg(output->dev, "connecting panel\n");
>> + output->output = display_entity_get(entity);
>> + display_entity_connect(&output->this, output->output);
>> + }
>> + of_node_put(pnode);
>> +
>> + break;
>> +
>> + case DISPLAY_ENTITY_NOTIFIER_DISCONNECT:
>> + if (!output->output || output->output != entity)
>> + break;
>> +
>> + dev_dbg(output->dev, "disconnecting panel\n");
>> + display_entity_disconnect(&output->this, output->output);
>> + output->output = NULL;
>> + display_entity_put(&output->this);
>
> No "display_entity_get" for "output->this", so I don't think we need
> "display_entity_put" here. If you register this entity with "release"
> callback and you wanna release "output->this", call the "release"
> function manually.
Oh, this was supposed to be called on output->output actually, to
balance the display_entity_get() of the connect event. Thanks for
catching this.
>> + /* register display entity */
>> + memset(&output->this, 0, sizeof(output->this));
>> + output->this.dev = drm->dev;
>
> Use "output->dev" here. Actually the device you wanna register it to
> display entity is the "encoder"(in drm terms), not "drm->dev". If we use
> "drm->dev" here, we will have all same device for all encoders(HDMI,
> DSI...).
Yes, that's absolutely right.
>> + /* register display notifier */
>> + output->display_notifier.dev = NULL;
>
> Set "display_notifier.dev" to NULL makes we have to compare with every
> display entity, just like what you do in "display_notify_callback":
>
> entity->dev && entity->dev->of_node = pnode
>
> So can we get the "struct device *" of panel here? Seems currently the
> "of" framework doesn't allow "device_node -> device".
Nope. AFAICT the device might not be instanciated at this point. We
become aware of it for the first time in the callback function. We also
don't want to defer probing until the panel is parsed first, since the
panel might also depend on resources of the display device.
Thanks,
Alex.
^ permalink raw reply
* Re: [RFC 3/4] drm: tegra: use the Common Display Framework
From: Mark Zhang @ 2013-01-30 6:50 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Laurent Pinchart, Thierry Reding, Stephen Warren, Mark Zhang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, gnurou-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1359514939-15653-4-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 01/30/2013 11:02 AM, Alexandre Courbot wrote:
> Make the tegra-drm driver use the Common Display Framework, letting it
> control the panel state according to the DPMS status.
>
> A "nvidia,panel" property is added to the output node of the Tegra DC
> that references the panel connected to a given output.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
[...]
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 741b5dc..5e63c56 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -17,6 +17,7 @@
> #include <drm/drm_gem_cma_helper.h>
> #include <drm/drm_fb_cma_helper.h>
> #include <drm/drm_fixed.h>
> +#include <video/display.h>
>
> struct tegra_framebuffer {
> struct drm_framebuffer base;
> @@ -147,6 +148,9 @@ struct tegra_output {
>
> struct drm_encoder encoder;
> struct drm_connector connector;
> + struct display_entity this;
> + struct display_entity *output;
Could you pick up a somewhat meaningful name? You know, there are too
many variables with name "drm/connector/output/encoder"... :)
> + struct display_entity_notifier display_notifier;
> };
>
[...]
> +static int display_notify_callback(struct display_entity_notifier *notifier,
> + struct display_entity *entity, int event)
> +{
> + struct tegra_output *output = display_notifier_to_output(notifier);
> + struct device_node *pnode;
> +
> + switch (event) {
> + case DISPLAY_ENTITY_NOTIFIER_CONNECT:
> + if (output->output)
> + break;
> +
> + pnode = of_parse_phandle(output->of_node, "nvidia,panel", 0);
> + if (!pnode)
> + break;
> +
> + if (entity->dev && entity->dev->of_node = pnode) {
> + dev_dbg(output->dev, "connecting panel\n");
> + output->output = display_entity_get(entity);
> + display_entity_connect(&output->this, output->output);
> + }
> + of_node_put(pnode);
> +
> + break;
> +
> + case DISPLAY_ENTITY_NOTIFIER_DISCONNECT:
> + if (!output->output || output->output != entity)
> + break;
> +
> + dev_dbg(output->dev, "disconnecting panel\n");
> + display_entity_disconnect(&output->this, output->output);
> + output->output = NULL;
> + display_entity_put(&output->this);
No "display_entity_get" for "output->this", so I don't think we need
"display_entity_put" here. If you register this entity with "release"
callback and you wanna release "output->this", call the "release"
function manually.
Only when you have "display_entity_get", use "display_entity_put" to
release.
> +
> + break;
> +
> + default:
> + dev_dbg(output->dev, "unhandled display event\n");
> + break;
> + }
> +
> + return 0;
> +}
> +
[...]
> int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
> {
> int connector, encoder, err;
> @@ -250,6 +341,23 @@ int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
>
> output->encoder.possible_crtcs = 0x3;
>
> + /* register display entity */
> + memset(&output->this, 0, sizeof(output->this));
> + output->this.dev = drm->dev;
Use "output->dev" here. Actually the device you wanna register it to
display entity is the "encoder"(in drm terms), not "drm->dev". If we use
"drm->dev" here, we will have all same device for all encoders(HDMI,
DSI...).
> + output->this.ops.video = &tegra_output_video_ops;
> + err = display_entity_register(&output->this);
> + if (err) {
> + dev_err(output->dev, "cannot register display entity\n");
> + return err;
> + }
> +
> + /* register display notifier */
> + output->display_notifier.dev = NULL;
Set "display_notifier.dev" to NULL makes we have to compare with every
display entity, just like what you do in "display_notify_callback":
entity->dev && entity->dev->of_node = pnode
So can we get the "struct device *" of panel here? Seems currently the
"of" framework doesn't allow "device_node -> device".
> + output->display_notifier.notify = display_notify_callback;
> + err = display_entity_register_notifier(&output->display_notifier);
> + if (err)
> + return err;
> +
> return 0;
>
> free_hpd:
> @@ -260,6 +368,12 @@ free_hpd:
>
> int tegra_output_exit(struct tegra_output *output)
> {
> + if (output->output)
> + display_entity_put(output->output);
> +
> + display_entity_unregister_notifier(&output->display_notifier);
> + display_entity_unregister(&output->this);
> +
> if (gpio_is_valid(output->hpd_gpio)) {
> free_irq(output->hpd_irq, output);
> gpio_free(output->hpd_gpio);
>
^ permalink raw reply
* [PATCH] pwm-backlight: handle BL_CORE_FBBLANK state
From: Alexandre Courbot @ 2013-01-30 6:19 UTC (permalink / raw)
To: Thierry Reding, Richard Purdie
Cc: linux-kernel, linux-fbdev, gnurou, Alexandre Courbot
According to include/linux/backlight.h, the fb_blank field is to be
removed and blank status should preferably be set by setting the
BL_CORE_FBBLANK bit of the state field. This patch ensures this
condition is also taken into account when updating the backlight state.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
drivers/video/backlight/pwm_bl.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..4af6d13 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -41,10 +41,9 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
int brightness = bl->props.brightness;
int max = bl->props.max_brightness;
- if (bl->props.power != FB_BLANK_UNBLANK)
- brightness = 0;
-
- if (bl->props.fb_blank != FB_BLANK_UNBLANK)
+ if (bl->props.power != FB_BLANK_UNBLANK ||
+ bl->props.fb_blank != FB_BLANK_UNBLANK ||
+ bl->props.state & BL_CORE_FBBLANK)
brightness = 0;
if (pb->notify)
--
1.8.1.2
^ permalink raw reply related
* [RFC 4/4] tegra: enable CDF and claa101 panel
From: Alexandre Courbot @ 2013-01-30 3:02 UTC (permalink / raw)
To: Laurent Pinchart, Thierry Reding, Stephen Warren, Mark Zhang
Cc: linux-kernel, linux-fbdev, linux-tegra, gnurou, Alexandre Courbot
In-Reply-To: <1359514939-15653-1-git-send-email-acourbot@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
arch/arm/configs/tegra_defconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index a7827fd..6da0de2 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -150,6 +150,8 @@ CONFIG_BACKLIGHT_LCD_SUPPORT=y
CONFIG_BACKLIGHT_CLASS_DEVICE=y
# CONFIG_BACKLIGHT_GENERIC is not set
CONFIG_BACKLIGHT_PWM=y
+CONFIG_DISPLAY_CORE=y
+CONFIG_DISPLAY_PANEL_CLAA101WA01A=y
CONFIG_FRAMEBUFFER_CONSOLE=y
CONFIG_LOGO=y
CONFIG_SOUND=y
--
1.8.1.1
^ permalink raw reply related
* [RFC 3/4] drm: tegra: use the Common Display Framework
From: Alexandre Courbot @ 2013-01-30 3:02 UTC (permalink / raw)
To: Laurent Pinchart, Thierry Reding, Stephen Warren, Mark Zhang
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, gnurou-Re5JQEeQqe8AvxtiuMwx3w,
Alexandre Courbot
In-Reply-To: <1359514939-15653-1-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Make the tegra-drm driver use the Common Display Framework, letting it
control the panel state according to the DPMS status.
A "nvidia,panel" property is added to the output node of the Tegra DC
that references the panel connected to a given output.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
.../bindings/gpu/nvidia,tegra20-host1x.txt | 2 +
drivers/gpu/drm/tegra/drm.h | 16 +++
drivers/gpu/drm/tegra/output.c | 118 ++++++++++++++++++++-
3 files changed, 134 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
index b4fa934..9c65e8e 100644
--- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
+++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
@@ -67,6 +67,7 @@ of the following host1x client modules:
- nvidia,ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
- nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
- nvidia,edid: supplies a binary EDID blob
+ - nvidia,panel: phandle of a display entity connected to this output
- hdmi: High Definition Multimedia Interface
@@ -81,6 +82,7 @@ of the following host1x client modules:
- nvidia,ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
- nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
- nvidia,edid: supplies a binary EDID blob
+ - nvidia,panel: phandle of a display entity connected to this output
- tvo: TV encoder output
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 741b5dc..5e63c56 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -17,6 +17,7 @@
#include <drm/drm_gem_cma_helper.h>
#include <drm/drm_fb_cma_helper.h>
#include <drm/drm_fixed.h>
+#include <video/display.h>
struct tegra_framebuffer {
struct drm_framebuffer base;
@@ -147,6 +148,9 @@ struct tegra_output {
struct drm_encoder encoder;
struct drm_connector connector;
+ struct display_entity this;
+ struct display_entity *output;
+ struct display_entity_notifier display_notifier;
};
static inline struct tegra_output *encoder_to_output(struct drm_encoder *e)
@@ -159,6 +163,18 @@ static inline struct tegra_output *connector_to_output(struct drm_connector *c)
return container_of(c, struct tegra_output, connector);
}
+static inline
+struct tegra_output *display_entity_to_output(struct display_entity *e)
+{
+ return container_of(e, struct tegra_output, this);
+}
+
+static inline struct tegra_output *
+display_notifier_to_output(struct display_entity_notifier *n)
+{
+ return container_of(n, struct tegra_output, display_notifier);
+}
+
static inline int tegra_output_enable(struct tegra_output *output)
{
if (output && output->ops && output->ops->enable)
diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index 8140fc6..d5bf37a 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -105,6 +105,29 @@ static const struct drm_encoder_funcs encoder_funcs = {
static void tegra_encoder_dpms(struct drm_encoder *encoder, int mode)
{
+ struct tegra_output *output = encoder_to_output(encoder);
+ enum display_entity_state state;
+
+ if (!output->output)
+ return;
+
+ switch (mode) {
+ case DRM_MODE_DPMS_ON:
+ state = DISPLAY_ENTITY_STATE_ON;
+ break;
+ case DRM_MODE_DPMS_STANDBY:
+ case DRM_MODE_DPMS_SUSPEND:
+ state = DISPLAY_ENTITY_STATE_STANDBY;
+ break;
+ case DRM_MODE_DPMS_OFF:
+ state = DISPLAY_ENTITY_STATE_OFF;
+ break;
+ default:
+ dev_warn(output->dev, "unknown DPMS state, ignoring\n");
+ return;
+ }
+
+ display_entity_set_state(output->output, state);
}
static bool tegra_encoder_mode_fixup(struct drm_encoder *encoder,
@@ -129,9 +152,14 @@ static void tegra_encoder_mode_set(struct drm_encoder *encoder,
struct tegra_output *output = encoder_to_output(encoder);
int err;
- err = tegra_output_enable(output);
+ if (output->output)
+ err = display_entity_set_state(output->output,
+ DISPLAY_ENTITY_STATE_ON);
+ else
+ err = tegra_output_enable(output);
+
if (err < 0)
- dev_err(encoder->dev->dev, "tegra_output_enable(): %d\n", err);
+ dev_err(encoder->dev->dev, "cannot enable output: %d\n", err);
}
static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
@@ -185,6 +213,69 @@ int tegra_output_parse_dt(struct tegra_output *output)
return 0;
}
+static int display_notify_callback(struct display_entity_notifier *notifier,
+ struct display_entity *entity, int event)
+{
+ struct tegra_output *output = display_notifier_to_output(notifier);
+ struct device_node *pnode;
+
+ switch (event) {
+ case DISPLAY_ENTITY_NOTIFIER_CONNECT:
+ if (output->output)
+ break;
+
+ pnode = of_parse_phandle(output->of_node, "nvidia,panel", 0);
+ if (!pnode)
+ break;
+
+ if (entity->dev && entity->dev->of_node = pnode) {
+ dev_dbg(output->dev, "connecting panel\n");
+ output->output = display_entity_get(entity);
+ display_entity_connect(&output->this, output->output);
+ }
+ of_node_put(pnode);
+
+ break;
+
+ case DISPLAY_ENTITY_NOTIFIER_DISCONNECT:
+ if (!output->output || output->output != entity)
+ break;
+
+ dev_dbg(output->dev, "disconnecting panel\n");
+ display_entity_disconnect(&output->this, output->output);
+ output->output = NULL;
+ display_entity_put(&output->this);
+
+ break;
+
+ default:
+ dev_dbg(output->dev, "unhandled display event\n");
+ break;
+ }
+
+ return 0;
+}
+
+static int tegra_output_set_stream(struct display_entity *entity,
+ enum display_entity_stream_state state)
+{
+ struct tegra_output *output = display_entity_to_output(entity);
+
+ switch (state) {
+ case DISPLAY_ENTITY_STATE_OFF:
+ case DISPLAY_ENTITY_STATE_STANDBY:
+ return output->ops->disable(output);
+ case DISPLAY_ENTITY_STATE_ON:
+ return output->ops->enable(output);
+ }
+
+ return 0;
+}
+
+static struct display_entity_video_ops tegra_output_video_ops = {
+ .set_stream = tegra_output_set_stream,
+};
+
int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
{
int connector, encoder, err;
@@ -250,6 +341,23 @@ int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
output->encoder.possible_crtcs = 0x3;
+ /* register display entity */
+ memset(&output->this, 0, sizeof(output->this));
+ output->this.dev = drm->dev;
+ output->this.ops.video = &tegra_output_video_ops;
+ err = display_entity_register(&output->this);
+ if (err) {
+ dev_err(output->dev, "cannot register display entity\n");
+ return err;
+ }
+
+ /* register display notifier */
+ output->display_notifier.dev = NULL;
+ output->display_notifier.notify = display_notify_callback;
+ err = display_entity_register_notifier(&output->display_notifier);
+ if (err)
+ return err;
+
return 0;
free_hpd:
@@ -260,6 +368,12 @@ free_hpd:
int tegra_output_exit(struct tegra_output *output)
{
+ if (output->output)
+ display_entity_put(output->output);
+
+ display_entity_unregister_notifier(&output->display_notifier);
+ display_entity_unregister(&output->this);
+
if (gpio_is_valid(output->hpd_gpio)) {
free_irq(output->hpd_irq, output);
gpio_free(output->hpd_gpio);
--
1.8.1.1
^ permalink raw reply related
* [RFC 2/4] tegra: ventana: add display and backlight DT nodes
From: Alexandre Courbot @ 2013-01-30 3:02 UTC (permalink / raw)
To: Laurent Pinchart, Thierry Reding, Stephen Warren, Mark Zhang
Cc: linux-kernel, linux-fbdev, linux-tegra, gnurou, Alexandre Courbot
In-Reply-To: <1359514939-15653-1-git-send-email-acourbot@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
arch/arm/boot/dts/tegra20-ventana.dts | 34 +++++++++++++++++++++++++++++++---
1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index adc4754..48f4e6d 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -10,6 +10,16 @@
reg = <0x00000000 0x40000000>;
};
+ host1x {
+ dc@54200000 {
+ rgb {
+ status = "okay";
+ nvidia,ddc-i2c-bus = <&lcd_ddc>;
+ nvidia,panel = <&panel>;
+ };
+ };
+ };
+
pinmux {
pinctrl-names = "default";
pinctrl-0 = <&state_default>;
@@ -341,7 +351,7 @@
#size-cells = <0>;
};
- i2c@1 {
+ lcd_ddc: i2c@1 {
reg = <1>;
#address-cells = <1>;
#size-cells = <0>;
@@ -516,6 +526,24 @@
bus-width = <8>;
};
+ backlight: backlight {
+ compatible = "pwm-backlight";
+ pwms = <&pwm 2 5000000>;
+
+ brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
+ default-brightness-level = <10>;
+ };
+
+ panel: panel {
+ compatible = "chunghwa,claa101wa01a";
+
+ pnl-supply = <&vdd_pnl_reg>;
+ pnl-enable-gpios = <&gpio 10 0>;
+
+ bl-supply = <&vdd_bl_reg>;
+ bl-enable-gpios = <&gpio 28 0>;
+ };
+
regulators {
compatible = "simple-bus";
#address-cells = <1>;
@@ -549,7 +577,7 @@
enable-active-high;
};
- regulator@3 {
+ vdd_pnl_reg: regulator@3 {
compatible = "regulator-fixed";
reg = <3>;
regulator-name = "vdd_pnl";
@@ -559,7 +587,7 @@
enable-active-high;
};
- regulator@4 {
+ vdd_bl_reg: regulator@4 {
compatible = "regulator-fixed";
reg = <4>;
regulator-name = "vdd_bl";
--
1.8.1.1
^ permalink raw reply related
* [RFC 1/4] video: panel: add CLAA101WA01A panel support
From: Alexandre Courbot @ 2013-01-30 3:02 UTC (permalink / raw)
To: Laurent Pinchart, Thierry Reding, Stephen Warren, Mark Zhang
Cc: linux-kernel, linux-fbdev, linux-tegra, gnurou, Alexandre Courbot
In-Reply-To: <1359514939-15653-1-git-send-email-acourbot@nvidia.com>
Add support for the Chunghwa CLAA101WA01A display panel.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
.../video/display/chunghwa,claa101wa01a.txt | 8 +
drivers/video/display/Kconfig | 8 +
drivers/video/display/Makefile | 1 +
drivers/video/display/panel-claa101wa01a.c | 209 +++++++++++++++++++++
4 files changed, 226 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt
create mode 100644 drivers/video/display/panel-claa101wa01a.c
diff --git a/Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt b/Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt
new file mode 100644
index 0000000..cfdc7fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt
@@ -0,0 +1,8 @@
+Chunghwa CLAA101WA01A Display Panel
+
+Required properties:
+- compatible: "chunghwa,claa101wa01a"
+- pnl-supply: regulator controlling power supply to the panel
+- bl-supply: regulator controlling power supply to the backlight
+- pnl-enable-gpios: GPIO that enables the panel
+- bl-enable-gpios: GPIO that enables the backlight
diff --git a/drivers/video/display/Kconfig b/drivers/video/display/Kconfig
index 9ca2e60..6902abb 100644
--- a/drivers/video/display/Kconfig
+++ b/drivers/video/display/Kconfig
@@ -32,4 +32,12 @@ config DISPLAY_PANEL_R61517
If you are in doubt, say N.
+config DISPLAY_PANEL_CLAA101WA01A
+ tristate "Chunghwa CLAA101WA01A Display Panel"
+ select BACKLIGHT_PWM
+ ---help---
+ Support for the Chunghwa CLAA101WA01A Display Panel.
+
+ If you are in doubt, say N.
+
endif # DISPLAY_CORE
diff --git a/drivers/video/display/Makefile b/drivers/video/display/Makefile
index ec557a1..19084a2 100644
--- a/drivers/video/display/Makefile
+++ b/drivers/video/display/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_DISPLAY_CORE) += display-core.o
obj-$(CONFIG_DISPLAY_PANEL_DPI) += panel-dpi.o
obj-$(CONFIG_DISPLAY_PANEL_R61505) += panel-r61505.o
obj-$(CONFIG_DISPLAY_PANEL_R61517) += panel-r61517.o
+obj-$(CONFIG_DISPLAY_PANEL_CLAA101WA01A) += panel-claa101wa01a.o
diff --git a/drivers/video/display/panel-claa101wa01a.c b/drivers/video/display/panel-claa101wa01a.c
new file mode 100644
index 0000000..93ae86b
--- /dev/null
+++ b/drivers/video/display/panel-claa101wa01a.c
@@ -0,0 +1,209 @@
+/*
+ * CLAA101WA01A Display Panel
+ *
+ * Copyright (C) 2013 NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+
+#include <video/display.h>
+
+#define CLAA101WA01A_WIDTH 223
+#define CLAA101WA01A_HEIGHT 125
+
+struct panel_claa101 {
+ struct display_entity entity;
+ struct regulator *vdd_pnl;
+ struct regulator *vdd_bl;
+ /* Enable GPIOs */
+ int pnl_enable;
+ int bl_enable;
+};
+
+#define to_panel_claa101(p) container_of(p, struct panel_claa101, entity)
+
+static int panel_claa101_set_state(struct display_entity *entity,
+ enum display_entity_state state)
+{
+ struct panel_claa101 *panel = to_panel_claa101(entity);
+
+ /* OFF and STANDBY are equivalent to us */
+ if (state = DISPLAY_ENTITY_STATE_STANDBY)
+ state = DISPLAY_ENTITY_STATE_OFF;
+
+ switch (state) {
+ case DISPLAY_ENTITY_STATE_OFF:
+ case DISPLAY_ENTITY_STATE_STANDBY:
+ if (entity->source)
+ display_entity_set_stream(entity->source,
+ DISPLAY_ENTITY_STREAM_STOPPED);
+
+ /* TODO error checking? */
+ gpio_set_value_cansleep(panel->bl_enable, 0);
+ usleep_range(10000, 10000);
+ regulator_disable(panel->vdd_bl);
+ usleep_range(200000, 200000);
+ gpio_set_value_cansleep(panel->pnl_enable, 0);
+ regulator_disable(panel->vdd_pnl);
+ break;
+
+ case DISPLAY_ENTITY_STATE_ON:
+ regulator_enable(panel->vdd_pnl);
+ gpio_set_value_cansleep(panel->pnl_enable, 1);
+ usleep_range(200000, 200000);
+ regulator_enable(panel->vdd_bl);
+ usleep_range(10000, 10000);
+ gpio_set_value_cansleep(panel->bl_enable, 1);
+
+ if (entity->source)
+ display_entity_set_stream(entity->source,
+ DISPLAY_ENTITY_STREAM_CONTINUOUS);
+ break;
+ }
+
+ return 0;
+}
+
+static int panel_claa101_get_modes(struct display_entity *entity,
+ const struct videomode **modes)
+{
+ /* TODO get modes from EDID? */
+ return 0;
+}
+
+static int panel_claa101_get_size(struct display_entity *entity,
+ unsigned int *width, unsigned int *height)
+{
+ *width = CLAA101WA01A_WIDTH;
+ *height = CLAA101WA01A_HEIGHT;
+
+ return 0;
+}
+
+static int panel_claa101_get_params(struct display_entity *entity,
+ struct display_entity_interface_params *params)
+{
+ return 0;
+}
+
+static const struct display_entity_control_ops panel_claa101_control_ops = {
+ .set_state = panel_claa101_set_state,
+ .get_modes = panel_claa101_get_modes,
+ .get_size = panel_claa101_get_size,
+ .get_params = panel_claa101_get_params,
+};
+
+static int __init panel_claa101_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct panel_claa101 *panel;
+ int err;
+
+ panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
+ if (!panel)
+ return -ENOMEM;
+
+ panel->vdd_pnl = devm_regulator_get(dev, "pnl");
+ if (IS_ERR(panel->vdd_pnl)) {
+ dev_err(dev, "cannot get vdd regulator\n");
+ return PTR_ERR(panel->vdd_pnl);
+ }
+
+ panel->vdd_bl = devm_regulator_get(dev, "bl");
+ if (IS_ERR(panel->vdd_bl)) {
+ dev_err(dev, "cannot get bl regulator\n");
+ return PTR_ERR(panel->vdd_bl);
+ }
+
+ err = of_get_named_gpio(dev->of_node, "pnl-enable-gpios", 0);
+ if (err < 0) {
+ dev_err(dev, "cannot find panel enable GPIO!\n");
+ return err;
+ }
+ panel->pnl_enable = err;
+ err = devm_gpio_request_one(dev, panel->pnl_enable,
+ GPIOF_DIR_OUT | GPIOF_INIT_LOW, "panel");
+ if (err < 0) {
+ dev_err(dev, "cannot acquire panel enable GPIO!\n");
+ return err;
+ }
+
+ err = of_get_named_gpio(dev->of_node, "bl-enable-gpios", 0);
+ if (err < 0) {
+ dev_err(dev, "cannot find backlight enable GPIO!\n");
+ return err;
+ }
+ panel->bl_enable = err;
+ err = devm_gpio_request_one(dev, panel->bl_enable,
+ GPIOF_DIR_OUT | GPIOF_INIT_LOW, "backlight");
+ if (err < 0) {
+ dev_err(dev, "cannot acquire backlight enable GPIO!\n");
+ return err;
+ }
+
+ panel->entity.dev = dev;
+ panel->entity.ops.ctrl = &panel_claa101_control_ops;
+ err = display_entity_register(&panel->entity);
+ if (err < 0)
+ return err;
+
+ platform_set_drvdata(pdev, panel);
+
+ dev_info(dev, "%s successful\n", __func__);
+
+ return 0;
+}
+
+static int __exit panel_claa101_remove(struct platform_device *pdev)
+{
+ struct panel_claa101 *panel = platform_get_drvdata(pdev);
+
+ display_entity_unregister(&panel->entity);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id panel_claa101_of_match[] = {
+ { .compatible = "chunghwa,claa101wa01a", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, pwm_backlight_of_match);
+#else
+#endif
+
+static const struct dev_pm_ops panel_claa101_dev_pm_ops = {
+};
+
+static struct platform_driver panel_claa101_driver = {
+ .probe = panel_claa101_probe,
+ .remove = panel_claa101_remove,
+ .driver = {
+ .name = "panel_claa101wa01a",
+ .owner = THIS_MODULE,
+#ifdef CONFIG_PM
+ .pm = &panel_claa101_dev_pm_ops,
+#endif
+#ifdef CONFIG_OF
+ .of_match_table = of_match_ptr(panel_claa101_of_match),
+#endif
+ },
+};
+
+module_platform_driver(panel_claa101_driver);
+
+MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
+MODULE_DESCRIPTION("Chunghwa CLAA101WA01A Display Panel");
+MODULE_LICENSE("GPL");
--
1.8.1.1
^ permalink raw reply related
* [RFC 0/4] Use the Common Display Framework in tegra-drm
From: Alexandre Courbot @ 2013-01-30 3:02 UTC (permalink / raw)
To: Laurent Pinchart, Thierry Reding, Stephen Warren, Mark Zhang
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, gnurou-Re5JQEeQqe8AvxtiuMwx3w,
Alexandre Courbot
This series leverages the (still work-in-progress) Common Display Framework to
add panel support to the tegra-drm driver. It also adds a driver for the
CLAA101WA01A panel used on the Ventana board.
The CDF is a moving target but Tegra needs some sort of display framework and
even in its current state the CDF seems to be the best candidate. Besides, by
using the CDF from now on we hope to provide useful feedback to Laurent and the
other CDF designers.
The changes to tegra-drm are rather minimal. Panels are referenced from Tegra DC
output nodes through the "nvidia,panel" property. This property is looked up
when a display connect notification is received in order to see if it points to
the connected display entity. If it does, the entity is then used for output.
The DPMS states are then propagated to the output entity, which is then supposed
to call back into the set_stream() hook in order to enable/disable the output
stream as needed.
Although the overall design seems to work ok, a few specific issues need to be
addressed:
1) The CDF has a get_modes() hook, but this is already implemented by
tegra_connector_get_modes(). Ideally everything should be moved to the CDF hook,
but Tegra's implementation uses DRM functions to retrieve the EDID and CDF
should, AFAIK, remain DRM-agnostic.
2) There is currently no panel/backlight interaction, e.g. backlight status is
controlled through FB events, independantly from the panel state. It could make
sense to have the panel DT node reference the backlight and control it as part
of its own power on/off sequence. Right now however, a backlight device cannot
ignore FB events.
3) Probably related to 2), now the backlight's power controls are part of the
panel driver, because the pwm-backlight driver cannot control the power
regulator and enable GPIO. This means that the backlight power is not turned off
when its brightness is set to 0 through sysfs. Once again this speaks in favor
of having stronger panel/backlight interaction: for instance, the panel driver
could reference the backlight and hijack its update_status() op to replace it by
one that does the correct power sequencing before calling the original function.
This would require some extra infrastructure though. Another possibility would
be to have a dedicated backlight driver for each panel, with its own
"compatible" string.
The code is based on 3.8rc5 + Steffen's videomode patches and the CDF v2.
Alexandre Courbot (4):
video: panel: add CLAA101WA01A panel support
tegra: ventana: add display and backlight DT nodes
drm: tegra: use the Common Display Framework
tegra: enable CDF and claa101 panel
.../bindings/gpu/nvidia,tegra20-host1x.txt | 2 +
.../video/display/chunghwa,claa101wa01a.txt | 8 +
arch/arm/boot/dts/tegra20-ventana.dts | 34 +++-
arch/arm/configs/tegra_defconfig | 2 +
drivers/gpu/drm/tegra/drm.h | 16 ++
drivers/gpu/drm/tegra/output.c | 118 +++++++++++-
drivers/video/display/Kconfig | 8 +
drivers/video/display/Makefile | 1 +
drivers/video/display/panel-claa101wa01a.c | 209 +++++++++++++++++++++
9 files changed, 393 insertions(+), 5 deletions(-)
create mode 100644 Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt
create mode 100644 drivers/video/display/panel-claa101wa01a.c
--
1.8.1.1
^ permalink raw reply
* Re: [patch] backlight: s6e63m0: report ->gamma_table_count correctly
From: Andrew Morton @ 2013-01-30 1:01 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Dan Carpenter', 'Inki Dae',
'Richard Purdie', 'Florian Tobias Schandinat',
linux-fbdev, kernel-janitors, linux-kernel
In-Reply-To: <00c601cdfaa2$c52d03c0$4f870b40$%han@samsung.com>
On Fri, 25 Jan 2013 11:22:06 +0900
Jingoo Han <jg1.han@samsung.com> wrote:
> On Thursday, January 24, 2013 10:45 PM, Dan Carpenter wrote
>
> CC'ed Andrew Morton, Inki Dae.
>
> >
> > gamma_table has 3 arrays which each hold MAX_GAMMA_LEVEL pointers to
> > int.
> >
> > The current code sets ->gamma_table_count to 6 on 64bit arches and to 3
> > on 32 bit arches. It should be 3 on everything.
>
> Actually, I don't know it is right.
> However, it is certain that this panel is currently used on 32 bit arches
> such as ARM SoCs.
I don't know what gamma_table_count is supposed to do. The only place
it is used is in s6e63m0_sysfs_show_gamma_table(). That function
doesn't actually show the table - it just prints out gamme_table_count.
Why is that useful?
Ho hum, the patch is clearly correct - the array stores int*'s and the
sysfs file should display "3" for all architectures. However I suspect
we could just remove the whole sysfs file and nobody would care...
^ permalink raw reply
* Re: [Linaro-mm-sig] CDF discussions at FOSDEM
From: Daniel Vetter @ 2013-01-29 21:46 UTC (permalink / raw)
To: Marcus Lorentzon
Cc: Linux Fbdev development list, Syrjala, Ville, Daniel Vetter,
dri-devel@lists.freedesktop.org, Linaro MM SIG, Laurent Pinchart,
Clark, Rob
In-Reply-To: <51082480.9070500@stericsson.com>
On Tue, Jan 29, 2013 at 08:35:28PM +0100, Marcus Lorentzon wrote:
> On 01/29/2013 04:50 PM, Daniel Vetter wrote:
> >On Tue, Jan 29, 2013 at 3:19 PM, Daniel Vetter<daniel.vetter@ffwll.ch> wrote:
> >Ok, in the interest of pre-heating the discussion a bit I've written down
> >my thoughts about display slave drivers. Adding a few more people and
> >lists to make sure I haven't missed anyone ...
> >
> >Cheers, Daniel
> >--
> >Display Slaves
> >=======
> >
> >A highly biased quick analysis from Daniel Vetter.
> And here is my biased version as one of the initiators of the idea of CDF.
Thanks a lot for your detailed answer. Some quick replies, I need to go
through this more carefully and maybe send another mail.
> I work with ARM SoCs (ST-Ericsson) and mobile devices (DSI/DPI
> panels). Of course some of these have the "PC" type of encoder
> devices like HDMI and eDP or even VGA. But from what I have seen
> most of these encoders are used by few different SoCs(GPUs?). And
> using these type of encoders was quite straight forward from DRM
> encoders. My goal was to get some common code of all the "mobile"
> panel encoders or "display module driver IC"s as some call them.
> Instead of tens of drivers (my assumption) you now have hundreds of
> drivers often using MIPI DSI/DPI/DBI or some similar interface. And
> lots of new come each year. There are probably more panel types than
> there are products on the market, since most products use more than
> one type of panel on the same product to secure sourcing for mass
> production (note multiple panels use same driver IC).
> So that was the initial goal, to cover all of these, which most are
> maintained per SoC/CPU out of kernel.org. If HDMI/DP etc fits in
> this framework, then that is just a nice bonus.
> I just wanted to give my history so we are not trying to include to
> many different types of encoders without an actual need. Maybe the
> I2C drm stuff is good enough for that type of encoders. But again,
> it would be nice with one suit that fits all ...
> I also like the idea to start out small. But if no support is added
> initially for the mobile panel types. Then I think it will be hard
> to get all vendors to start pushing those drivers, because the
> benefit of doing so would be small. But maybe the CDF work with
> Linaro and Laurent could just be a second step of adding the
> necessary details to your really simple baseline. And I also favor
> the helpers over framework approach but I miss a big piece which is
> the ops for panel drivers to call back to display controller (the
> video source stuff).
Yeah, I think we have two main goals here for enabling code sharing for
these output devices:
1. Basic panel support, with the panel usually glued onto the board, so
squat runtime configuration required. Aim is to get the gazillion of
out-of-tree drivers merged.
2. Allowing generic output encoder slaves to be used in a bunch of SoCs in.
Summarizing my previous mail I fear that if we start with with the first
point and don't take some of the mad features required to do the 2nd one
right into account, we'll end up at a rather ugly spot.
[cut]
> >- hdmi/dp helpers: HDMI/DP are both standardized output connectors with nice
> > complexity. DP is mostly about handling dp aux transactions and DPCD
> > registers, hdmi mostly about infoframes and how to correctly set them up from
> > the mode + edid.
> Yes, it is a mess. But we have managed to hide that below a simple
> panel API similar to CDF/omap so far.
Well, my concern is that we need to expose a bunch of special properties
(both to the master driver and ultimately to userspace) which are rather
hard to shovel through a simple panel abstraction. Ime from desktop
graphics there's no limits to the insane usecases and devices people come
up with and want to plug into your machine ;-)
> >- dpms is 4 states in drm, even more in fbdev afaict, but real hw only supports
> > on/off nowadays ... how should/do we care?
> Agreed, they should all really go away unless someone find a valid use case.
> >- Fancy modes and how to represent them. Random list of things we need to
> > represent somehow: broadcast/reduced rbg range for hdmi, yuv modes, different
> > bpc modes (and handling how this affects bandwidth/clocks, e.g. i915
> > auto-dithers to 6bpc on DP if there's not enough), 3D hdmi modes (patches have
> > floated on dri-devel for this), overscan compensation. Many of these things
> > link in with e.g. the helper libraries for certain outputs, e.g. discovering
> > DP sink capabilities or setting up the correct hdmi infoframe.
> Are you saying drm modes doesn't support this as of today? I have
> not used these types of modes in DRM yet. Maybe the common video
> mode patches is a good start.
All the stuff I've mentioned is support in drm/i915 (or at least we have
patches floating around), and on a quick look at the proposed video_mode I
couldn't fit this all in. Some of the features are fully fledged out, but
I expect that we fill all the little tiny holes in the next few releases.
> >- How to expose random madness as properties, e.g. backlight controllers,
> > broadcast mode, enable/disable embedded audio (some screens advertise it, but
> > don't like it). For additional fun I expect different users of a display slave
> > driver to expect different set of "standardized" properties.
> Some standardized properties would be nice :). Whatever is not
> standard doesn't really matter.
The problem is that we have a few 100klocs of driver code lying around in
upstream, so if we switch standards there's some decent fun involved
converting things. Or we need to add conversion functions all over the
place, which seems rather ugly, too.
> >- Debug support: Register dumping, exposing random debugfs files, tracing.
> > Preferably somewhat unified to keep things sane, since most often slave
> > drivers are rather simple, but we expect quite a few different ones.
> >
> >- Random metadata surrounding a display sink, like output type. Or flags for
> > support special modes (h/vsync polarity, interlaced/doublescan, pixel
> > doubling, ...).
> One thing that is needed is all the meta data related to the
> control/data interface between display controller and encoder.
> Because this has to be unified per interface type like DSI/DBI so
> the same CDF driver can setup different display controllers. But I
> hope we could split the "CDF API" (panel ops) from the control/data
> bus API (host/source ops or CDF video source).
I guess we have two options of panels on such buses with special needs:
- either add a bunch of optional functions to the common interfaces
- or subclass the common interface/struct and add additional magic in
there, i.e.
struct dsi_slave {
struct display_slave;
struct dsi_panel_ops;
/* whatever other magic we need for dsi, e.g. callbacks to the
* source for start/stopping pixel data ... */
}
The later requires a bit more casting of struct pointers, but should be
more flexible. Ime from i915 code it's not too onereous, e.g. for encoders
we nest such C struct classes about 4 levels deep in the code: drm_encoder
-> intel_encoder -> intel_dig_encoder -> intel_dp/hdmi/ddi
So I think both approaches are doable.
> >- mode_fixup: Used a lot in drm-land to allow encoders to change the input mode,
> > e.g. for lvds encoders which can do upscaling, or if the encoder supports
> > progressive input with interlaced output and similar fancy stuff. See e.g. the
> > intel sdvo encoder chip support.
> >
> >- Handling different control buses like i2c, direct access (not seen that yet),
> > DSI, DP aux, some other protocols.
> This is actually the place I wanted to start. With vendor specific
> panel drivers using common ops to access the bus (DSI/I2C/DBI etc).
> Then once we have a couple of panel drivers we could unify the API
> making them do their stuff (like the current CDF ops). Or even
> better, maybe these two could be made completely separate and worked
> on in parallel.
Hm, so starting with some DSI interface code, similarly to how we have
i2c? tbh I have pretty much zero clue about how dsi exactly works, but
growing different parts of a common panel infrastructure sounds
intriguing.
> >- Handling of different display data standards like dsi (intel invented a few of
> > its own, I'm sure we're not the only ones).
> >
> >- hpd support/polling. Depending upon desing hpd handling needs to be
> > cooperative between slave and master, or is a slave only thing (which means
> > the slave needs to be able to poke the master when something changes).
> > Similarly, masters need to know which slaves require output polling.
> I prefer a slave only thing forwarded to the drm encoder which I
> assume would be the drm equivalent of the display slave. At least I
> have not seen any need to involve the display controller in hpd
> (which I assume you mean by master).
I've used pretty unclear definitions. Generally master is everything no
behind the slave/panel interface. Call it display driver maybe ... For
this case I don't expect that hpd involves any piece of hw on the
master/driver side, but we need to somehow forward this to the usespace
interfaces. At least in drm, dunno what other display drivers do here.
> >- Initializing of slave drivers: of/devicetree based, compiled-in static tables
> > in the driver, dynamic discovery by i2c probing, lookup through some
> > platform-specific firmware table (ACPI). Related is how to forward random
> > platform init values to the drivers from these sources (e.g. the panel fixed
> > modes) to the slave driver.
> I'm not that familiar with the bios/uefi world. But on our SoCs we
> always have to show a splash screen from the boot loader (like bios,
> usually little kernel, uboot etc). And so all probing is done by
> bootloader and HW is running when kernel boot. And you are not
> allowed to disrupt it either because that would yield visual
> glitches during boot. So some way or the other the boot loader would
> need to transfer the state to the kernel or you would have to
> reverse engineer the state from hw at kernel probe.
Actually reverse engineer the bios state from the actual hw state is what
we now do for i915 ;-) Which is why we need the ->get_hw_state callback in
some form. But that's just a result of some of the horrible things old
firmware does, it /should/ be better on newer platforms. And hopefully the
embedded ones aren't that massively screwed up ... Iirc the only current
interface exposed by ACPI lets you get at the vendor boot splash and
display it after you've taken over the hw.
> >- get_hw_state support. One of the major point in the i915 modeset rewrite which
> > landed in 3.7 is that a lot of the hw state can be cross-checked with the sw
> > tracking. Helps tremendously in tracking down driver (writer) fumbles ;-)
> This sounds more like a display controller feature than a display
> slave feature.
See above for why we have that in i915. And we do call down into slave
encoders (Intel (s)dvo standards) on older hw. Might be we won't need that
any more on SoC platforms (I do hope that's the case at least).
> >- PSR/dsi command mode and how the start/stop frame dance should be handled.
> Again, a vital piece for the many mobile driver ICs. And I think we
> have several sources (STE, Renesas, TI, Samsung, ...) on how to do
> this and tested in many products. So I hope this could be an early
> step in the evolution.
One issue with start/stop callbacks I've discussed a bit with Jani Nikula
and Rob Clark is locking rules around start/stop callbacks from the slave
to the display source. Especially how to handle fun like blocking the dsi
bus while we need to wait for the transfer window.
> >- Random funny expectations around the modeset sequence, i.e. when (and how
> > often) the video stream should be enabled/disabled. In the worst case this
> > needs some serious cooperation between master and slaves. Even more fun for
> > trained output links like DP where a re-training and so restarting parts - or
> > even the complete - modeset sequence could be required to happen any time.
> Again, we have several samples of platforms already doing this
> stuff. So we should be able to get a draft pretty early. From my
> experience when to enable/disable video stream could vary between
> versions of the same display controller. So I think it could be
> pretty hairy to get a single solution for all. Instead I think we
> need to leave some room for the master/slave to decide when to
> enable/disable. And to be able to do this we should try to have
> pretty specific ops on the slave and master. I'm not sure
> prepare/modeset/commit is specific enough unless we document what is
> expected to be done by the slave in each of these.
Well, drm/i915 killed prepare/modeset/commit ops, we now have our own
which semantics matching our hw. My concern here is mostly about fancier
display buses with link training - e.g. on DP you can't just start/stop
the pixel stream, but there's a nice dance involved to do it.
> >- There's more I'm sure, gfx hw tends to be insane ...
> Yes, and one is the chain of slaves issue that is "common" on mobile
> systems. One example I have is
> dispc->dsi->dsi2dsi-bridge->dsi2lvds-bridge->lvds-panel.
> My proposal to hide this complexity in CDF was aggregate drivers. So
> from drm there will only be one master (dispc) and one slave
> (dsi2dsi). Then dsi2dsi will itself use another CDF/slave driver to
> talk to its slave. This way the top master (dispc) driver never have
> to care about this complexity. Whether this is possible to hide in
> practice we will see ...
I think even more fun would be to replace the lvds endpoint with hdmi, and
the try to coax the infoframe control attributes down that pipeline (plus
who's responsibilty it is to do the various adjustments to the pixels).
[cut]
> >- I think we should reduce the scope of the intial version massively and instead
> > increase the depth to fully cover everything. So instead of something which
> > covers everything of a limited use-case from discover, setup, modes handling
> > and mode-setting, concentrate on only one operation. The actual mode-set seems
> > to be the best case, since it usually involves a lot of the boring register
> > bashing code. The first interface version would ignore everything else
> > completely.
> To also cover and be useful to mobile panels I suggest starting with
> on/off using a fixed mode initially. Because modeset is not used for
> most mobile panels (they only have one mode).
Would that be start/stop a frame for manual refresh or enable/disable the
display itself? Just curious what you're aiming for as the minimal useful
thing here ...
> >- Shot for the most powerful api for that little piece we're starting with, make
> > it the canonical thing. I.e. for modeset we need a video mode thing, and imo
> > it only makes sense if that's the native data structure for all invovled
> > subsystems. At least it should be the aim. Yeah, that means tons of work. Even
> > more important is that the new datastructure supports every feature already
> > support in some insane way in one of the existing subsystems. Imo if we keep
> > different datastructures everywhere, the impendance matching will eat up most
> > of the code sharing benefits.
> >
> >- Since converting all invovled subsystems we should imo just forget about
> > fbdev. For obvious reasons I'm also leaning towards simply ditching the
> > drm prefix from the drm defines and using those ;-)
> >
> >- I haven't used it in a driver yet, but mandating regmap (might need some
> > improvements) should get us decent unification between drivers. And hopefully
> > also an easy way to have unified debug tools. regmap already has trace points
> > and a few other cool things.
> Guideline for I2C slave drivers maybe? Do we really want to enforce
> how drivers are implemented when it doesn't affect the API?
> Also, I don't think it fits in general for slaves. Since DSI/DBI
> have not only registers but also operations you can execute using
> control interface.
Yeah, that was an idea for i2c guidelines. I guess if we have a different
(sub)type for DSI we could gather helpers somewhere which are useful only
for DSI. E.g. drm is in the process of growing some DP helpers shared
among a few drivers.
My idea behind being a bit more anal about standardization is that we
exect tons of these drivers, and also that lots of different SoC platforms
might share them. So trying to make them look similar and work in similar
ways (where reasonable) to help enable existing drivers on new SoCs and
debug isssue seemed like something we should discuss a bit.
> >- We need some built-in way to drill direct paths from the master display driver
> > to the slave driver for the different subsystems. Jumping through hoops (or
> > even making it impossible) to extend drivers in funny ways would be a big step
> > backwards.
> >
> >- Locking will be fun, especially once we start to add slave->master callbacks
> > (e.g. for stopping/starting the display signal, hpd interrupts, ...). As a
> > general rule I think we should aim for no locks in the slave driver, with the
> > master owning the slave and ensure exclusion with its own locks. Slaves which
> > use shared resources and so need locks (everything doing i2c actually) may not
> > call master callback functions with locks held.
> Agreed, and I think we should rely on upper layers like drm as much
> as possible for locking.
> >Then, once we've gotten things of the ground and have some slave encoder drivers
> >which are actually shared between different subsystems/drivers/platforms or
> >whatever we can start to organically grow more common interfaces. Ime it's much
> >easier to simply extract decent interfaces after the fact than trying to come
> >up.
> >
> >Now let's pour this into a more concrete form:
> >
> >struct display_slave_ops {
> > /* modeset ops, e.g. prepare/modset/commit from drm */
> >};
> >
> >struct display_slave {
> > struct display_slave_ops *ops;
> > void *driver_private;
> >};
> >
> >I think even just that will be worth a lot of flames to come up with a good and
> >agreeable interface for everyone. It'll probably satisfactory to no one though.
> >
> >Then each subsystem adds it's own magic, e.g.
> >
> >struct drm_encoder_slave {
> > struct display_slave slave;
> >
> > /* everything else which is there already and not covered by the display
> > * slave interface. */
> >};
> I like the starting point. Hard to make it any more simple ;). But
> next step would probably follow quickly. I also like the idea to
> have current drivers aggregate the slave to make transition easier.
> CDF as it is now is an all or nothing API. And since you don't care
> how slaves interact with master (bus ops) I see the possibility
> still to separate "CDI device API" and "CDF bus API". Which would
> allow using DSI bus API for DSI panels and I2C bus API (or regmap)
> for I2C encoders instead of force use of the video source API in all
> slave drivers.
I didn't follow here which pieces you'd like to cut apart along which
lines exactly ... Maybe some example structs or asci-art to help the
clueless?
Aside about the simplicity of the above: It's slightly tongue-in-check, I
expect it to be a bit feature-full ;-) Just wanted to direct the
discussion a bit into a minimal, but still useful interface, highly
extensible.
[cut]
> >But imo the key part is that we aim for real unification in the users of
> >display_slave's, so internally convert over everything to the new structures.
> >That should also make code-sharing much easier, so that we could move existing
> >helper functions to the common display helper library.
> What about drivers that are waiting for CDF to be pushed upstream
> instead of having to push another custom panel framework? I'm
> talking of my own KMS driver ... but maybe I could put most of it in
> staging and move relevant parts of DSI/DPI/HDMI panel drivers to
> "common" slave drivers ...
Hm, I think I've missed your driver drm/kms driver. Links to source? I
think reading through a drm driver using the current cdf would be nice,
that way I'm at least familiar with one part of the code ;-)
> >Bikesheds
> >---------
> >
> >I.e. the boring details:
> >
> >- Where to put slave drivers? I'll vote for anything which does not include
> > drivers/video ;-)
> drivers/video +1, drivers/gpu -1, who came up with putting KMS under
> drivers/gpu ;)
I think the main reason was to be as far away from fbdev/fbcon code as
possible ;-) Also, we have gem/ttm in drm, which is all about PU part and
not really about G ..
> >- Maybe we want to start with a different part than modeset, or add a bit more
> > on top. Though I really think we should start minimally and modesetting seemed
> > like the most useful piece of the puzzle.
> As suggested, start with on/off and static/fixed mode would help
> single resolution LCDs. Actually that is almost all that is needed
> for mobile panels and what I intended to get from CDF :)
> >
> >- Naming the new interfaces. I'll have more asbestos suites on order ...
> Until you get them. Would it make sense to reuse the encoder name
> from drm or is that to restrictive?
On a quick check drm lacks names for DSI encoders/panels, so we might want
to add those. And maybe a generic panel output type. I guess it would be
good to take my caveats list above and strike off everything we don't need
for basic dsi panel support, then figure out where to steal the
definitions from. Common definitions will be hard to come by, e.g. after
much bikesheds and deciding to use common fourcc codes for pixel layouts
drm ended up with simply adding a bunch of its own fourcc codes since the
ones negotiated with v4l didn't cut it.
> >- Can we just copy the new "native" interface structs from drm, pls?
> I hope you are not talking about the helper interfaces at least ;).
Nope, the drm helpers are not the interfaces. Ofc, if we end up with a
massively generic panel interface, we might add a few helpers to give
slave/panel drivers an easy way to opt for sane default behaviour. E.g.
handling a fixed panel mode and always returning that mode is something
which is reinvented in drm a few times ...
I probably should have written metadata structs/definitions, since that'll
be the part which could get ugly if we end up with diverging standards.
Interface functions obviously need to fit into what the hw bus at hand
requires us to do (e.g. for DSI special cases).
[Aside wrt drm helpers: With i915 we now have an imo rather nice example
that the drm crtc are really just helpers, and that it's not too hard to
come up with your own modeset infrastructure. On an established driver
codebase even.]
> But if CDF is going to be the new drm helpers of choice for
> encoder/connector parts. Then it sounds like CDF would replace most
> of the old helpers. It would be far to many layers with the old
> helpers too. And I think I recall Jesse wanting to deprecate/remove
> them too.
Rob's tilcdc driver uses the drm crtc helpers and for the i2c encoder
slaves he added a new set of helpers to easier integrate the crtc helpers
with the existing drm_encoder_slave infrastructure. The end-result looks
fairly reasonable imo.
In general I think as long as we aim for the different libraries to be
as orthogonal as possible so that drivers can pick and choose, more kinds
of helpers doesn't really sound bad. On the drm side I've recently brushed
up the crtc/output polling and fb helpers quite a bit, so drivers can now
pick&choose (and i915 does only use some of them). Similarly for other
helper ideas floating around like DSI, hdmi infoframe handling, dp aux
stuff ...
Of course I expect that we'll wrap things up into dwim() functions for all
the common cases.
> Hopefully we could have some generic encoder/connector helper
> implementations that only depend on CDF.
I'm not sure whether we should aim for that really - having a slave/panel
driver with mostly common code and a wee bit of shim code once for drm and
once for dss (or whatever else is out there) doesn't sound too horrible to
me. But I agree that at least for new code we should aim to get this right
from the start.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Alexander Holler @ 2013-01-29 20:56 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-fbdev, Florian Tobias Schandinat,
Bernie Thompson, Steve Glendinning, stable
In-Reply-To: <5108329E.2050802@ahsoftware.de>
Am 29.01.2013 21:35, schrieb Alexander Holler:
>
> So, if the above down_timeout_killable() is only down_interruptible(),
> as in kernel 3.7.5, the box would not shutdown afterwards, because on
> shutdown no signal would be send to that kernel-thread which called
> dlfb_free_urb_list().
>
> A last note: dlfb_usb_disconnect() (thus dlfb_free_urb_list()) isn't
> called on shutdown if the device would still be connected. So the
> problem only might happen, if the screen will be disconnected before
> shutdown (and an urb gets missed). So the subject of my patch is
> correct. ;)
>
> </monologue>
Hmm, wrong, sorry, I still have something to add: As no signal arrives
at all, v1 of that patch is enough and the implementation of
down_timeout_killable() isn't necessary at all.
If there is a chance that the patch would be Acked-by by someone, I
would made a v3, replacing
+ ret = down_timeout(&dev->urbs.limit_sem, GET_URB_TIMEOUT);
in v1 with
+ ret = down_timeout(&dev->urbs.limit_sem, FREE_URB_TIMEOUT);
as this seems to be what it should be.
Regards,
Alexander
^ permalink raw reply
* Re: [PATCH 2/3 v2] fb: udlfb: fix hang at disconnect
From: Alexander Holler @ 2013-01-29 20:35 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-fbdev, Florian Tobias Schandinat,
Bernie Thompson, Steve Glendinning, stable
In-Reply-To: <5107F014.4030704@ahsoftware.de>
Am 29.01.2013 16:51, schrieb Alexander Holler:
> Am 29.01.2013 12:11, schrieb Alexander Holler:
>
>>
>> To explain the problem on shutdown a bit further, I think the following
>> happens (usb and driver are statically linked and started by the kernel):
>>
>> shutdown -> kill signal -> usb stack shuts down -> udlfb waits (forever)
>> for a kill or an urb which it doesn't get.
>
> Having a second look at what I've written above, I'm not even sure if
> the kernel sends one or more fatal signals on shutdown at all. I've just
> assumed it because otherwise down_interruptible() wouldn't have worked
> before (it would have stalled on shutdown too (if an urb got missed),
> not only on disconnect).
>
> Sounds like an interesting question I should read about (if/when fatal
> signals are issued by the kernel). ;)
>
>> Maybe the sequence is different if the usb-stack and udlfb are used as a
>> module and/or udlfb is used only for X/fb. I'm not sure what actually
>> does shut down the usb-stack in such a case, but maybe more than one
>> kill signal might be thrown around.
If anyone still follows my monologue: The question was interesting
enough that I couldn't resist for long. ;)
(all pasted => format broken)
In drivers/tty/sysrq.c there is
------
static void send_sig_all(int sig)
{
struct task_struct *p;
read_lock(&tasklist_lock);
for_each_process(p) {
if (p->flags & PF_KTHREAD)
continue;
if (is_global_init(p))
continue;
do_send_sig_info(sig, SEND_SIG_FORCED, p, true);
}
read_unlock(&tasklist_lock);
}
static void sysrq_handle_term(int key)
{
send_sig_all(SIGTERM);
console_loglevel = 8;
}
(...)
static void sysrq_handle_kill(int key)
{
send_sig_all(SIGKILL);
console_loglevel = 8;
}
------
Now I've done some learning by doing (kernel 3.7.5 + some patches):
------
diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
index df249f3..db8a86c 100644
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -1876,14 +1876,18 @@ static void dlfb_free_urb_list(struct dlfb_data
*dev)
unsigned long flags;
pr_notice("Freeing all render urbs\n");
+ if (current->flags & PF_KTHREAD)
+ pr_info("AHO: I'm a kernel thread\n");
/* keep waiting and freeing, until we've got 'em all */
while (count--) {
/* Timeout likely occurs at disconnect (resulting in a
leak) */
ret = down_timeout_killable(&dev->urbs.limit_sem,
FREE_URB_TIMEOUT);
- if (ret)
+ if (ret) {
+ pr_info("AHO: ret %d\n", ret);
break;
+ }
spin_lock_irqsave(&dev->urbs.lock, flags);
------
Now I've disconnected the display. And, as send_sig_all() already
suggests, the result was (besides discovering an oops in
call_timer_fn.isra (once)):
------
[ 120.963010] udlfb: AHO: I'm a kernel thread
[ 122.957024] udlfb: AHO: ret -62
------
(-62 is -ETIME)
So, if the above down_timeout_killable() is only down_interruptible(),
as in kernel 3.7.5, the box would not shutdown afterwards, because on
shutdown no signal would be send to that kernel-thread which called
dlfb_free_urb_list().
A last note: dlfb_usb_disconnect() (thus dlfb_free_urb_list()) isn't
called on shutdown if the device would still be connected. So the
problem only might happen, if the screen will be disconnected before
shutdown (and an urb gets missed). So the subject of my patch is correct. ;)
</monologue>
Regards,
Alexander
^ permalink raw reply related
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