[PATCH 4/4] ARM: tegra: roth: add display DT node
On 07/02/2014 09:10 PM, Alexandre Courbot wrote:
> On 07/03/2014 12:55 AM, Stephen Warren wrote:
>> On 07/02/2014 06:19 AM, Alexandre Courbot wrote:
>>> DSI support has been fixed to support continuous clock behavior that the
>>> panel used on SHIELD requires, so finally add its device tree node since
>>> it is functional.
>>
>>> diff --git a/arch/arm/boot/dts/tegra114-roth.dts
>>> b/arch/arm/boot/dts/tegra114-roth.dts
>>
>>> +host1x at 5000 {
>>> +dsi at 5430 {
>>
>> That node looks fine, but I wonder why we need to mark a bunch of
>> regulators as always-on? shouldn't the references to vdd-supply and
>> power-supply from this node be enough? If not, perhaps the tree
>> structure of the regulators isn't correct, or the DSI or panel bindings
>> don't allow enough regulators to be referenced?
>
> regulator-always-on is indeed not needed for vdd_lcd. I actually had a
> patch in my tree removing this line that I forgot to squash. Will post a
> v2 for this patch that fixes this, thanks.
>
> vdd-2v8-display needs to remain always-on however. Here we may hit a
> limitation of the simple-panel driver, where only one power supply can
> be provided.
Can't we fix the simple-panel driver to allow a list of regulators in
the property?
I suppose that this technique is OK though; we can always simplify the
DT later if the simple-panel binding gets enhanced.
[PATCH V2] drm/tegra: add MODULE_DEVICE_TABLEs
On 06/18/2014 04:21 PM, Stephen Warren wrote: > From: Stephen Warren > > When tegra-drm.ko is built as a module, these MODULE_DEVICE_TABLEs allow > the module to be auto-loaded since the module will match the devices > instantiated from device tree. > > (Notes for stable: in 3.14+, just git rm any conflicting file, since they > are added in later kernels. For 3.13 and below, manual merging will be > needed) > > Cc: > Signed-off-by: Stephen Warren > --- > v2: Remove change to drm.c, since the match table there isn't used for > probing. Thierry, I think this patch got lost? It doesn't appear in next-20140721 anyway.
[RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver
On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
> Adds functionality for registering memory bandwidth needs and setting
> the EMC clock rate based on that.
>
> Also adds API for setting floor and ceiling frequency rates.
> diff --git
> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
> new file mode 100644
> index 000..88e6a55
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra124-emc.txt
> @@ -0,0 +1,26 @@
> +Tegra124 External Memory Controller
> +
> +Properties:
> +- compatible : Should contain "nvidia,tegra124-emc".
> +- reg : Should contain the register range of the device
> +- #address-cells : Should be 1
> +- #size-cells : Should be 0
> +- nvidia,mc : phandle to the mc bus connected to EMC.
> +- clocks : phandle to EMC, EMC shared bus override, and all parent clocks.
> +- clock-names : name of each clock.
> +- nvidia,pmc : phandle to the PMC syscon node.
> +- max-clock-frequency : optional, specifies the maximum EMC rate in kHz.
> +
> +Child device nodes describe the memory settings for different configurations
> and
> +clock rates.
How do the child nodes do that? The binding needs to specify the format
of the child node. This binding looks quite anaemic vs.
Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt; I
would expect that this binding needs all the EMC register data from the
tegra20-emc binding too. Can the two bindings be identical?
Can you explain what the nvidia,mc and nvidia,pmc references are needed
for? Hopefully, this driver isn't going to reach into those devices and
touch their registers directly.
> diff --git a/include/linux/platform_data/tegra_emc.h
> b/include/linux/platform_data/tegra_emc.h
A header file that defines platform data format isn't the correct place
to put the definitions of public APIs. I'd expect something more like
.
> +#ifdef CONFIG_TEGRA124_EMC
> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long
> rate);
> +void tegra124_emc_set_floor(unsigned long freq);
> +void tegra124_emc_set_ceiling(unsigned long freq);
> +#else
> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned long rate)
> +{ return -ENODEV; }
> +void tegra124_emc_set_floor(unsigned long freq)
> +{ return; }
> +void tegra124_emc_set_ceiling(unsigned long freq)
> +{ return; }
> +#endif
I'll repeat what I said off-list so that we can have the whole
conversation on the list:
That looks like a custom Tegra-specific API. I think it'd be much better
to integrate this into the common clock framework as a standard clock
constraints API. There are other use-cases for clock constraints besides
EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
SoCs too).
See https://lkml.org/lkml/2014/5/16/569 for some previous discussion on
this topic.
[RFC PATCH 3/4] drm/tegra: Request memory bandwidth for the display controller
On 06/16/2014 07:35 AM, Tomeu Vizoso wrote: > Request it based solely on the current mode's refresh rate. More > accurate requirements can be requested in future patches. > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > + bandwidth = mode->clock * window.bits_per_pixel / 8; > + err = tegra124_emc_reserve_bandwidth(TEGRA_EMC_CONSUMER_DISP1, > bandwidth); DISP1 shouldn't be hard-coded here; the code should use DISP1 or DISP2 based on head or DC identity. We certainly have some boards capable of dual-head operation.
[RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver
On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>> This binding looks quite anaemic vs.
>> Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt; I
>> would expect that this binding needs all the EMC register data from the
>> tegra20-emc binding too. Can the two bindings be identical?
>
> There's even less stuff needed right now, as all what ultimately the EMC
> driver does is call clk_set_rate on the EMC clock. As the T124 EMC
> driver gains more features, they should get more similar.
IIRC, even changing the EMC clock rate requires modifying the memory
controller's programming (e.g. delays/taps/tuning etc.). That's exactly
what the more complex stuff in the nvidia,tegra20-emc.txt is all about.
I not convinced that a driver that just modifies the clock rate without
adjusting the EMC programming will work reliably.
>>> +#ifdef CONFIG_TEGRA124_EMC
>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>> long rate);
>>> +void tegra124_emc_set_floor(unsigned long freq);
>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>> +#else
>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>> long rate)
>>> +{ return -ENODEV; }
>>> +void tegra124_emc_set_floor(unsigned long freq)
>>> +{ return; }
>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>> +{ return; }
>>> +#endif
>>
>> I'll repeat what I said off-list so that we can have the whole
>> conversation on the list:
>>
>> That looks like a custom Tegra-specific API. I think it'd be much better
>> to integrate this into the common clock framework as a standard clock
>> constraints API. There are other use-cases for clock constraints besides
>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>> SoCs too).
>
> Yes, I wrote a bit in the cover letter about our requirements and how
> they map to the CCF. Could you please comment on that?
My comments remain the same. I believe this is something that belongs in
the clock driver, or at the least, some API that takes a struct clock as
its parameter, so that drivers can use the existing DT clock lookup
mechanism.
[RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver
On 06/18/2014 11:23 AM, Tomeu Vizoso wrote:
> On 06/17/2014 06:15 PM, Stephen Warren wrote:
>> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>>>>> +#ifdef CONFIG_TEGRA124_EMC
>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>> long rate);
>>>>> +void tegra124_emc_set_floor(unsigned long freq);
>>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>>>> +#else
>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>> long rate)
>>>>> +{ return -ENODEV; }
>>>>> +void tegra124_emc_set_floor(unsigned long freq)
>>>>> +{ return; }
>>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>>>> +{ return; }
>>>>> +#endif
>>>>
>>>> I'll repeat what I said off-list so that we can have the whole
>>>> conversation on the list:
>>>>
>>>> That looks like a custom Tegra-specific API. I think it'd be much
>>>> better
>>>> to integrate this into the common clock framework as a standard clock
>>>> constraints API. There are other use-cases for clock constraints
>>>> besides
>>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>>>> SoCs too).
>>>
>>> Yes, I wrote a bit in the cover letter about our requirements and how
>>> they map to the CCF. Could you please comment on that?
>>
>> My comments remain the same. I believe this is something that belongs in
>> the clock driver, or at the least, some API that takes a struct clock as
>> its parameter, so that drivers can use the existing DT clock lookup
>> mechanism.
>
> Ok, let me put this strawman here to see if I have gotten close to what
> you have in mind:
>
> * add per-client accounting (Rabin's patches referenced before)
>
> * add clk_set_floor, to be used by cpufreq, load stats, etc.
>
> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
Yes. I'd expect those to be maintained per-client, and so the clock core
(or whatever higher level code implements clk_set_floor/ceiling)
performs the logic that "blends" together all the different requests
from different clients.
As an aside, for audio usage, I would expect clk_set_rate to be a
per-client (rather than per HW clock) operation too, and to error out if
one client says it wants to set pll_a to the rate needed for
44.1KHz-based audio and a different client wants the rate for
48KHz-based audio.
> * an EMC driver would collect bandwidth and latency requests from
> consumers and call clk_set_floor on the EMC clock.
>
> * the EMC driver would also register for rate change notifications in
> the EMC clock and would update the latency allowance registers at that
> point.
>
> How does it sound?
At a high level, yes this sounds about right to me.
[PATCH] drm/tegra: add MODULE_DEVICE_TABLEs
From: Stephen Warren
When tegra-drm.ko is built as a module, these MODULE_DEVICE_TABLEs allow
the module to be auto-loaded since the module will match the devices
instantiated from device tree.
(Notes for stable: in 3.14+, just git rm any conflicting file, since they
are added in later kernels. For 3.13 and below, manual merging will be
needed)
Cc:
Signed-off-by: Stephen Warren
---
drivers/gpu/drm/tegra/dc.c| 1 +
drivers/gpu/drm/tegra/dpaux.c | 1 +
drivers/gpu/drm/tegra/drm.c | 1 +
drivers/gpu/drm/tegra/dsi.c | 1 +
drivers/gpu/drm/tegra/gr2d.c | 1 +
drivers/gpu/drm/tegra/gr3d.c | 1 +
drivers/gpu/drm/tegra/hdmi.c | 1 +
drivers/gpu/drm/tegra/sor.c | 1 +
8 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index ef40381f3909..48c3bc460eef 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1303,6 +1303,7 @@ static const struct of_device_id tegra_dc_of_match[] = {
/* sentinel */
}
};
+MODULE_DEVICE_TABLE(of, tegra_dc_of_match);
static int tegra_dc_parse_dt(struct tegra_dc *dc)
{
diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
index 3f132e356e9c..708f783ead47 100644
--- a/drivers/gpu/drm/tegra/dpaux.c
+++ b/drivers/gpu/drm/tegra/dpaux.c
@@ -382,6 +382,7 @@ static const struct of_device_id tegra_dpaux_of_match[] = {
{ .compatible = "nvidia,tegra124-dpaux", },
{ },
};
+MODULE_DEVICE_TABLE(of, tegra_dpaux_of_match);
struct platform_driver tegra_dpaux_driver = {
.driver = {
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 3396f9f6a9f7..a1f9b06a75d5 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -694,6 +694,7 @@ static const struct of_device_id host1x_drm_subdevs[] = {
{ .compatible = "nvidia,tegra124-hdmi", },
{ /* sentinel */ }
};
+MODULE_DEVICE_TABLE(of, host1x_drm_subdevs);
static struct host1x_driver host1x_drm_driver = {
.name = "drm",
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index bd56f2affa78..97c409f10456 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -982,6 +982,7 @@ static const struct of_device_id tegra_dsi_of_match[] = {
{ .compatible = "nvidia,tegra114-dsi", },
{ },
};
+MODULE_DEVICE_TABLE(of, tegra_dsi_of_match);
struct platform_driver tegra_dsi_driver = {
.driver = {
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index 7c53941f2a9e..02cd3e37a6ec 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -121,6 +121,7 @@ static const struct of_device_id gr2d_match[] = {
{ .compatible = "nvidia,tegra20-gr2d" },
{ },
};
+MODULE_DEVICE_TABLE(of, gr2d_match);
static const u32 gr2d_addr_regs[] = {
GR2D_UA_BASE_ADDR,
diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index 30f5ba9bd6d0..2bea2b2d204e 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -130,6 +130,7 @@ static const struct of_device_id tegra_gr3d_match[] = {
{ .compatible = "nvidia,tegra20-gr3d" },
{ }
};
+MODULE_DEVICE_TABLE(of, tegra_gr3d_match);
static const u32 gr3d_addr_regs[] = {
GR3D_IDX_ATTRIBUTE( 0),
diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
index ba067bb767e3..ffe26547328d 100644
--- a/drivers/gpu/drm/tegra/hdmi.c
+++ b/drivers/gpu/drm/tegra/hdmi.c
@@ -1450,6 +1450,7 @@ static const struct of_device_id tegra_hdmi_of_match[] = {
{ .compatible = "nvidia,tegra20-hdmi", .data = &tegra20_hdmi_config },
{ },
};
+MODULE_DEVICE_TABLE(of, tegra_hdmi_of_match);
static int tegra_hdmi_probe(struct platform_device *pdev)
{
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index 27c979b50111..061a5c501124 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -1455,6 +1455,7 @@ static const struct of_device_id tegra_sor_of_match[] = {
{ .compatible = "nvidia,tegra124-sor", },
{ },
};
+MODULE_DEVICE_TABLE(of, tegra_sor_of_match);
struct platform_driver tegra_sor_driver = {
.driver = {
--
1.8.1.5
[RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver
On 06/18/2014 04:03 PM, Thierry Reding wrote:
> On Wed, Jun 18, 2014 at 11:46:49AM -0600, Stephen Warren wrote:
>> On 06/18/2014 11:23 AM, Tomeu Vizoso wrote:
>>> On 06/17/2014 06:15 PM, Stephen Warren wrote:
>>>> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>>>>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>>>>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>>>>>>> +#ifdef CONFIG_TEGRA124_EMC
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate);
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq);
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>>>>>> +#else
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate)
>>>>>>> +{ return -ENODEV; }
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +#endif
>>>>>>
>>>>>> I'll repeat what I said off-list so that we can have the whole
>>>>>> conversation on the list:
>>>>>>
>>>>>> That looks like a custom Tegra-specific API. I think it'd be much
>>>>>> better
>>>>>> to integrate this into the common clock framework as a standard clock
>>>>>> constraints API. There are other use-cases for clock constraints
>>>>>> besides
>>>>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>>>>>> SoCs too).
>>>>>
>>>>> Yes, I wrote a bit in the cover letter about our requirements and how
>>>>> they map to the CCF. Could you please comment on that?
>>>>
>>>> My comments remain the same. I believe this is something that belongs in
>>>> the clock driver, or at the least, some API that takes a struct clock as
>>>> its parameter, so that drivers can use the existing DT clock lookup
>>>> mechanism.
>>>
>>> Ok, let me put this strawman here to see if I have gotten close to what
>>> you have in mind:
>>>
>>> * add per-client accounting (Rabin's patches referenced before)
>>>
>>> * add clk_set_floor, to be used by cpufreq, load stats, etc.
>>>
>>> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
>>
>> Yes. I'd expect those to be maintained per-client, and so the clock core
>> (or whatever higher level code implements clk_set_floor/ceiling)
>> performs the logic that "blends" together all the different requests
>> from different clients.
>>
>> As an aside, for audio usage, I would expect clk_set_rate to be a
>> per-client (rather than per HW clock) operation too, and to error out if
>> one client says it wants to set pll_a to the rate needed for
>> 44.1KHz-based audio and a different client wants the rate for
>> 48KHz-based audio.
>
> From what I remember, Mike was fairly strongly opposing the idea of
> virtual clocks, but what you're proposing here sounds like it would
> assume the existence of virtual clocks. clk_set_rate() per client
> doesn't work with the current API as I understand it.
>
> Or perhaps what you're proposing isn't about the individual clocks at
> all but rather about a mechanism to express constraints for a set of
> clocks?
This doesn't have anything to do with virtual clocks. As you mention,
it's just about constraints.
One user of clock "cpu" wants min rate 216MHz. Another wants max rate
1GHz. cpufreq will request some rate between the 2, or be capped to
those limits. These set of imposed constraints would need to be stored
per client of the clock, not per HW clock, since many clients could set
different max rates (e.g. thermal throttle 1.5GHz due to temperature,
CPU policy 1GHz due to the user selecting low CPU power, etc.)
Similarly for audio, of there are N clients of 1 clock/PLL, and they
each want the PLL to run at a different rate, something needs to detect
that and deny it.
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140618/c955e82b/attachment.sig>
[PATCH] drm/tegra: add MODULE_DEVICE_TABLEs
On 06/18/2014 03:51 PM, Thierry Reding wrote: > On Wed, Jun 18, 2014 at 03:19:15PM -0600, Stephen Warren wrote: >> From: Stephen Warren >> >> When tegra-drm.ko is built as a module, these MODULE_DEVICE_TABLEs allow >> the module to be auto-loaded since the module will match the devices >> instantiated from device tree. > > I vaguely remember doing something like this a while back and getting a > bunch of link-time errors. But I assume that you've tested this, so I > must be remembering wrongly. Were the problems due to: a) Simply building the tegradrm driver as modules. I vaguely recall some runtime issues with tegradrm as a module, but I'm not sure about build issues. I don't think this patch could make this any worse. b) Building as modules works, but adding MODULE_DEVICE_TABLE broke that. This seems unlikely since *many* module in the kernel have a MODULE_DEVICE_TABLE... Certainly, with this patch applied, building tegradrm as a module in next-20140611 works out just fine, and the code runs fine too. Building tegra_defconfig (which has tegradrm builtin) on Linus' master with this patch applied also works out fine. I'll post v2 with the issue you mentioned addressed. -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 901 bytes Desc: OpenPGP digital signature URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140618/12b77b6c/attachment.sig>
[PATCH V2] drm/tegra: add MODULE_DEVICE_TABLEs
From: Stephen Warren
When tegra-drm.ko is built as a module, these MODULE_DEVICE_TABLEs allow
the module to be auto-loaded since the module will match the devices
instantiated from device tree.
(Notes for stable: in 3.14+, just git rm any conflicting file, since they
are added in later kernels. For 3.13 and below, manual merging will be
needed)
Cc:
Signed-off-by: Stephen Warren
---
v2: Remove change to drm.c, since the match table there isn't used for
probing.
---
drivers/gpu/drm/tegra/dc.c| 1 +
drivers/gpu/drm/tegra/dpaux.c | 1 +
drivers/gpu/drm/tegra/dsi.c | 1 +
drivers/gpu/drm/tegra/gr2d.c | 1 +
drivers/gpu/drm/tegra/gr3d.c | 1 +
drivers/gpu/drm/tegra/hdmi.c | 1 +
drivers/gpu/drm/tegra/sor.c | 1 +
7 files changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index ef40381f3909..48c3bc460eef 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1303,6 +1303,7 @@ static const struct of_device_id tegra_dc_of_match[] = {
/* sentinel */
}
};
+MODULE_DEVICE_TABLE(of, tegra_dc_of_match);
static int tegra_dc_parse_dt(struct tegra_dc *dc)
{
diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
index 3f132e356e9c..708f783ead47 100644
--- a/drivers/gpu/drm/tegra/dpaux.c
+++ b/drivers/gpu/drm/tegra/dpaux.c
@@ -382,6 +382,7 @@ static const struct of_device_id tegra_dpaux_of_match[] = {
{ .compatible = "nvidia,tegra124-dpaux", },
{ },
};
+MODULE_DEVICE_TABLE(of, tegra_dpaux_of_match);
struct platform_driver tegra_dpaux_driver = {
.driver = {
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index bd56f2affa78..97c409f10456 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -982,6 +982,7 @@ static const struct of_device_id tegra_dsi_of_match[] = {
{ .compatible = "nvidia,tegra114-dsi", },
{ },
};
+MODULE_DEVICE_TABLE(of, tegra_dsi_of_match);
struct platform_driver tegra_dsi_driver = {
.driver = {
diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
index 7c53941f2a9e..02cd3e37a6ec 100644
--- a/drivers/gpu/drm/tegra/gr2d.c
+++ b/drivers/gpu/drm/tegra/gr2d.c
@@ -121,6 +121,7 @@ static const struct of_device_id gr2d_match[] = {
{ .compatible = "nvidia,tegra20-gr2d" },
{ },
};
+MODULE_DEVICE_TABLE(of, gr2d_match);
static const u32 gr2d_addr_regs[] = {
GR2D_UA_BASE_ADDR,
diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
index 30f5ba9bd6d0..2bea2b2d204e 100644
--- a/drivers/gpu/drm/tegra/gr3d.c
+++ b/drivers/gpu/drm/tegra/gr3d.c
@@ -130,6 +130,7 @@ static const struct of_device_id tegra_gr3d_match[] = {
{ .compatible = "nvidia,tegra20-gr3d" },
{ }
};
+MODULE_DEVICE_TABLE(of, tegra_gr3d_match);
static const u32 gr3d_addr_regs[] = {
GR3D_IDX_ATTRIBUTE( 0),
diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
index a0b8d8539d07..84ea0c8b47f7 100644
--- a/drivers/gpu/drm/tegra/hdmi.c
+++ b/drivers/gpu/drm/tegra/hdmi.c
@@ -1370,6 +1370,7 @@ static const struct of_device_id tegra_hdmi_of_match[] = {
{ .compatible = "nvidia,tegra20-hdmi", .data = &tegra20_hdmi_config },
{ },
};
+MODULE_DEVICE_TABLE(of, tegra_hdmi_of_match);
static int tegra_hdmi_probe(struct platform_device *pdev)
{
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index 27c979b50111..061a5c501124 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -1455,6 +1455,7 @@ static const struct of_device_id tegra_sor_of_match[] = {
{ .compatible = "nvidia,tegra124-sor", },
{ },
};
+MODULE_DEVICE_TABLE(of, tegra_sor_of_match);
struct platform_driver tegra_sor_driver = {
.driver = {
--
1.8.1.5
[RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver
On 06/18/2014 04:19 PM, St?phane Marchesin wrote:
> On Wed, Jun 18, 2014 at 3:00 PM, Thierry Reding
> wrote:
>> On Wed, Jun 18, 2014 at 07:23:47PM +0200, Tomeu Vizoso wrote:
>>> On 06/17/2014 06:15 PM, Stephen Warren wrote:
>>>> On 06/17/2014 06:16 AM, Tomeu Vizoso wrote:
>>>>> On 06/16/2014 10:02 PM, Stephen Warren wrote:
>>>>>> On 06/16/2014 07:35 AM, Tomeu Vizoso wrote:
>>>>>>> +#ifdef CONFIG_TEGRA124_EMC
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate);
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq);
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq);
>>>>>>> +#else
>>>>>>> +int tegra124_emc_reserve_bandwidth(unsigned int consumer, unsigned
>>>>>>> long rate)
>>>>>>> +{ return -ENODEV; }
>>>>>>> +void tegra124_emc_set_floor(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +void tegra124_emc_set_ceiling(unsigned long freq)
>>>>>>> +{ return; }
>>>>>>> +#endif
>>>>>>
>>>>>> I'll repeat what I said off-list so that we can have the whole
>>>>>> conversation on the list:
>>>>>>
>>>>>> That looks like a custom Tegra-specific API. I think it'd be much better
>>>>>> to integrate this into the common clock framework as a standard clock
>>>>>> constraints API. There are other use-cases for clock constraints besides
>>>>>> EMC scaling (e.g. some in audio on Tegra, and I'm sure many on other
>>>>>> SoCs too).
>>>>>
>>>>> Yes, I wrote a bit in the cover letter about our requirements and how
>>>>> they map to the CCF. Could you please comment on that?
>>>>
>>>> My comments remain the same. I believe this is something that belongs in
>>>> the clock driver, or at the least, some API that takes a struct clock as
>>>> its parameter, so that drivers can use the existing DT clock lookup
>>>> mechanism.
>>>
>>> Ok, let me put this strawman here to see if I have gotten close to what you
>>> have in mind:
>>>
>>> * add per-client accounting (Rabin's patches referenced before)
>>>
>>> * add clk_set_floor, to be used by cpufreq, load stats, etc.
>>>
>>> * add clk_set_ceiling, to be used by battery drivers, thermal, etc.
>>>
>>> * an EMC driver would collect bandwidth and latency requests from consumers
>>> and call clk_set_floor on the EMC clock.
>>>
>>> * the EMC driver would also register for rate change notifications in the
>>> EMC clock and would update the latency allowance registers at that point.
>>
>> Latency allowance registers are part of the MC rather than the EMC. So I
>> think we have two options: a) have a unified driver for MC and EMC or b)
>> provide two parts of the API in two drivers.
>>
>> Or perhaps c), create a generic framework that both MC and EMC can
>> register with (bandwidth for EMC, latency for MC).
>
> Is there any motivation for keeping MC and EMC separate? In my mind,
> the solution was always to handle those together.
Well, they are documented as being separate HW modules in the TRM.
I know there's an interlock in HW so that when the EMC clock is changed,
the EMC registers can flip atomically to a new configuration.
I'm not aware of any similar HW interlock between MC and EMC registers.
That would imply that very tight co-ordination shouldn't be required.
Do the MC latency allowance registers /really/ need to be *very tightly*
managed whenever the EMC clock is changed, or is it just a matter of it
being a good idea to update EMC clock and MC latency allowance registers
at roughly the same time? Even if there's some co-ordination required,
maybe it can be handled rather like cpufreq notifications; use clock
pre-rate change notifications to set MC up in a way that'll work at both
old/new EMC clocks, and then clock post-rate notifications to the final
MC configuration?
[RFC PATCH 1/4] memory: tegra124-emc: Add EMC driver
On 06/18/2014 05:14 PM, Thierry Reding wrote: > On Wed, Jun 18, 2014 at 04:09:06PM -0600, Stephen Warren wrote: >> On 06/18/2014 04:03 PM, Thierry Reding wrote: ... >>> From what I remember, Mike was fairly strongly opposing the idea of >>> virtual clocks, but what you're proposing here sounds like it would >>> assume the existence of virtual clocks. clk_set_rate() per client >>> doesn't work with the current API as I understand it. >>> >>> Or perhaps what you're proposing isn't about the individual clocks at >>> all but rather about a mechanism to express constraints for a set of >>> clocks? >> >> This doesn't have anything to do with virtual clocks. As you mention, >> it's just about constraints. >> >> One user of clock "cpu" wants min rate 216MHz. Another wants max rate >> 1GHz. cpufreq will request some rate between the 2, or be capped to >> those limits. These set of imposed constraints would need to be stored >> per client of the clock, not per HW clock, since many clients could set >> different max rates (e.g. thermal throttle 1.5GHz due to temperature, >> CPU policy 1GHz due to the user selecting low CPU power, etc.) >> >> Similarly for audio, of there are N clients of 1 clock/PLL, and they >> each want the PLL to run at a different rate, something needs to detect >> that and deny it. > > I'm wondering how this should work with the current API. Could the clock > core be modified to return a per-client struct clk * that references the > hardware clock internally? Or do we need to add a new API? I would assume the we can just change struct clk and hide the details from any driver. Hopefully only clock-core and clock-drivers would need any changes. -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 901 bytes Desc: OpenPGP digital signature URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140618/e86187fa/attachment.sig>
Build break due to 28ec711 "drm/agp: move AGP cleanup paths to drm_agpsupport.c"
In next-20130808, building tegra_defconfig for ARM yields: > drivers/built-in.o: In function `drm_lastclose': > /home/swarren/shared/git_wa/kernel/kernel.git/drivers/gpu/drm/drm_drv.c:198: > undefined reference to `drm_agp_clear' That's because drm_agp_clear() is called unconditionally, yet is only conditionally built into drm_agpsupport.c (#if __OS_HAS_AGP). Should the call from drm_drv.c be conditional, or should there be a dummy static inline replacement in include/drm/drmP.h for when AGP support isn't available? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Build break due to 28ec711 "drm/agp: move AGP cleanup paths to drm_agpsupport.c"
On 08/08/2013 12:13 PM, David Herrmann wrote: > Hi > > On Thu, Aug 8, 2013 at 8:00 PM, Stephen Warren wrote: >> In next-20130808, building tegra_defconfig for ARM yields: >> >>> drivers/built-in.o: In function `drm_lastclose': >>> /home/swarren/shared/git_wa/kernel/kernel.git/drivers/gpu/drm/drm_drv.c:198: >>> undefined reference to `drm_agp_clear' >> >> That's because drm_agp_clear() is called unconditionally, yet is only >> conditionally built into drm_agpsupport.c (#if __OS_HAS_AGP). >> >> Should the call from drm_drv.c be conditional, or should there be a >> dummy static inline replacement in include/drm/drmP.h for when AGP >> support isn't available? > > Sorry, I missed testing with AGP=n and the code I fixed depended on > dead-code-elimination to link correctly. I overlooked that. There is a > patch pending on dri-devel: > http://lists.freedesktop.org/archives/dri-devel/2013-August/043077.html That makes it worse! To solve it, you need: diff --git a/include/drm/drm_agpsupport.h b/include/drm/drm_agpsupport.h index f926542..a184eee 100644 --- a/include/drm/drm_agpsupport.h +++ b/include/drm/drm_agpsupport.h @@ -8,7 +8,7 @@ #include #include -#ifdef __OS_HAS_AGP +#if __OS_HAS_AGP void drm_free_agp(DRM_AGP_MEM * handle, int pages); int drm_bind_agp(DRM_AGP_MEM * handle, unsigned int start); ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm: provide agp dummies for CONFIG_AGP=n
On 08/08/2013 02:19 PM, David Herrmann wrote: > We currently rely on gcc dead-code elimination so the drm_agp_* helpers > are not called if drm_core_has_AGP() is false. That's ugly as hell so > provide "static inline" dummies for the case that AGP is disabled. > > Fixes a build-regression introduced by: > > commit 28ec711cd427f8b61f73712a43b8100ba8ca933b > Author: David Herrmann > Date: Sat Jul 27 16:37:00 2013 +0200 > > drm/agp: move AGP cleanup paths to drm_agpsupport.c Tested-by: Stephen Warren ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator
On 08/09/2013 07:15 AM, Tomasz Figa wrote: > Hi Chanho, > > On Friday 09 of August 2013 16:40:53 Chanho Park wrote: >> This patch describes each nodes of rotator and specifies a example how to >> bind it. >> diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt >> +* Samsung Image Rotator >> + >> +Required properties: >> + - compatible : value should be following: >> +(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210 >> +(b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412 >> +(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250 Is "rotator" the name that the HW documentation uses to refer to this block? If so, those compatible values seem fine. If not, they seem slightly too generic for me; perhaps better to use the raw HW block name? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [patch 2/2] gpu: host1x: returning success instead of -ENOMEM
On 08/23/2013 04:19 AM, Dan Carpenter wrote: > There is a mistake here so it returns PTR_ERR(NULL) which is success > instead of -ENOMEM. > > Signed-off-by: Dan Carpenter > --- > I can't compile this. For the record, just do: export CROSS_COMPILE=xxx make ARCH=arm tegra_defconfig make ARCH=arm zImage The only slightly difficult part is getting a CROSS_COMPILE value. Many distros ship at least some ARM cross-compiler in their standard package-set these days, and if not, you can download the Linaro compilers or go to ftp://ftp.kernel.org/pub/tools/crosstool/index.html. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 2/3] drm/panel: Add simple panel support
On 08/30/2013 09:25 AM, Thierry Reding wrote: > Add a driver for simple panels. Such panels can have a regulator that > provides the supply voltage and a separate GPIO to enable the panel. > Optionally the panels can have a backlight associated with them so it > can be enabled or disabled according to the panel's power management > mode. > diff --git a/Documentation/devicetree/bindings/panel/panel-simple.txt > b/Documentation/devicetree/bindings/panel/panel-simple.txt > +Simple display panel > + > +Required properties: > +- compatible: should be one of: > + - "auo,b101aw03": AU Optronics Corporation 10.1" WSVGA TFT LCD panel > + - "cptt,claa101wb03": Chunghwa Picture Tubes Ltd. 10.1" WXGA TFT LCD panel > + - "pc,vvx10f004b00": Panasonic Corporation 10.1" WUXGA TFT LCD panel > + > +Optional properties: > +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing > +- power-supply: regulator to provide the supply voltage > +- enable-gpios: GPIO pin to enable or disable the panel > +- backlight: phandle of the backlight device attached to the panel Do we need to represent the timing requirements e.g. between panel enable and backlight enable, or do you expect the driver to hard-code this based on the compatible value? Looking at the driver code, it seems that it has direct knowledge of the video mode that each panel supports, so DDC is actually optional unlike what I asserted/assumed in my previous response. As such, I guess we should have a separate DT binding document for each of the three panels (compatible values) listed above that pretty much just says "go look at simple-panel.txt". ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 2/3] drm/panel: Add simple panel support
On 08/30/2013 01:24 PM, Kumar Gala wrote: > > On Aug 30, 2013, at 10:25 AM, Thierry Reding wrote: > >> Add a driver for simple panels. Such panels can have a regulator that >> provides the supply voltage and a separate GPIO to enable the panel. >> Optionally the panels can have a backlight associated with them so it >> can be enabled or disabled according to the panel's power management >> mode. >> >> Support is added for three panels: An AU Optronics 10.1" WSVGA, a >> Chunghwa Picture Tubes 10.1" WXGA and a Panasonic 10.1 WUXGA TFT LCD >> panel. >> diff --git a/Documentation/devicetree/bindings/panel/panel-simple.txt >> b/Documentation/devicetree/bindings/panel/panel-simple.txt >> +Required properties: >> +- compatible: should be one of: >> + - "auo,b101aw03": AU Optronics Corporation 10.1" WSVGA TFT LCD panel >> + - "cptt,claa101wb03": Chunghwa Picture Tubes Ltd. 10.1" WXGA TFT LCD panel >> + - "pc,vvx10f004b00": Panasonic Corporation 10.1" WUXGA TFT LCD panel >> + > > It would seem there should be a more generic compatible to cover the basic > "panel-simple" case. I would suggest only documenting "simple-panel" here, and not documenting the specific panels at all; the panel-specific compatible values would show up simply due to the rule that all compatible values in *.dts should contain the exact HW model (e.g. "auo,b101aw03"), plus any HW they're compatible with (i.e. "simple-panel"). I'd suggest "simple-panel" rather than "panel-simple" since IIRC other simple bindings are "simple-xxx" rather than "xxx-simple". >> +Optional properties: >> +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing >> +- power-supply: regulator to provide the supply voltage >> +- enable-gpios: GPIO pin to enable or disable the panel >> +- backlight: phandle of the backlight device attached to the panel >> + > > If these are all optional, what does it mean to be a "panel-simple"? I think at least ddc-i2c-bus and backlight should be required properties. I suppose it might be possible for the panel to be always-on, and hence enable-gpios/power-supply could be optional? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/6] host1x: hdmi: Add Tegra114 support
On 08/28/2013 09:48 AM, Mikko Perttunen wrote:
> Add Tegra114 TMDS configuration, add new peak_current field and
> use new place for drive current override bit on Tegra114 platform.
> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> static struct of_device_id host1x_of_match[] = {
> + { .compatible = "nvidia,tegra114-host1x", .data = &host1x01_info, },
We should add that value to the host1x DT binding documentation.
> diff --git a/drivers/gpu/host1x/drm/hdmi.c b/drivers/gpu/host1x/drm/hdmi.c
> static const struct tegra_hdmi_audio_config *
> tegra_hdmi_get_audio_config(unsigned int audio_freq, unsigned int pclk)
> {
> @@ -593,8 +673,20 @@ static void tegra_hdmi_setup_tmds(struct tegra_hdmi
> *hdmi,
> tegra_hdmi_writel(hdmi, tmds->pll1, HDMI_NV_PDISP_SOR_PLL1);
> tegra_hdmi_writel(hdmi, tmds->pe_current, HDMI_NV_PDISP_PE_CURRENT);
>
> - value = tmds->drive_current | DRIVE_CURRENT_FUSE_OVERRIDE;
> - tegra_hdmi_writel(hdmi, value, HDMI_NV_PDISP_SOR_LANE_DRIVE_CURRENT);
> + if (of_device_is_compatible(hdmi->dev->of_node,
> + "nvidia,tegra114-hdmi")) {
We shouldn't do this at run-time. Rather, set tegra_hdmi_of_match[]'s
.data field to a structure that represents the various features of the
HW, and then make this code conditional upon a feature flag in that
structure.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/6] host1x: hdmi: Detect whether display is connected with HDMI or DVI
On 08/28/2013 09:48 AM, Mikko Perttunen wrote: > Use EDID data to determine whether the display supports HDMI or just DVI. > This used to be hardcoded to be HDMI, which broke support for DVI displays > that couldn't understand the interspersed audio/other data. > > If the EDID data isn't available, default to DVI, which should be a safer > choice. This seems like a bug-fix that might even be worth CC: stable? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/6] host1x: hdmi: Enable Vdd earlier for hotplug/DDC
On 08/28/2013 09:48 AM, Mikko Perttunen wrote: > The Vdd regulator used to be enabled only at tegra_output_hdmi_enable, > which is called after a sink is detected. However, the HDMI hotplug pin > works by returning the voltage supplied by the Vdd pin, so this meant > that the hotplug pin was never asserted and the sink was not detected > unless the Vdd regulator was set to be always on. > > This patch moves the enable to the tegra_hdmi_drm_init function to make > sure the regulator will get enabled. The DT binding document isn't very clear on this topic (and should be fixed): What is this regulator intended to control? If this regulator solely controls the supply to the hotplug detection circuit, this change makes sense. If the regulator mainly supplies something else (e.g. part of the HDMI core on the Tegra chip), then perhaps this change isn't correct. The correct approach might be to introduce another (optional) regulator specifically for the hotplug circuit. Presumably both DT properties vdd-supply and hotplug-supply could point at the same regulator if that's the way the HW was wired up. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/6] clk: tegra114: Initialize clocks needed for HDMI
On 08/28/2013 09:48 AM, Mikko Perttunen wrote: > Add host1x, disp1 and disp2 clocks to the clock initialization table. > These clocks are required for HDMI support. This patch should be sent to Mike Turquette so it can be taken through the clock tree. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 5/6] ARM: tegra: Add host1x, DC and HDMI to Tegra114 device tree
On 08/28/2013 09:48 AM, Mikko Perttunen wrote: > Add host1x, DC (display controller) and HDMI devices to Tegra114 > device tree. Patches 5 and 6 should be sent separately to the Tegra maintainer (me) to be taken through the Tegra tree. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/6] host1x: hdmi: Enable Vdd earlier for hotplug/DDC
On 09/04/2013 04:03 PM, Mikko Perttunen wrote: > On 09/04/2013 09:44 PM, Stephen Warren wrote: >> On 08/28/2013 09:48 AM, Mikko Perttunen wrote: >>> The Vdd regulator used to be enabled only at tegra_output_hdmi_enable, >>> which is called after a sink is detected. However, the HDMI hotplug pin >>> works by returning the voltage supplied by the Vdd pin, so this meant >>> that the hotplug pin was never asserted and the sink was not detected >>> unless the Vdd regulator was set to be always on. >>> >>> This patch moves the enable to the tegra_hdmi_drm_init function to make >>> sure the regulator will get enabled. >> >> The DT binding document isn't very clear on this topic (and should be >> fixed): What is this regulator intended to control? If this regulator >> solely controls the supply to the hotplug detection circuit, this change >> makes sense. If the regulator mainly supplies something else (e.g. part >> of the HDMI core on the Tegra chip), then perhaps this change isn't >> correct. The correct approach might be to introduce another (optional) >> regulator specifically for the hotplug circuit. Presumably both DT >> properties vdd-supply and hotplug-supply could point at the same >> regulator if that's the way the HW was wired up. >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > AFAICT, it controls the Vdd pin on the HDMI port, so it just affects the > hotplug pin and the DDC I2C bus power. Ah OK, then this code change makes sense. It'd be useful to put what you just wrote into the binding doc. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv3 2/4] drm/tegra: Add runtime pm support for gr2d
On 09/24/2013 06:05 AM, Arto Merilainen wrote:
> From: Mayuresh Kulkarni
>
> This far we have enabled gr2d clock on device probe and disabled
> it on device deinitialisation. This patch adds runtime pm support
> for the hardware unit allowing dynamic power management. If pm
> runtime is not enabled, gr2d clock is enabled in device probe and
> diff --git a/drivers/gpu/host1x/drm/gr2d.c b/drivers/gpu/host1x/drm/gr2d.c
> @@ -327,11 +336,48 @@ static int __exit gr2d_remove(struct platform_device
> *pdev)
> host1x_syncpt_free(gr2d->client.syncpts[i]);
>
> host1x_channel_free(gr2d->channel);
> +
> + if (pm_runtime_enabled(&pdev->dev))
> + pm_runtime_disable(&pdev->dev);
> + else
> + gr2d_runtime_suspend(&pdev->dev);
This code is slightly different to the code in e.g.
sound/soc/tegra/tegra20_i2s.c:remove(), whereas the code in probe() is
identical. I'm not sure whether there's some advantage in this version?
If so, perhaps the sound drivers should be updated to be consistent. If
not, perhaps this driver should do the same thing as the I2S driver, so
we keep the drivers consistent, and provide the same "example" code
everywhere.
> +static int gr2d_runtime_suspend(struct device *dev)
> +{
> + struct gr2d *gr2d;
> +
> + gr2d = dev_get_drvdata(dev);
> + if (!gr2d)
> + return -EINVAL;
Presumably, gr2d will never be NULL here, unless there's some chronic
bug. Can't we re-write those last 5 lines as simply:
struct gr2d *grd2 = dev_get_drvdata(dev);
If that's not valid, we should probably update the audio drivers (and
perhaps others) too.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv3 4/4] gpu: host1x: Add runtime pm support for host1x
On 09/24/2013 06:05 AM, Arto Merilainen wrote: > From: Mayuresh Kulkarni > > This patch adds runtime pm support for host1x hardware unit. This > allows host1x clock to be turned off when it is idle. If pm runtime > is not configured, we enable host1x clock in device probe and disable > it in remove. > diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c > +static int host1x_runtime_suspend(struct device *dev); > +static int host1x_runtime_resume(struct device *dev); You could avoid having these prototypes by simply putting the function bodies earlier on in the file, somewhere before they're used. I don't care much either way, but I've certainly seen some people care about this and ask for them to be moved. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3] drm/panel: Add support for EDT ETM0700G0DH6 and ET070080DH6 panels
On 05/15/2014 03:41 PM, Thierry Reding wrote: > On Thu, May 15, 2014 at 09:54:07AM -0600, Stephen Warren wrote: >> On 05/15/2014 04:54 AM, Thierry Reding wrote: >>> On Thu, May 15, 2014 at 12:25:47PM +0200, Philipp Zabel wrote: >>>> The EDT ETM0700G0DH6 and ET070080DH6 are 7" 800x480 panels, >>>> which can be supported by the simple panel driver. >>>> >>>> Signed-off-by: Philipp Zabel >>>> --- >>>> Changes since v2: >>>> - Added device tree binding documentation. Do we really want to add one >>>> little >>>>file for each panel? >>> >>> I guess we could move all of the compatible values into simple-panel.txt >>> but I don't see a need for that yet. >> >> If they aren't added there, then I believe checkpatch will complain >> about patches that start to use the new compatible values, since they >> won't be documented. > > Added where? simple-panel.txt or in separate files per compatible? I assume that checkpatch simply checks all files in Documentation/devicetree/bindings. By "there", I meant "somewhere in that directory". Perhaps I misinterpreted the email I was replying to; I thought you'd meant that we didn't need to document it yet, but it looks like you were simply discussing where to document it. Sorry for the noise.
[PATCH 2/5] ARM: tegra: of: add GK20A device tree binding
On 05/19/2014 03:24 AM, Alexandre Courbot wrote:
> Add the device tree binding documentation for the GK20A GPU used in
> Tegra K1 SoCs.
A few minor nits, but otherwise,
Acked-by: Stephen Warren
> diff --git a/Documentation/devicetree/bindings/gpu/nvidia,gk20a.txt
> b/Documentation/devicetree/bindings/gpu/nvidia,gk20a.txt
> +Required properties:
> +- compatible: "nvidia,-"
> + Currently recognized values:
> + - nvidia,tegra124-gk20a
> +- reg: Physical base address and length of the controller's registers.
> + Must contain two entries:
> + - first entry for bar0
> + - second entry for bar1
> +- interrupts: The interrupt outputs from the controller.
To be consistent with the clocks and resets properties, it'd be nice to
reword that as:
interrupts: Must contain an entry for each entry in interrupt-names.
> +- interrupt-names: Must include the following entries:
... and add the following here:
See ../interrupt-controller/interrupts.txt
> +/ {
No need to wrap a root node around this in the example.
> + gpu at 0,5700 {
...
> + };
> +
Extra blank line here.
> +};
[PATCH 3/5] ARM: tegra: add GK20A GPU to Tegra124 DT
On 05/19/2014 03:24 AM, Alexandre Courbot wrote: > From: Thierry Reding > > Add the GK20A device node to Tegra124's device tree. At a quick glance, patches 3-5 look fine too. I'll hold off on applying them until patches 1-2 have been applied to the DRM/... tree.
[PATCH 3/7] drm/vc4: Add KMS support for Raspberry Pi.
On 08/12/2015 06:56 PM, Eric Anholt wrote: > This is the start of a full VC4 driver. Right now this just supports > configuring the display using a pre-existing video mode (because > changing the pixel clock isn't available yet, and doesn't work when it > is). However, this is enough for fbcon and bringing up X using > xf86-video-modesetting. > diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig > +config DRM_VC4 > + tristate "Broadcom VC4 Graphics" > + help > + Choose this option if you have a system that has a Broadcom > + VC4 GPU, such as the Raspberry Pi or other BCM2708/BCM2835. > + > + This driver requires that "avoid_warnings=2" be present in > + the config.txt for the firmware, to keep it from smashing > + our display setup. The need for "avoid_warnings=2" seems like it will trip people up. I don't think it's in any config.txt I've seen. Can you expand more on that?
[PATCH 2/7] MAINTAINERS: Add myself for the new VC4 (RPi GPU) graphics driver.
On 08/12/2015 06:56 PM, Eric Anholt wrote: > diff --git a/MAINTAINERS b/MAINTAINERS > +DRM DRIVERS FOR VC4 > +M: Eric Anholt > +T: git git://github.com/anholt/linux > +S: Maintained > +F: drivers/gpu/drm/vc4/* S: Supported ?
[PATCH 1/7] drm/vc4: Add devicetree bindings for VC4.
On 08/12/2015 06:56 PM, Eric Anholt wrote:
> Signed-off-by: Eric Anholt
This one definitely needs a patch description, since someone might not
know what a VC4 is, and "git log" won't show the text from the binding
doc itself. I'd suggest adding the initial paragraph of the binding doc
as the patch description, or more.
> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt
> b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt
> +Required properties for VC4:
> +- compatible:Should be "brcm,vc4"
> +- crtcs: List of references to pixelvalve scanout engines
s/references to/phandles of/ would be more typical DT language.
> +- hvss: List of references to HVS video scalers
> +- encoders: List of references to output encoders (HDMI, SDTV)
Would it make sense to make all those nodes child node of the vc4
object. That way, there's no need to have these lists of objects; they
can be automatically built up as the DT is enumerated. I know that e.g.
the NVIDIA Tegra host1x binding works this way, and I think it may have
been inspired by other similar cases.
Of course, this is only appropriate if the HW modules really are
logically children of the VC4 HW module. Perhaps they aren't. If they
aren't though, I wonder what this "vc4" module actually represents in HW?
> +Required properties for HDMI
> +- compatible:Should be "brcm,vc4-hdmi"
> +- reg: Physical base address and length of the two register
> ranges
> + ("HDMI" and "HD")
I'd add "in that order" right before ")".
> +Example:
> +/ {
> + soc {
Minor nit: Examples often don't include any nodes "above" the nodes
whose bindings are being documented.
[PATCH 6/7] ARM: bcm2835: Add the DDC I2C controller to the device tree.
On 08/12/2015 06:56 PM, Eric Anholt wrote:
> We need to use it for getting video modes over HDMI.
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> + i2c2: i2c at 7e805000 {
> + compatible = "brcm,bcm2835-i2c";
> + reg = <0x7e805000 0x1000>;
> + interrupts = <2 21>;
> + clocks = <&clk_i2c>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
In an SoC .dtsi file, you'd typically write:
status = "disabled";
... in all nodes that represent IO controllers that interface to
external HW, so that board DT files can/must explicitly choose to enable
the device if it's actually in use on the board. Some systems might not
have HDMI and hence might not hook up the HDMI_SCL/SDA pads.
BCM2835-ARM-Peripherals.pdf states "Note that the BSC2 master is used
dedicated with the HDMI interface and should not be accessed by user
programs.". Does this imply the Linux kernel shouldn't be touching this
I2C controller; that the VC4 firmware might also be attempting to use
it? I wonder how any such sharing of the HW would work.
[PATCH 7/7] ARM: bcm2835: Add VC4 to the device tree.
On 08/12/2015 06:56 PM, Eric Anholt wrote:
> Signed-off-by: Eric Anholt
Patch description?
> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
> arm-pmu {
> compatible = "arm,arm1176-pmu";
> };
> +
> + hdmi: brcm,vc4-hdmi at 7e902000 {
It'd be nice to keep all the DT nodes with a reg property together, and
sorted in reg order.
As before, I think any DT node for a HW block that may-or-may-not-be
used based on board connectivity/features should be disabled in the SoC
.dtsi file, and enabled in the board's DT file if the feature is useful
for that board.
[PATCH v2 6/7] ARM: bcm2835: Add the DDC I2C controller to the device tree.
On 08/18/2015 03:54 PM, Eric Anholt wrote: > We need to use it for getting video modes over HDMI. This patch, Acked-by: Stephen Warren
[PATCH v2 7/7] ARM: bcm2835: Add VC4 to the device tree.
On 08/18/2015 03:54 PM, Eric Anholt wrote: > VC4 is the GPU (display and 3D) present on the 2835. This patch and patch 1 seem OK to me, although I'll withhold any ack until the DT binding design discussion with Rob has been resolved. I haven't looked at the OF graph bindings he mentioned so have no clue about his suggestions.
[5/5] ARM: tegra: jetson-tk1: enable GK20A GPU
On 09/25/2014 07:27 AM, Sjoerd Simons wrote:
> Playing a bit with todays linux-next on my jetson, it seems this patch is
> still required for enabling the GPU. Is there anything blocking it (firmware
> not available yet in liux-firmware?)
I think initially I was waiting for the DRM patch "drm/nouvea: support
for probing platform devices" to be applied, but it looks like that's
been applied already, so only patches 4 and 5 in this series are still
outstanding.
Alex, wasn't there also some issue where the VPR register had to be
programmed, and if it wasn't there'd be a hang when the GPU registers
were touched? If we've added code to Nouveau/tegradrm to detect that and
avoid the problem, then I guess we can commit these last two patches for
3.19. A resend after the 3.18 merge window might help.
> On Mon, May 19, 2014 at 06:24:10PM +0900, Alexandre Courbot wrote:
>> Signed-off-by: Alexandre Courbot
>> ---
>> arch/arm/boot/dts/tegra124-jetson-tk1.dts | 8 +++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>> b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>> index e31fb61a81d3..15a194d1277f 100644
>> --- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>> +++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
>> @@ -30,6 +30,12 @@
>> };
>> };
>>
>> +gpu at 0,5700 {
>> +status = "okay";
>> +
>> +vdd-supply = <&vdd_gpu>;
>> +};
>> +
>> pinmux: pinmux at 0,7868 {
>> pinctrl-names = "default";
>> pinctrl-0 = <&state_default>;
>> @@ -1505,7 +1511,7 @@
>> regulator-always-on;
>> };
>>
>> -sd6 {
>> +vdd_gpu: sd6 {
>> regulator-name = "+VDD_GPU_AP";
>> regulator-min-microvolt = <65>;
>> regulator-max-microvolt = <120>;
>
[5/5] ARM: tegra: jetson-tk1: enable GK20A GPU
On 09/25/2014 10:41 AM, Thierry Reding wrote: > On Thu, Sep 25, 2014 at 09:48:01AM -0600, Stephen Warren wrote: >> On 09/25/2014 07:27 AM, Sjoerd Simons wrote: >>> Playing a bit with todays linux-next on my jetson, it seems this patch is >>> still required for enabling the GPU. Is there anything blocking it (firmware >>> not available yet in liux-firmware?) >> >> I think initially I was waiting for the DRM patch "drm/nouvea: support for >> probing platform devices" to be applied, but it looks like that's been >> applied already, so only patches 4 and 5 in this series are still >> outstanding. >> >> Alex, wasn't there also some issue where the VPR register had to be >> programmed, and if it wasn't there'd be a hang when the GPU registers were >> touched? If we've added code to Nouveau/tegradrm to detect that and avoid >> the problem, then I guess we can commit these last two patches for 3.19. A >> resend after the 3.18 merge window might help. > > A patch that programs VPR was merged into U-Boot (though I don't think > it's made it into master yet). I'm not sure we can reasonably check for > that in Nouveau, given that the register is somewhere completely > unrelated. In fact I think the U-Boot patch was triggered by some > discussion about how to solve this and it was decided that it shouldn't > be done in the kernel, but U-Boot should set it up. > > That said, perhaps one solution would be to make U-Boot enable the gk20a > device if it's set up the VPR and disable it otherwise? For that to work, we'd need the DT to say status="disabled" by default for the GPU, and for the fixed U-Boot (and indeed every other bootloader...) to enable the GPU node. This would allow people with old versions of U-Boot (or other bootloaders) to continue to boot. This means bootloaders would only have to set status="okay", but never have to set status="disabled", which at least simplifies them a tiny bit.
[patch 2/2] gpu: host1x: returning success instead of -ENOMEM
On 08/23/2013 04:19 AM, Dan Carpenter wrote: > There is a mistake here so it returns PTR_ERR(NULL) which is success > instead of -ENOMEM. > > Signed-off-by: Dan Carpenter > --- > I can't compile this. For the record, just do: export CROSS_COMPILE=xxx make ARCH=arm tegra_defconfig make ARCH=arm zImage The only slightly difficult part is getting a CROSS_COMPILE value. Many distros ship at least some ARM cross-compiler in their standard package-set these days, and if not, you can download the Linaro compilers or go to ftp://ftp.kernel.org/pub/tools/crosstool/index.html.
[PATCH 00/31] ARM: tegra: use common reset and DMA bindings
On 11/15/2013 01:53 PM, Stephen Warren wrote: > From: Stephen Warren > > This series implements a common reset framework driver for Tegra, and > updates all relevant Tegra drivers to use it. It also removes the custom > DMA bindings and replaced them with the standard DMA DT bindings. > > Historically, the Tegra clock driver has exported a custom API for module > reset. This series removes that API, and transitions DT and drivers to > the new reset framework. > > The custom API used a "struct clk" to identify which module to reset, and > consequently some DT bindings and drivers required clocks to be provided > where they really needed just a reset identifier instead. Due to this > known deficiency, I have always considered most Tegra bindings to be > unstable. This series removes this excuse for instability, although I > still consider some Tegra bindings unstable due to the need to convert to > the common DMA bindings. > > Historically, Tegra DMA channels have been represented in DT using a > custom nvidia,dma-request-selector property. Now that standard DMA DT > bindings exist, convert all Tegra bindings, DTs, and drivers to use the > standard instead. > > This series makes a DT-ABI-incompatible change to: > - Require reset specifiers in DT where relevant. > - Require standard DMA specifiers. > - Remove clock specifiers from DT where they were only needed for reset. > - Remove legacy DMA specifier properties. > > I anticipate merging this whole series into the Tegra and arm-soc trees > as its own branch, due to internal dependencies. This branch will be > stable and can then be merged into any other subsystem trees should any > conflicts arise. > > This series depends on Peter's Tegra clock driver rework, available at > git://nv-tegra.nvidia.com/user/pdeschrijver/linux tegra-clk-tegra124-0 > (or whatever version of that gets included in 3.14) I've applied this series (and pulled in the DMA/ASoC/clk dependencies required) to Tegra's for-3.14/dmas-resets-rework branch.
[PATCH] drm/tegra: fix compile w/ CONFIG_DYNAMIC_DEBUG
From: Stephen Warren
With CONFIG_DYNAMIC_DEBUG=y, the followin compile error occurs:
drivers/gpu/drm/tegra/mipi-phy.c: In function ?mipi_dphy_timing_validate?:
drivers/gpu/drm/tegra/mipi-phy.c:69:11: error: ?EINVAL? undeclared (first use
in this function)
drivers/gpu/drm/tegra/mipi-phy.c:69:11: note: each undeclared identifier is
reported only once for each function it appears in
Fix this by directly including the header that defines EINVAL.
Fixes: 39aa0a4f3be5 ("drm/tegra: Add DSI support")
Signed-off-by: Stephen Warren
---
drivers/gpu/drm/tegra/mipi-phy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/tegra/mipi-phy.c b/drivers/gpu/drm/tegra/mipi-phy.c
index 1d2ad267cfce..e2c4aedaee78 100644
--- a/drivers/gpu/drm/tegra/mipi-phy.c
+++ b/drivers/gpu/drm/tegra/mipi-phy.c
@@ -20,6 +20,7 @@
* OF THIS SOFTWARE.
*/
+#include
#include
#include "mipi-phy.h"
--
1.8.1.5
[PATCH 3/3] ARM: bcm2835: Add VC4 to the device tree.
On 03/04/2016 01:32 PM, Eric Anholt wrote:
> VC4 is the GPU (display and 3D) present on the 283x.
> diff --git a/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
> b/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
> +&hdmi {
> + hpd-gpios = <&gpio 46 GPIO_ACTIVE_LOW>;
> +};
Isn't that the same everywhere? If so, adding it to bcm2835-rpi.dtsi
seems like a better idea; it'd avoid duplicating it everywhere.
[PATCH 3/3] ARM: bcm2835: Add VC4 to the device tree.
On 03/08/2016 11:04 AM, Eric Anholt wrote:
> Stephen Warren writes:
>
>> On 03/04/2016 01:32 PM, Eric Anholt wrote:
>>> VC4 is the GPU (display and 3D) present on the 283x.
>>
>>> diff --git a/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
>>> b/arch/arm/boot/dts/bcm2835-rpi-a-plus.dts
>>
>>> +&hdmi {
>>> + hpd-gpios = <&gpio 46 GPIO_ACTIVE_LOW>;
>>> +};
>>
>> Isn't that the same everywhere? If so, adding it to bcm2835-rpi.dtsi
>> seems like a better idea; it'd avoid duplicating it everywhere.
>
> It's not the same everywhere (_HIGH vs _LOW), which is why it's in the
> individual files.
Oh right, it looks OK then. The series,
Acked-by: Stephen Warren
One could reduce the duplication by moving the common block into the
common .dtsi file, but using a board-defined #define for the polarity:
bcm2835-rpi-a-plus.dts:
#define RPI_HDMI_HPD_POLARITY
#include "bcm2835-rpi.dtsi"
bcm2835-rpi.dtsi:
&hdmi {
hpd-gpios = <&gpio 46 RPI_HDMI_HPD_POLARITY>;
};
... although this case is so tiny I'm not sure there's any benefit
trying to unify it like that right now.
[PATCH] drm: tegra: Use framebuffer pitch as line stride
On 11/22/2012 12:37 PM, Thierry Reding wrote: > Instead of using the stride derived from the display mode, use the pitch > associated with the currently active framebuffer. This fixes a bug where > the LCD display content would be skewed when enabling HDMI with a video > mode different from that of the LCD. This patch certainly doesn't cause any additional issues for me, so: Tested-by: Stephen Warren Howwever, it still doesn't allow both Cardhu's LCD panel and external HDMI port (1080p) to be active at once. If I boot with both enabled, or boot with just the LCD enabled and hot-plug HDMI, as soon as both heads are active, then some kind of display corruption starts; it looks like a clocking issue or perhaps memory underflow. Mark, can you please investigate this. I haven't tested to see if the issue is Tegra30-specific, or also happens on Tegra20.
[RFC v2 5/8] ARM: tegra: Add auxiliary data for nvhost
On 11/26/2012 06:19 AM, Terje Bergstrom wrote:
> Add SoC specific auxiliary data to host1x and gr2d. nvhost uses
> this data.
>
> Signed-off-by: Terje Bergstrom
> Signed-off-by: Arto Merilainen
Arto's S-o-b really should be first and yours last since you're (Terje)
the one who touched the patches last.
> diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c
> b/arch/arm/mach-tegra/board-dt-tegra20.c
I think none of the changes the board-dt-tegra*.c should be made.
AUXDATA is a temporary measure to keep things working during the
transition to device tree. We want to remove entries from the AUXDATA
tables rather than add them. The only thing that's stopping us from
doing so right now is the lack of DT-based clock lookups, which hence
require devices to have a specific name.
> +static struct nvhost_device_data tegra_host1x_info = {
> + .clocks = { {"host1x", UINT_MAX} },
> +static struct nvhost_device_data tegra_gr2d_info = {
> + .clocks = { {"gr2d", UINT_MAX, true}, {"epp", UINT_MAX, true} },
Clock names shouldn't be passed in platform data; instead, clk_get()
should be passed the device object and device-relative (i.e. not global)
clock name. I expect if the driver is fixed to make this change, the
changes to tegra*_clocks_data.c won't be needed either.
[RFC v2 5/8] ARM: tegra: Add auxiliary data for nvhost
On 11/26/2012 11:33 PM, Terje Bergstr?m wrote:
> On 27.11.2012 01:39, Stephen Warren wrote:
>> Clock names shouldn't be passed in platform data; instead, clk_get()
>> should be passed the device object and device-relative (i.e. not global)
>> clock name. I expect if the driver is fixed to make this change, the
>> changes to tegra*_clocks_data.c won't be needed either.
>
> Isn't this code doing exactly that - getting a device relative clock,
> nvhost_module_init() in nvhost.acm.c:
>
> (...)
> /* initialize clocks to known state */
> while (pdata->clocks[i].name && i < NVHOST_MODULE_MAX_CLOCKS) {
> long rate = pdata->clocks[i].default_rate;
> struct clk *c;
>
> c = devm_clk_get(&dev->dev, pdata->clocks[i].name);
The line above is getting the (device-relative) clock name from platform
data, rather than using some fixed name as it should be.
[PATCH] drm: tegra: Use framebuffer pitch as line stride
On 11/26/2012 08:16 PM, Mark Zhang wrote: > On 11/27/2012 06:37 AM, Stephen Warren wrote: >> On 11/22/2012 12:37 PM, Thierry Reding wrote: >>> Instead of using the stride derived from the display mode, use the pitch >>> associated with the currently active framebuffer. This fixes a bug where >>> the LCD display content would be skewed when enabling HDMI with a video >>> mode different from that of the LCD. >> >> This patch certainly doesn't cause any additional issues for me, so: >> >> Tested-by: Stephen Warren >> >> Howwever, it still doesn't allow both Cardhu's LCD panel and external >> HDMI port (1080p) to be active at once. If I boot with both enabled, or >> boot with just the LCD enabled and hot-plug HDMI, as soon as both heads >> are active, then some kind of display corruption starts; it looks like a >> clocking issue or perhaps memory underflow. > > I haven't observed this issue. What kind of display corruption you mean? > Did it recover after some seconds or the display in LVDS panel was > always corrupted? > > During my testing, I connected HDMI while booting cardhu and I can see > the LVDS and HDMI working with no corruptions. For your viewing pleasure (and playing with my new phone) :-) http://www.youtube.com/watch?v=ZJxJnONz7DA The external monitor is 1920x1200 I believe.
[PATCH] drm: tegra: Use framebuffer pitch as line stride
On 11/27/2012 11:17 AM, Stephen Warren wrote: > On 11/26/2012 08:16 PM, Mark Zhang wrote: >> On 11/27/2012 06:37 AM, Stephen Warren wrote: >>> On 11/22/2012 12:37 PM, Thierry Reding wrote: >>>> Instead of using the stride derived from the display mode, use the pitch >>>> associated with the currently active framebuffer. This fixes a bug where >>>> the LCD display content would be skewed when enabling HDMI with a video >>>> mode different from that of the LCD. >>> >>> This patch certainly doesn't cause any additional issues for me, so: >>> >>> Tested-by: Stephen Warren >>> >>> Howwever, it still doesn't allow both Cardhu's LCD panel and external >>> HDMI port (1080p) to be active at once. If I boot with both enabled, or >>> boot with just the LCD enabled and hot-plug HDMI, as soon as both heads >>> are active, then some kind of display corruption starts; it looks like a >>> clocking issue or perhaps memory underflow. >> >> I haven't observed this issue. What kind of display corruption you mean? >> Did it recover after some seconds or the display in LVDS panel was >> always corrupted? >> >> During my testing, I connected HDMI while booting cardhu and I can see >> the LVDS and HDMI working with no corruptions. > > For your viewing pleasure (and playing with my new phone) :-) > http://www.youtube.com/watch?v=ZJxJnONz7DA > > The external monitor is 1920x1200 I believe. Jon Mayo says the corruption in the video is display (memory fetch) underflow. Perhaps this is because (IIRC) the BCT I'm using on Cardhu programs the memory controller at a slow rate, and the bootloader and/or kernel is supposed to bump up the rate to the max, but that's not implemented anywhere yet upstream. If you're testing with "fastboot" instead of U-Boot, that might be re-programming the memory frequencies, and hence avoiding this. I guess we have a fun time ahead of us with mode validation and memory controller programming.
[RFC v2 8/8] drm: tegra: Add gr2d device
On 11/28/2012 07:45 AM, Terje Bergstr?m wrote: > On 28.11.2012 16:06, Lucas Stach wrote: >> Why do even need/use dma-buf for this use case? This is all one DRM >> device, even if we separate host1x and gr2d as implementation modules. > > I didn't want to implement dependency to drm gem objects in nvhost, but > we have thought about doing that. dma-buf brings quite a lot of > overhead, so implementing support for gem buffers would make the > sequence a bit leaner. > > nvhost already has infra to support multiple memory managers. > >> So standard way of doing this is: >> 1. create gem object for pushbuffer >> 2. create fake mmap offset for gem obj >> 3. map pushbuf using the fake offset on the drm device >> 4. at submit time zap the mapping >> >> You need this logic anyway, as normally we don't rely on userspace to >> sync gpu and cpu, but use the kernel to handle the concurrency issues. > > Taking a step back - 2D streams are actually very short, in the order of > <100 bytes. Just copying them to kernel space would actually be faster > than doing MMU operations. I'm not sure it's a good idea to have one buffer submission mechanism for the 2D class and another for the 3D/... class, nor to bet that 2D streams will always be short.
[PATCH v2] drm: tegra: Add maintainers entry
On 11/28/2012 12:18 PM, Thierry Reding wrote: > Add myself as the maintainer of the NVIDIA Tegra DRM driver. Aside from one comment below, Acked-by: Stephen Warren > +L: dri-devel at lists.freedesktop.org Should linux-tegra at vger.kernel.org also be CC'd so that everything Tegra-related goes to one place?
[RFC v2 1/8] video: tegra: Add nvhost driver
On 11/29/2012 03:21 AM, Terje Bergstr?m wrote:
> On 28.11.2012 23:23, Thierry Reding wrote:
...
>>> + regs = platform_get_resource(dev, IORESOURCE_MEM, 0);
>>> + intr0 = platform_get_resource(dev, IORESOURCE_IRQ, 0);
>>> + intr1 = platform_get_resource(dev, IORESOURCE_IRQ, 1);
>>> +
>>> + if (!regs || !intr0 || !intr1) {
>>
>> I prefer to have these checked for explicitly, one by one for
>> readability and potentially more useful diagnostics.
>
> Can do.
>
>> Also you should be using platform_get_irq() for interrupts. Furthermore
>> the host1x DT node (and the TRM) name the interrupts "syncpt" and
>> "general", so maybe those would be more useful variable names than
>> "intr0" and "intr1".
>>
>> But since you don't use them anyway they shouldn't be part of this
>> patch.
>
> True. I might also as well delete the general interrupt altogether, as
> we don't use it for any real purpose.
Do make sure the interrupts still are part of the DT binding though, so
that the binding fully describes the HW, and the interrupt is available
to retrieve if we ever do use it in the future.
>>> + for (i = 0; i < pdata->num_clks; i++)
>>> + clk_prepare_enable(pdata->clk[i]);
>>> + nvhost_syncpt_reset(&host->syncpt);
>>> + for (i = 0; i < pdata->num_clks; i++)
>>> + clk_disable_unprepare(pdata->clk[i]);
>>
>> Stephen already hinted at this when discussing the AUXDATA. You should
>> explicitly request the clocks.
>
> I'm not too happy about that idea. The clock management code is generic
> for all modules, and that's why it's driven by a data structure. Now
> Stephen and you ask me to unroll the loops and make copies of the code
> to facilitate different modules and different SoCs.
You can still create tables of clocks inside the driver and loop over
them. So, loop unrolling isn't related to my comments at least. It's
just that clk_get() shouldn't take its parameters from platform data.
But if these are clocks for (arbitrary) child modules (that may or may
not exist dynamically), why aren't the drivers for the child modules
managing them?
[RFC v2 1/8] video: tegra: Add nvhost driver
On 11/29/2012 04:47 AM, Thierry Reding wrote: > On Thu, Nov 29, 2012 at 12:21:04PM +0200, Terje Bergstr?m wrote: >> On 28.11.2012 23:23, Thierry Reding wrote: >>> This could be problematic. Since drivers/video and >>> drivers/gpu/drm are separate trees, this would entail a >>> continuous burden on keeping both trees synchronized. While I >>> realize that eventually it might be better to put the host1x >>> driver in a separate place to accomodate for its use by other >>> subsystems, I'm not sure moving it here right away is the best >>> approach. >> >> I understand your point, but I hope also that we'd end up with >> something that can be used as basis for the downstream kernel to >> migrate to upstream stack. >> >> The key point here is to make the API between nvhost and tegradrm >> as small and robust to changes as possible. > > I agree. But I also fear that there will be changes eventually and > having both go in via different tree requires those trees to be > merged in a specific order to avoid breakage should the API change. > This will be particularly ugly in linux-next. > > That's why I explicitly proposed to take this into > drivers/gpu/drm/tegra for the time being, until we can be > reasonably sure that the API is fixed. Then I'm fine with moving it > wherever seems the best fit. Even then there might be the > occasional dependency, but they should get fewer and fewer as the > code matures. It is acceptable for one maintainer to ack patches, and another maintainer to merge a series that touches both "their own" code and code owned by another tree. This should of course only be needed when inter-module APIs change; changes to code within a module shouldn't require this.
[RFC v2 2/8] video: tegra: Add syncpoint wait and interrupts
On 11/29/2012 01:44 AM, Thierry Reding wrote:
> On Mon, Nov 26, 2012 at 03:19:08PM +0200, Terje Bergstrom wrote:
>> diff --git a/drivers/video/tegra/host/host1x/host1x_intr.c
>> b/drivers/video/tegra/host/host1x/host1x_intr.c
> [...]
>> +/* Spacing between sync registers */ +#define REGISTER_STRIDE 4
>
> Erm... no. The usual way you should be doing this is either make
> the register definitions account for the stride or use accessors
> that apply the stride. You should be doing the latter anyway to
> make accesses. For example:
>
> static inline void host1x_syncpt_writel(struct host1x *host1x,
> unsigned long value, unsigned long offset) { writel(value,
> host1x->regs + SYNCPT_BASE + offset); }
>
> static inline unsigned long host1x_syncpt_readl(struct host1x
> *host1x, unsigned long offset) { return readl(host1x->regs +
> SYNCPT_BASE + offset); }
>
> Alternatively, if you want to pass the register index instead of
> the offset, you can use just multiply the offset in that function:
>
> writel(value, host1x->regs + SYNCPT_BASE + (offset << 2));
>
> The same can also be done with the non-syncpt registers.
It seems like reasonable documentation to replace "<< 2" with "*
REGISTER_STRIDE" here.
[PATCH V6 2/2] video: drm: exynos: Add device tree support
On 09/30/2012 11:29 PM, Leela Krishna Amudala wrote: > Hello Stephen Warren, > > The binding names that I use in my dts file should match with the > names given in > http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html > right? I don't think so; the binding in that link is for example: > + - xres, yres: Display resolution > + - left-margin, right-margin, hsync-len: Horizontal Display timing parameters > + in pixels > + upper-margin, lower-margin, vsync-len: Vertical display timing parameters > in > + lines > + - clock: displayclock in Hz i.e. a bunch of separate properties, one for each value needed to describe the display timing. However, your patch contains: >>> + - samsung,fimd-display: This property should specify the phandle of the >>> + display device node which holds the video interface timing with the >>> + below mentioned properties. >>> + >>> + - lcd-htiming: Specifies the horizontal timing for the overlay. The >>> + horizontal timing includes four parameters in the following order. >>> + >>> + - horizontal back porch (in number of lcd clocks) >>> + - horizontal front porch (in number of lcd clocks) >>> + - hsync pulse width (in number of lcd clocks) >>> + - Display panels X resolution. A single lcd-htiming property, which contains 4 values. (and a similar construct for the vertical timing). That seems entirely different to me...
[PATCH 1/2] of: add helper to parse display specs
On 09/24/2012 09:35 AM, Steffen Trumtrar wrote:
> Parse a display-node with timings and hardware-specs from devictree.
> diff --git a/Documentation/devicetree/bindings/video/display
> b/Documentation/devicetree/bindings/video/display
> new file mode 100644
> index 000..722766a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display
This should be display.txt.
> @@ -0,0 +1,208 @@
> +display bindings
> +==
> +
> +display-node
> +
I'm not personally convinced about the direction this is going. While I
think it's reasonable to define DT bindings for displays, and DT
bindings for display modes, I'm not sure that it's reasonable to couple
them together into a single binding.
I think creating a well-defined timing binding first will be much
simpler than doing so within the context of a display binding; the
scope/content of a general display binding seems much less well-defined
to me at least, for reasons I mentioned before.
> +required properties:
> + - none
> +
> +optional properties:
> + - default-timing: the default timing value
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
At least those two properties should exist in the display timing instead
(or perhaps as well). There are certainly cases where different similar
display modes are differentiated by hsync/vsync polarity more than
anything else. This is probably more likely with analog display
connectors than digital, but I see no reason why a DT binding for
display timing shouldn't cover both.
> + - de-active-high (bool): Data-Enable pulse is active high
> + - pixelclk-inverted (bool): pixelclock is inverted
> + - pixel-per-clk
pixel-per-clk is probably something that should either be part of the
timing definition, or something computed internally to the display
driver based on rules for the signal type, rather than something
represented in DT.
The above comment assumes this property is intended to represent DVI's
requirement for pixel clock doubling for low-pixel-clock-rate modes. If
it's something to do with e.g. a single-data-rate vs. double-data-rate
property of the underlying physical connection, that's most likely
something that should be defined in a binding specific to e.g. LVDS,
rather than something generic.
> + - link-width: number of channels (e.g. LVDS)
> + - bpp: bits-per-pixel
> +
> +timings-subnode
> +---
> +
> +required properties:
> +subnodes that specify
> + - hactive, vactive: Display resolution
> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> + in pixels
> + vfront-porch, vback-porch, vsync-len: Vertical display timing parameters
> in
> + lines
> + - clock: displayclock in Hz
> +
> +There are different ways of describing a display and its capabilities. The
> devicetree
> +representation corresponds to the one commonly found in datasheets for
> displays.
> +The description of the display and its timing is split in two parts: first
> the display
> +properties like size in mm and (optionally) multiple subnodes with the
> supported timings.
> +If a display supports multiple signal timings, the default-timing can be
> specified.
> +
> +Example:
> +
> + display at 0 {
> + width-mm = <800>;
> + height-mm = <480>;
> + default-timing = <&timing0>;
> + timings {
> + timing0: timing at 0 {
If you're going to use a unit address ("@0") to ensure that node names
are unique (which is not mandatory), then each node also needs a reg
property with matching value, and #address-cells/#size-cells in the
parent. Instead, you could name the nodes something unique based on the
mode name to avoid this, e.g. 1080p24 { ... }.
[PATCH 1/2] of: add helper to parse display specs
On 10/01/2012 01:16 PM, Mitch Bradley wrote:
> On 10/1/2012 6:53 AM, Stephen Warren wrote:
>> On 09/24/2012 09:35 AM, Steffen Trumtrar wrote:
>>> Parse a display-node with timings and hardware-specs from devictree.
>>
>>> diff --git a/Documentation/devicetree/bindings/video/display
>>> b/Documentation/devicetree/bindings/video/display
>>> new file mode 100644
>>> index 000..722766a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/video/display
>>
>> This should be display.txt.
>>
>>> @@ -0,0 +1,208 @@
>>> +display bindings
>>> +==
>>> +
>>> +display-node
>>> +
>>
>> I'm not personally convinced about the direction this is going. While I
>> think it's reasonable to define DT bindings for displays, and DT
>> bindings for display modes, I'm not sure that it's reasonable to couple
>> them together into a single binding.
>>
>> I think creating a well-defined timing binding first will be much
>> simpler than doing so within the context of a display binding; the
>> scope/content of a general display binding seems much less well-defined
>> to me at least, for reasons I mentioned before.
>>
>>> +required properties:
>>> + - none
>>> +
>>> +optional properties:
>>> + - default-timing: the default timing value
>>> + - width-mm, height-mm: Display dimensions in mm
>>
>>> + - hsync-active-high (bool): Hsync pulse is active high
>>> + - vsync-active-high (bool): Vsync pulse is active high
>>
>> At least those two properties should exist in the display timing instead
>> (or perhaps as well). There are certainly cases where different similar
>> display modes are differentiated by hsync/vsync polarity more than
>> anything else. This is probably more likely with analog display
>> connectors than digital, but I see no reason why a DT binding for
>> display timing shouldn't cover both.
>>
>>> + - de-active-high (bool): Data-Enable pulse is active high
>>> + - pixelclk-inverted (bool): pixelclock is inverted
>>
>>> + - pixel-per-clk
>>
>> pixel-per-clk is probably something that should either be part of the
>> timing definition, or something computed internally to the display
>> driver based on rules for the signal type, rather than something
>> represented in DT.
>>
>> The above comment assumes this property is intended to represent DVI's
>> requirement for pixel clock doubling for low-pixel-clock-rate modes. If
>> it's something to do with e.g. a single-data-rate vs. double-data-rate
>> property of the underlying physical connection, that's most likely
>> something that should be defined in a binding specific to e.g. LVDS,
>> rather than something generic.
>>
>>> + - link-width: number of channels (e.g. LVDS)
>>> + - bpp: bits-per-pixel
>>> +
>>> +timings-subnode
>>> +---
>>> +
>>> +required properties:
>>> +subnodes that specify
>>> + - hactive, vactive: Display resolution
>>> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing
>>> parameters
>>> + in pixels
>>> + vfront-porch, vback-porch, vsync-len: Vertical display timing
>>> parameters in
>>> + lines
>>> + - clock: displayclock in Hz
>>> +
>>> +There are different ways of describing a display and its capabilities. The
>>> devicetree
>>> +representation corresponds to the one commonly found in datasheets for
>>> displays.
>>> +The description of the display and its timing is split in two parts: first
>>> the display
>>> +properties like size in mm and (optionally) multiple subnodes with the
>>> supported timings.
>>> +If a display supports multiple signal timings, the default-timing can be
>>> specified.
>>> +
>>> +Example:
>>> +
>>> + display at 0 {
>>> + width-mm = <800>;
>>> + height-mm = <480>;
>>> + default-timing = <&timing0>;
>>> + timings {
>>> + timing0: timing at 0 {
>>
>> If you're going to use a unit address ("@0") to ensure that node names
>> are unique (which is not mandatory), then each node also needs a reg
>> property with matching value, and #address-cells/#size-cells in the
>> parent. Instead, you could name the nodes something unique ba
[PATCH V6 2/2] video: drm: exynos: Add device tree support
On 10/02/2012 10:06 PM, Leela Krishna Amudala wrote: > On Mon, Oct 1, 2012 at 9:50 PM, Stephen Warren > wrote: >> On 09/30/2012 11:29 PM, Leela Krishna Amudala wrote: >>> Hello Stephen Warren, >>> >>> The binding names that I use in my dts file should match with the >>> names given in >>> http://lists.freedesktop.org/archives/dri-devel/2012-July/024875.html >>> right? >> >> I don't think so; the binding in that link is for example: >> >>> + - xres, yres: Display resolution >>> + - left-margin, right-margin, hsync-len: Horizontal Display timing >>> parameters >>> + in pixels >>> + upper-margin, lower-margin, vsync-len: Vertical display timing >>> parameters in >>> + lines >>> + - clock: displayclock in Hz >> >> i.e. a bunch of separate properties, one for each value needed to >> describe the display timing. However, your patch contains: > > I mean to say that even I have to use separate properties for each one > instead of grouping them. > Also the names should match with the ones given in the example..? Yes. The patch I pointed to isn't supposed to be just an example, but /the/ standard way of representing display timings.
[PATCH 1/2 v6] of: add helper to parse display timings
On 10/04/2012 11:59 AM, Steffen Trumtrar wrote: A patch description would be useful for something like this. > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt > b/Documentation/devicetree/bindings/video/display-timings.txt > new file mode 100644 ... > +Usage in backend > + Everything before that point in the file looks fine to me. Everything after this point in the file seems to be Linux-specific implementation details. Does it really belong in the DT binding documentation, rather than some Linux-specific documentation file? > +struct drm_display_mode > +=== > + > + > +--+-+--+---+ > + | | | | > | ? > + | | | | > | | > + | | | | > | | > + > +--###--+---+ > | I suspect the entire horizontal box above (and the entire vertical box all the way down the left-hand side) should be on the bottom/right instead of top/left. The reason I think this is because all of vsync_start, vsync_end, vdisplay have to be referenced to some known point, which is usually zero or the start of the timing definition, /or/ there would be some value indicating the size of the top marging/porch in order to say where those other values are referenced to. > + | # ? ? ?# | > | | > + | # | | |# | > | | > + | # | | |# | > | | > + | # | | |# | > | | > + | # | | |# | > | | > + | # | | | hdisplay # | > | | > + | #<--++---># | > | | > + | # | | |# | > | | > + | # |vsync_start |# | > | | > + | # | | |# | > | | > + | # | |vsync_end |# | > | | > + | # | | |vdisplay# | > | | > + | # | | |# | > | | > + | # | | |# | > | | > + | # | | |# | > | | vtotal > + | # | | |# | > | | > + | # | | | hsync_start# | > | | > + | #<--+-+--+-->| > | | > + | # | | |# | > | | > + | # | | | hsync_end # | > | | > + | > #<--+-+--+-->| | > + | # | | ?# | > | | > + > +--|#|--+---+ > | > + | | | | | | > | | > + | | | | | | > | | > + | | ? | | | > | | > + > +--+-+---+--+---+ > | > + | | | | | > | | > + | | | | | > | | > + | | ? | | > | ? > + > +--+-+--+---+ > + htotal > + > <-> > diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c > +static int parse_property(struct device_node *np, char *name, > + struct timing_entry *result) > + if (cells == 1) > + ret = of_property_read_u32_array(np, name, &result->typ, cells); Should that branch not just set result->min/max to typ as well? Presumably it'd prevent any code that interprets struct timing_entry from having to check if those values were 0 or not? > + else if (cells == 3) > +
[PATCH 2/2 v6] of: add generic videomode description
On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> Get videomode from devicetree in a format appropriate for the
> backend. drm_display_mode and fb_videomode are supported atm.
> Uses the display signal timings from of_display_timings
> +++ b/drivers/of/of_videomode.c
> +int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
> + st = display_timings_get(disp, index);
> +
> + if (!st) {
It's a little odd to leave a blank line between those two lines.
Only half of the code in this file seems OF-related; the routines to
convert a timing to a videomode or drm display mode seem like they'd be
useful outside device tree, so I wonder if putting them into
of_videomode.c is the correct thing to do. Still, it's probably not a
big deal.
[PATCH 1/2 v6] of: add helper to parse display timings
On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote: > Hi Steffen > > Sorry for chiming in so late in the game, but I've long been wanting to > have a look at this and compare with what we do for V4L2, so, this seems a > great opportunity to me:-) > > On Thu, 4 Oct 2012, Steffen Trumtrar wrote: >> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt >> b/Documentation/devicetree/bindings/video/display-timings.txt >> +timings-subnode >> +--- >> + >> +required properties: >> + - hactive, vactive: Display resolution >> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing >> parameters >> + in pixels >> + vfront-porch, vback-porch, vsync-len: Vertical display timing parameters >> in >> + lines >> + - clock: displayclock in Hz > > You're going to hate me for this, but eventually we want to actually > reference clock objects in our DT bindings. For now, even if you don't > want to actually add clock phandles and stuff here, I think, using the > standard "clock-frequency" property would be much better! In a definition of a display timing, we will never need to use the clock binding; the clock binding would be used by the HW module that is generating a timing, not by the timing definition itself. That said, your comment about renaming the property to avoid any kind of conceptual conflict is still quite valid. This is bike-shedding, but "pixel-clock" might be more in line with typical video mode terminology, although there's certainly preference in DT for using the generic term clock-frequency that you proposed. Either is fine by me. >> +optional properties: >> + - hsync-active-high (bool): Hsync pulse is active high >> + - vsync-active-high (bool): Vsync pulse is active high > > For the above two we also considered using bool properties but eventually > settled down with integer ones: > > - hsync-active = <1> > > for active-high and 0 for active low. This has the added advantage of > being able to omit this property in the .dts, which then doesn't mean, > that the polarity is active low, but rather, that the hsync line is not > used on this hardware. So, maybe it would be good to use the same binding > here too? I agree. This also covers the case where analog display connectors often use polarity to differentiate similar modes, yet digital connectors often always use a fixed polarity since the receiving device can "measure" the signal in more complete ways. If the board HW inverts these lines, the same property names can exist in the display controller itself, and the two values XORd together to yield the final output polarity. >> + - de-active-high (bool): Data-Enable pulse is active high >> + - pixelclk-inverted (bool): pixelclock is inverted > > We don't (yet) have a de-active property in V4L, don't know whether we'll > ever have to distingsuish between what some datasheets call "HREF" and > HSYNC in DT, but maybe similarly to the above an integer would be > preferred. As for pixclk, we call the property "pclk-sample" and it's also > an integer. Thinking about this more: de-active-high is likely to be a board-specific property and hence something in the display controller, not in the mode definition? >> + - interlaced (bool) > > Is "interlaced" a property of the hardware, i.e. of the board? Can the > same display controller on one board require interlaced data and on > another board - progressive? Interlace is a property of a display mode. It's quite possible for a particular display controller to switch between interlace and progressive output at run-time. For example, reconfiguring the output between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to a built-in LCD display, you're probably always going to be driving the single mode required by the panel, and that mode will likely always be progressive. However, since this binding attempts to describe any display timing, I think we still need this property per mode. > BTW, I'm not very familiar with display > interfaces, but for interlaced you probably sometimes use a field signal, > whose polarity you also want to specify here? We use a "field-even-active" > integer property for it. I think that's a property of the display controller itself, rather than an individual mode, although I'm not 100% certain. My assertion is that the physical interface that the display controller is driving will determine whether embedded or separate sync is used, and in the separate sync case, how the field signal is defined, and that all interlace modes driven over that interface will use the same field signal definition.
[PATCH 1/2 v6] of: add helper to parse display timings
On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:
> On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
>> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
...
>>> + for_each_child_of_node(timings_np, entry) {
>>> + struct signal_timing *st;
>>> +
>>> + st = of_get_display_timing(entry);
>>> +
>>> + if (!st)
>>> + continue;
>>
>> I wonder if that shouldn't be an error?
>
> In the sense of a pr_err not a -EINVAL I presume?! It is a little bit quiet in
> case of a faulty spec, that is right.
I did mean return an error; if we try to parse something and can't,
shouldn't we return an error?
I suppose it may be possible to limp on and use whatever subset of modes
could be parsed and drop the others, which is what this code does, but
the code after the loop would definitely return an error if zero timings
were parseable.
[PATCH 1/2 v6] of: add helper to parse display timings
On 10/08/2012 02:25 AM, Guennadi Liakhovetski wrote: > On Fri, 5 Oct 2012, Stephen Warren wrote: > >> On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote: >>> Hi Steffen >>> >>> Sorry for chiming in so late in the game, but I've long been wanting to >>> have a look at this and compare with what we do for V4L2, so, this seems a >>> great opportunity to me:-) >>> >>> On Thu, 4 Oct 2012, Steffen Trumtrar wrote: >> >>>> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt >>>> b/Documentation/devicetree/bindings/video/display-timings.txt >> >>>> +timings-subnode >>>> +--- >>>> + >>>> +required properties: >>>> + - hactive, vactive: Display resolution >>>> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing >>>> parameters >>>> + in pixels >>>> + vfront-porch, vback-porch, vsync-len: Vertical display timing >>>> parameters in >>>> + lines >>>> + - clock: displayclock in Hz >>> >>> You're going to hate me for this, but eventually we want to actually >>> reference clock objects in our DT bindings. For now, even if you don't >>> want to actually add clock phandles and stuff here, I think, using the >>> standard "clock-frequency" property would be much better! >> >> In a definition of a display timing, we will never need to use the clock >> binding; the clock binding would be used by the HW module that is >> generating a timing, not by the timing definition itself. > > You mean clock consumer bindings will be in the display device DT node? > And the display-timings node will be its child? Yes ... >>>> + - interlaced (bool) >>> >>> Is "interlaced" a property of the hardware, i.e. of the board? Can the >>> same display controller on one board require interlaced data and on >>> another board - progressive? >> >> Interlace is a property of a display mode. It's quite possible for a >> particular display controller to switch between interlace and >> progressive output at run-time. For example, reconfiguring the output >> between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to >> a built-in LCD display, you're probably always going to be driving the >> single mode required by the panel, and that mode will likely always be >> progressive. However, since this binding attempts to describe any >> display timing, I think we still need this property per mode. > > But why do you need this in the DT then at all? Because the driver for the display controller has no idea what display or panel will be connected to it. > If it's fixed, as required > per display controller, then its driver will know it. If it's runtime > configurable, then it's a purely software parameter and doesn't depend on > the board? interlace-vs-progressive isn't "fixed, as required per display controller", but is a property of the mode being sent by the display controller, and the requirements for that mode are driven by the panel/display connected to the display controller, not the display controller, in general. ... >>> BTW, I'm not very familiar with display >>> interfaces, but for interlaced you probably sometimes use a field signal, >>> whose polarity you also want to specify here? We use a "field-even-active" >>> integer property for it. >> >> I think that's a property of the display controller itself, rather than >> an individual mode, although I'm not 100% certain. My assertion is that >> the physical interface that the display controller is driving will >> determine whether embedded or separate sync is used, and in the separate >> sync case, how the field signal is defined, and that all interlace modes >> driven over that interface will use the same field signal definition. > > In general, I might be misunderstanding something, but don't we have to > distinguish between 2 types of information about display timings: (1) is > defined by the display controller requirements, is known to the display > driver and doesn't need to be present in timings DT. We did have some of > these parameters in board data previously, because we didn't have proper > display controller drivers... Yes, there probably is data of that kind, but the display mode timings binding is only address standardized display timings information, not controller-specific information, and hence doesn't cover this case. > (2) is board specific configuration, and is > such it has to be present in DT. Certainly, yes. > In that way, doesn't "interlaced" belong to type (1) and thus doesn't need > to be present in DT? I don't believe so.
[PATCH 1/2 v6] of: add helper to parse display timings
On 10/08/2012 06:20 AM, Tomi Valkeinen wrote: > On Mon, 2012-10-08 at 14:04 +0200, Laurent Pinchart wrote: >> On Monday 08 October 2012 12:01:18 Tomi Valkeinen wrote: >>> On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski >>> wrote: ... >>> Of course, if this is about describing the hardware, the >>> default-mode property doesn't really fit in... >> >> Maybe we should rename it to native-mode then ? > > Hmm, right, if it means native mode, then it is describing the > hardware. But would it make sense to require that the native mode > is the first mode in the list, then? This would make the separate > default-mode/native-mode property not needed. I'm not sure if device-tree guarantees that the nodes enumerate in a specific order. If it does, then that may be a reasonable solution.
Kconfig DRM_USB/DRM_UDL, and select vs. depends, and causing Tegra USB to be disabled
With respect to the following commits: df0b344 drm/usb: select USB_SUPPORT in Kconfig 8f057d7 gpu/mfd/usb: Fix USB randconfig problems ... which end up with the following in next-20120904: config DRM_USB depends on DRM depends on USB_ARCH_HAS_HCD select USB select USB_SUPPORT config DRM_UDL depends on DRM && EXPERIMENTAL depends on USB_ARCH_HAS_HCD select DRM_USB Surely this is backwards; these should be dependencies, not selects? In other words: config DRM_USB depends on DRM && USB config DRM_UDL depends on DRM && EXPERIMENTAL && USB select DRM_USB or perhaps: config DRM_USB depends on DRM && USB config DRM_UDL depends on DRM && EXPERIMENTAL && DRM_USB The problem here is that currently, the dependency logic for USB: config USB depends on USB_ARCH_HAS_HCD ... is duplicated into each of DRM_USB and DRM_UDL, thus requiring both of those to be edited should the dependencies for USB ever change. The current state of the code also causes some strange problem with ARM's tegra_defconfig, whereby running "make tegra_defconfig" will result in USB support fully enabled in .config as expected, yet subsequently running "make oldconfig" will cause all USB support to be removed from .config. For some reason, the above DRM logic is causing CONFIG_USB_ARCH_HAS_HCD not to be selected (perhaps it isn't evaluated because USB is selected, so there's no need to evaluate USB's dependencies?). Arguably, this is a deficiency in Tegra's Kconfig, in that it probably should say: select USB_ARCH_HAS_EHCI not: select USB_ARCH_HAS_EHCI if USB_SUPPORT ... but it has contained the latter for quite some time, and it's always worked before somehow.
Kconfig DRM_USB/DRM_UDL, and select vs. depends, and causing Tegra USB to be disabled
On 09/04/2012 02:00 PM, Guenter Roeck wrote: > On Tue, Sep 04, 2012 at 01:19:12PM -0600, Stephen Warren wrote: >> With respect to the following commits: >> >> df0b344 drm/usb: select USB_SUPPORT in Kconfig >> 8f057d7 gpu/mfd/usb: Fix USB randconfig problems >> >> ... which end up with the following in next-20120904: >> >> config DRM_USB >> depends on DRM >> depends on USB_ARCH_HAS_HCD >> select USB >> select USB_SUPPORT >> >> config DRM_UDL >> depends on DRM && EXPERIMENTAL >> depends on USB_ARCH_HAS_HCD >> select DRM_USB >> >> Surely this is backwards; these should be dependencies, not selects? In >> other words: >> >> config DRM_USB >> depends on DRM && USB >> >> config DRM_UDL >> depends on DRM && EXPERIMENTAL && USB >> select DRM_USB >> >> or perhaps: >> >> config DRM_USB >> depends on DRM && USB >> >> config DRM_UDL >> depends on DRM && EXPERIMENTAL && DRM_USB >> >> The problem here is that currently, the dependency logic for USB: >> >> config USB >> depends on USB_ARCH_HAS_HCD >> >> ... is duplicated into each of DRM_USB and DRM_UDL, thus requiring both >> of those to be edited should the dependencies for USB ever change. > > This should be fixed with in https://patchwork.kernel.org/patch/1373371/ (drm: > udl: usb: Fix recursive Kconfig dependency), which should make it into the > next > iteration of linux-next. Yes, this does appear to solve all the problems for me. Thanks. I still tend to believe that drivers should probably depend on things rather than select them, but given the common precedent for "select USB" that exists here, others clearly don't agree! Sorry; accidentally sent the email too early last time:-(
Re: [PATCH v3 0/2] NVIDIA Tegra DRM driver
On 11/15/2012 02:28 PM, Thierry Reding wrote: > This third version of the patch series cleans up some minor issues that > were found in the previous versions, such as deferred probe not working > properly and the display remaining enabled after the driver is removed. > > I've also put the two patches in this series into the tegra/drm/for-3.8 > branch of my repository on gitorious[0]. The series, Tested-by: Stephen Warren (on Harmony, using the HDMI output) ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Add NVIDIA Tegra30 support
On 11/15/2012 09:58 PM, Mark Zhang wrote: > This patch is based on Thierry's drm patch for Tegra20: > - [PATCH v2 0/6] Device tree updates for host1x support > - [PATCH v3 0/2] NVIDIA Tegra DRM driver > > It adds the support for NVIDIA Tegra30. Mark, I tried to apply this for testing locally, but it doesn't apply. For some reason, all the whitespace in the context has been replaced with spaces. Are your local copies of dc.c and host1x.c indented with spaces for some reason, or did the MS Exchange server corrupt your patch as you sent it (I've previously only observed inbound corruption...) ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Add NVIDIA Tegra30 support
On 11/15/2012 09:58 PM, Mark Zhang wrote:
> This patch is based on Thierry's drm patch for Tegra20:
> - [PATCH v2 0/6] Device tree updates for host1x support
> - [PATCH v3 0/2] NVIDIA Tegra DRM driver
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 216cd0f..6e9f1b4 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -819,6 +819,7 @@ static int tegra_dc_remove(struct platform_device *pdev)
>
> static struct of_device_id tegra_dc_of_match[] = {
> { .compatible = "nvidia,tegra20-dc", },
> + { .compatible = "nvidia,tegra30-dc", },
The Tegra30 entries should come first in the table due to the order the
kernel (currently) matches compatible properties against theses tables.
The same comment applies for all 3 tables.
With the patch manually applied (due to the whitespace issues I
mentioned earlier), and this ordering issue fixed,
Tested-by: Stephen Warren
(On Cardhu, with no HDMI plugged in; see my next email for details).
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: tegra: Use framebuffer pitch as line stride
On 11/22/2012 12:37 PM, Thierry Reding wrote: > Instead of using the stride derived from the display mode, use the pitch > associated with the currently active framebuffer. This fixes a bug where > the LCD display content would be skewed when enabling HDMI with a video > mode different from that of the LCD. This patch certainly doesn't cause any additional issues for me, so: Tested-by: Stephen Warren Howwever, it still doesn't allow both Cardhu's LCD panel and external HDMI port (1080p) to be active at once. If I boot with both enabled, or boot with just the LCD enabled and hot-plug HDMI, as soon as both heads are active, then some kind of display corruption starts; it looks like a clocking issue or perhaps memory underflow. Mark, can you please investigate this. I haven't tested to see if the issue is Tegra30-specific, or also happens on Tegra20. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 5/8] ARM: tegra: Add auxiliary data for nvhost
On 11/26/2012 06:19 AM, Terje Bergstrom wrote:
> Add SoC specific auxiliary data to host1x and gr2d. nvhost uses
> this data.
>
> Signed-off-by: Terje Bergstrom
> Signed-off-by: Arto Merilainen
Arto's S-o-b really should be first and yours last since you're (Terje)
the one who touched the patches last.
> diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c
> b/arch/arm/mach-tegra/board-dt-tegra20.c
I think none of the changes the board-dt-tegra*.c should be made.
AUXDATA is a temporary measure to keep things working during the
transition to device tree. We want to remove entries from the AUXDATA
tables rather than add them. The only thing that's stopping us from
doing so right now is the lack of DT-based clock lookups, which hence
require devices to have a specific name.
> +static struct nvhost_device_data tegra_host1x_info = {
> + .clocks = { {"host1x", UINT_MAX} },
> +static struct nvhost_device_data tegra_gr2d_info = {
> + .clocks = { {"gr2d", UINT_MAX, true}, {"epp", UINT_MAX, true} },
Clock names shouldn't be passed in platform data; instead, clk_get()
should be passed the device object and device-relative (i.e. not global)
clock name. I expect if the driver is fixed to make this change, the
changes to tegra*_clocks_data.c won't be needed either.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 5/8] ARM: tegra: Add auxiliary data for nvhost
On 11/26/2012 11:33 PM, Terje Bergström wrote:
> On 27.11.2012 01:39, Stephen Warren wrote:
>> Clock names shouldn't be passed in platform data; instead, clk_get()
>> should be passed the device object and device-relative (i.e. not global)
>> clock name. I expect if the driver is fixed to make this change, the
>> changes to tegra*_clocks_data.c won't be needed either.
>
> Isn't this code doing exactly that - getting a device relative clock,
> nvhost_module_init() in nvhost.acm.c:
>
> (...)
> /* initialize clocks to known state */
> while (pdata->clocks[i].name && i < NVHOST_MODULE_MAX_CLOCKS) {
> long rate = pdata->clocks[i].default_rate;
> struct clk *c;
>
> c = devm_clk_get(&dev->dev, pdata->clocks[i].name);
The line above is getting the (device-relative) clock name from platform
data, rather than using some fixed name as it should be.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: tegra: Use framebuffer pitch as line stride
On 11/26/2012 08:16 PM, Mark Zhang wrote: > On 11/27/2012 06:37 AM, Stephen Warren wrote: >> On 11/22/2012 12:37 PM, Thierry Reding wrote: >>> Instead of using the stride derived from the display mode, use the pitch >>> associated with the currently active framebuffer. This fixes a bug where >>> the LCD display content would be skewed when enabling HDMI with a video >>> mode different from that of the LCD. >> >> This patch certainly doesn't cause any additional issues for me, so: >> >> Tested-by: Stephen Warren >> >> Howwever, it still doesn't allow both Cardhu's LCD panel and external >> HDMI port (1080p) to be active at once. If I boot with both enabled, or >> boot with just the LCD enabled and hot-plug HDMI, as soon as both heads >> are active, then some kind of display corruption starts; it looks like a >> clocking issue or perhaps memory underflow. > > I haven't observed this issue. What kind of display corruption you mean? > Did it recover after some seconds or the display in LVDS panel was > always corrupted? > > During my testing, I connected HDMI while booting cardhu and I can see > the LVDS and HDMI working with no corruptions. For your viewing pleasure (and playing with my new phone) :-) http://www.youtube.com/watch?v=ZJxJnONz7DA The external monitor is 1920x1200 I believe. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: tegra: Use framebuffer pitch as line stride
On 11/27/2012 11:17 AM, Stephen Warren wrote: > On 11/26/2012 08:16 PM, Mark Zhang wrote: >> On 11/27/2012 06:37 AM, Stephen Warren wrote: >>> On 11/22/2012 12:37 PM, Thierry Reding wrote: >>>> Instead of using the stride derived from the display mode, use the pitch >>>> associated with the currently active framebuffer. This fixes a bug where >>>> the LCD display content would be skewed when enabling HDMI with a video >>>> mode different from that of the LCD. >>> >>> This patch certainly doesn't cause any additional issues for me, so: >>> >>> Tested-by: Stephen Warren >>> >>> Howwever, it still doesn't allow both Cardhu's LCD panel and external >>> HDMI port (1080p) to be active at once. If I boot with both enabled, or >>> boot with just the LCD enabled and hot-plug HDMI, as soon as both heads >>> are active, then some kind of display corruption starts; it looks like a >>> clocking issue or perhaps memory underflow. >> >> I haven't observed this issue. What kind of display corruption you mean? >> Did it recover after some seconds or the display in LVDS panel was >> always corrupted? >> >> During my testing, I connected HDMI while booting cardhu and I can see >> the LVDS and HDMI working with no corruptions. > > For your viewing pleasure (and playing with my new phone) :-) > http://www.youtube.com/watch?v=ZJxJnONz7DA > > The external monitor is 1920x1200 I believe. Jon Mayo says the corruption in the video is display (memory fetch) underflow. Perhaps this is because (IIRC) the BCT I'm using on Cardhu programs the memory controller at a slow rate, and the bootloader and/or kernel is supposed to bump up the rate to the max, but that's not implemented anywhere yet upstream. If you're testing with "fastboot" instead of U-Boot, that might be re-programming the memory frequencies, and hence avoiding this. I guess we have a fun time ahead of us with mode validation and memory controller programming. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 8/8] drm: tegra: Add gr2d device
On 11/28/2012 07:45 AM, Terje Bergström wrote: > On 28.11.2012 16:06, Lucas Stach wrote: >> Why do even need/use dma-buf for this use case? This is all one DRM >> device, even if we separate host1x and gr2d as implementation modules. > > I didn't want to implement dependency to drm gem objects in nvhost, but > we have thought about doing that. dma-buf brings quite a lot of > overhead, so implementing support for gem buffers would make the > sequence a bit leaner. > > nvhost already has infra to support multiple memory managers. > >> So standard way of doing this is: >> 1. create gem object for pushbuffer >> 2. create fake mmap offset for gem obj >> 3. map pushbuf using the fake offset on the drm device >> 4. at submit time zap the mapping >> >> You need this logic anyway, as normally we don't rely on userspace to >> sync gpu and cpu, but use the kernel to handle the concurrency issues. > > Taking a step back - 2D streams are actually very short, in the order of > <100 bytes. Just copying them to kernel space would actually be faster > than doing MMU operations. I'm not sure it's a good idea to have one buffer submission mechanism for the 2D class and another for the 3D/... class, nor to bet that 2D streams will always be short. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm: tegra: Add maintainers entry
On 11/28/2012 12:18 PM, Thierry Reding wrote: > Add myself as the maintainer of the NVIDIA Tegra DRM driver. Aside from one comment below, Acked-by: Stephen Warren > +L: dri-devel@lists.freedesktop.org Should linux-te...@vger.kernel.org also be CC'd so that everything Tegra-related goes to one place? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 1/8] video: tegra: Add nvhost driver
On 11/29/2012 03:21 AM, Terje Bergström wrote:
> On 28.11.2012 23:23, Thierry Reding wrote:
...
>>> + regs = platform_get_resource(dev, IORESOURCE_MEM, 0);
>>> + intr0 = platform_get_resource(dev, IORESOURCE_IRQ, 0);
>>> + intr1 = platform_get_resource(dev, IORESOURCE_IRQ, 1);
>>> +
>>> + if (!regs || !intr0 || !intr1) {
>>
>> I prefer to have these checked for explicitly, one by one for
>> readability and potentially more useful diagnostics.
>
> Can do.
>
>> Also you should be using platform_get_irq() for interrupts. Furthermore
>> the host1x DT node (and the TRM) name the interrupts "syncpt" and
>> "general", so maybe those would be more useful variable names than
>> "intr0" and "intr1".
>>
>> But since you don't use them anyway they shouldn't be part of this
>> patch.
>
> True. I might also as well delete the general interrupt altogether, as
> we don't use it for any real purpose.
Do make sure the interrupts still are part of the DT binding though, so
that the binding fully describes the HW, and the interrupt is available
to retrieve if we ever do use it in the future.
>>> + for (i = 0; i < pdata->num_clks; i++)
>>> + clk_prepare_enable(pdata->clk[i]);
>>> + nvhost_syncpt_reset(&host->syncpt);
>>> + for (i = 0; i < pdata->num_clks; i++)
>>> + clk_disable_unprepare(pdata->clk[i]);
>>
>> Stephen already hinted at this when discussing the AUXDATA. You should
>> explicitly request the clocks.
>
> I'm not too happy about that idea. The clock management code is generic
> for all modules, and that's why it's driven by a data structure. Now
> Stephen and you ask me to unroll the loops and make copies of the code
> to facilitate different modules and different SoCs.
You can still create tables of clocks inside the driver and loop over
them. So, loop unrolling isn't related to my comments at least. It's
just that clk_get() shouldn't take its parameters from platform data.
But if these are clocks for (arbitrary) child modules (that may or may
not exist dynamically), why aren't the drivers for the child modules
managing them?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 1/8] video: tegra: Add nvhost driver
On 11/29/2012 04:47 AM, Thierry Reding wrote: > On Thu, Nov 29, 2012 at 12:21:04PM +0200, Terje Bergström wrote: >> On 28.11.2012 23:23, Thierry Reding wrote: >>> This could be problematic. Since drivers/video and >>> drivers/gpu/drm are separate trees, this would entail a >>> continuous burden on keeping both trees synchronized. While I >>> realize that eventually it might be better to put the host1x >>> driver in a separate place to accomodate for its use by other >>> subsystems, I'm not sure moving it here right away is the best >>> approach. >> >> I understand your point, but I hope also that we'd end up with >> something that can be used as basis for the downstream kernel to >> migrate to upstream stack. >> >> The key point here is to make the API between nvhost and tegradrm >> as small and robust to changes as possible. > > I agree. But I also fear that there will be changes eventually and > having both go in via different tree requires those trees to be > merged in a specific order to avoid breakage should the API change. > This will be particularly ugly in linux-next. > > That's why I explicitly proposed to take this into > drivers/gpu/drm/tegra for the time being, until we can be > reasonably sure that the API is fixed. Then I'm fine with moving it > wherever seems the best fit. Even then there might be the > occasional dependency, but they should get fewer and fewer as the > code matures. It is acceptable for one maintainer to ack patches, and another maintainer to merge a series that touches both "their own" code and code owned by another tree. This should of course only be needed when inter-module APIs change; changes to code within a module shouldn't require this. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 2/8] video: tegra: Add syncpoint wait and interrupts
On 11/29/2012 01:44 AM, Thierry Reding wrote:
> On Mon, Nov 26, 2012 at 03:19:08PM +0200, Terje Bergstrom wrote:
>> diff --git a/drivers/video/tegra/host/host1x/host1x_intr.c
>> b/drivers/video/tegra/host/host1x/host1x_intr.c
> [...]
>> +/* Spacing between sync registers */ +#define REGISTER_STRIDE 4
>
> Erm... no. The usual way you should be doing this is either make
> the register definitions account for the stride or use accessors
> that apply the stride. You should be doing the latter anyway to
> make accesses. For example:
>
> static inline void host1x_syncpt_writel(struct host1x *host1x,
> unsigned long value, unsigned long offset) { writel(value,
> host1x->regs + SYNCPT_BASE + offset); }
>
> static inline unsigned long host1x_syncpt_readl(struct host1x
> *host1x, unsigned long offset) { return readl(host1x->regs +
> SYNCPT_BASE + offset); }
>
> Alternatively, if you want to pass the register index instead of
> the offset, you can use just multiply the offset in that function:
>
> writel(value, host1x->regs + SYNCPT_BASE + (offset << 2));
>
> The same can also be done with the non-syncpt registers.
It seems like reasonable documentation to replace "<< 2" with "*
REGISTER_STRIDE" here.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 1/8] video: tegra: Add nvhost driver
On 11/30/2012 03:38 AM, Thierry Reding wrote: > On Fri, Nov 30, 2012 at 10:56:39AM +0200, Terje Bergström wrote: >> On 29.11.2012 13:47, Thierry Reding wrote: >>> On Thu, Nov 29, 2012 at 12:21:04PM +0200, Terje Bergström >>> wrote: Tegra20 and Tegra30 are compatible, but future chips are not. I was hoping we would be ready in upstream kernel for future chips. >>> >>> I think we should ignore that problem for now. Generally >>> planning for any possible combination of incompatibilities >>> leads to overgeneralized designs that require precisely these >>> kinds of indirections. >>> >>> Once some documentation for Tegra 40 materializes we can start >>> thinking about how to encapsulate the incompatible code. >> >> I think here our perspectives differ a lot. That is natural >> considering the company I work for and company you work for, so >> let's try to sync the perspective. >> >> In my reality, whatever is in market is old news and I barely >> work on them anymore. Upstreaming activity is the exception. 90% >> of my time is spent dealing with future chips which I know cannot >> be handled without this split to logical and physical driver >> parts. >> >> For you, Tegra2 and Tegra3 are the reality. > > To be fair, Tegra2 and Tegra3 are the reality for *everyone* > *outside* NVIDIA. > > It's great that you spend most of your time working with future > chips, but unless you submit the code for inclusion or review > nobody upstream needs to be concerned about the implications. Most > people don't have time to waste so we naturally try to keep the > maintenance burden to a minimum. > > The above implies that when you submit code it shouldn't contain > pieces that prepare for possible future extensions which may or may > not be submitted (the exception being if such changes are part of a > series where subsequent patches actually use them). The outcome is > that the amount of cruft in the mainline kernel is kept to a > minimum. And that's a very good thing. I think there's room for letting Terje's complete knowledge of future chips guide the design of the current code that's sent upstream. Certainly we shouldn't add a ton of unnecessary abstraction layers right now that aren't needed for Tegra20/30, but if there's some decision that doesn't affect the bloat, opaqueness, ... of the current code but one choice is better for future development without serious negatives for the current code, it's pretty reasonable to make that decision rather than the other. (That all said, I haven't really followed the details of this particular point, so I can't say how my comment applies to any decisions being made right now - just that we shouldn't blanket reject future knowledge when making decisions) After all, making the right decision now will reduce the number/size of patches later, and hence reduce code churn and reviewer load. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 1/8] video: tegra: Add nvhost driver
On 12/01/2012 07:58 AM, Thierry Reding wrote:
> On Sat, Dec 01, 2012 at 01:31:02PM +0200, Terje Bergström wrote:
...
>> I was thinking of definitions like this:
>>
>> static inline u32 host1x_sync_cfpeek_ctrl_cfpeek_addr_f(u32 v) {
>> return (v & 0x1ff) << 0; }
>>
>> versus
>>
>> #define host1x_sync_cfpeek_ctrl_cfpeek_addr_f(v) ((v) >> 16) &
>> 0x3ff
>>
>> Both of these produce the same machine code and have same usage,
>> but the latter has type checking and code coverage analysis and
>> the former is (in my eyes) clearer. In both of these cases the
>> usage is like this:
>>
>> writel(host1x_sync_cfpeek_ctrl_cfpeek_ena_f(1) |
>> host1x_sync_cfpeek_ctrl_cfpeek_channr_f(chid) |
>> host1x_sync_cfpeek_ctrl_cfpeek_addr_f(rd_ptr), m->sync_aperture +
>> host1x_sync_cfpeek_ctrl_r());
>
> Again there's no precedent for doing this with static inline
> functions. You can do the same with macros. Type checking isn't an
> issue in these cases since we're talking about bitfields for which
> no proper type exists.
I suspect the inline functions could encode signed-vs-unsigned fields,
perhaps catch u8 variables when they should have been u32, etc.?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: tegra: Use framebuffer pitch as line stride
On 12/03/2012 08:00 PM, Mark Zhang wrote: > On 11/28/2012 02:37 PM, Mark Zhang wrote: >> On 11/28/2012 05:39 AM, Stephen Warren wrote: >>> On 11/27/2012 11:17 AM, Stephen Warren wrote: >>>> On 11/26/2012 08:16 PM, Mark Zhang wrote: >>>>> On 11/27/2012 06:37 AM, Stephen Warren wrote: >>>>>> On 11/22/2012 12:37 PM, Thierry Reding wrote: >>>>>>> Instead of using the stride derived from the display mode, use the pitch >>>>>>> associated with the currently active framebuffer. This fixes a bug where >>>>>>> the LCD display content would be skewed when enabling HDMI with a video >>>>>>> mode different from that of the LCD. >>>>>> >>>>>> This patch certainly doesn't cause any additional issues for me, so: >>>>>> >>>>>> Tested-by: Stephen Warren >>>>>> >>>>>> Howwever, it still doesn't allow both Cardhu's LCD panel and external >>>>>> HDMI port (1080p) to be active at once. If I boot with both enabled, or >>>>>> boot with just the LCD enabled and hot-plug HDMI, as soon as both heads >>>>>> are active, then some kind of display corruption starts; it looks like a >>>>>> clocking issue or perhaps memory underflow. >>>>> >>>>> I haven't observed this issue. What kind of display corruption you mean? >>>>> Did it recover after some seconds or the display in LVDS panel was >>>>> always corrupted? >>>>> >>>>> During my testing, I connected HDMI while booting cardhu and I can see >>>>> the LVDS and HDMI working with no corruptions. >>>> >>>> For your viewing pleasure (and playing with my new phone) :-) >>>> http://www.youtube.com/watch?v=ZJxJnONz7DA >>>> >>>> The external monitor is 1920x1200 I believe. >>> >>> Jon Mayo says the corruption in the video is display (memory fetch) >>> underflow. Perhaps this is because (IIRC) the BCT I'm using on Cardhu >>> programs the memory controller at a slow rate, and the bootloader and/or >>> kernel is supposed to bump up the rate to the max, but that's not >>> implemented anywhere yet upstream. If you're testing with "fastboot" >>> instead of U-Boot, that might be re-programming the memory frequencies, >>> and hence avoiding this. >>> >> >> All right, I just test the framebuffer console and "xinit", I didn't >> install the whole ubuntu. >> >> I'll install the ubuntu in my cardhu and see whether I have this kind of >> issues. > > Hi swarren, I installed ubuntu 12.04 in l4t and didn't observe the issue > you described. The display worked with no corruptions. I can show you > the video if you want. > > What I used for testing is a cardhu board with our downstream U-Boot. > > But the HDMI didn't work. The HDMI monitor showed this: "CANNOT DISPLAY > THIS VIDEO MODE, CHANGE COMPUTER DISPLAY INPUT TO 1920x1080@60HZ". So > sounds like the clock setting has some problems... I'll have a look at this. Oh, I thought I'd followed up on this - Jon Mayo said it was display underflow due to lack of memory bandwidth. IIRC, this may be due to the BCT programming the memory controller for conservative settings that don't require non-default voltages from the PMIC, with the expectation that the bootloader or kernel will reprogram everything for correct performance. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: First version of host1x intro
On 12/06/2012 01:13 AM, Mark Zhang wrote: > On 12/06/2012 04:00 PM, Lucas Stach wrote: >> Am Donnerstag, den 06.12.2012, 15:49 +0800 schrieb Mark Zhang: > [...] >>> >>> OK. So these relocation addresses are used to let userspace tells kernel >>> which buffers mentioned in the command should be relocated to addresses >>> which host1x clients able to reach. >>> >> Yep, preferably all buffers referenced by a command stream should >> already be set up in such a position (CMA with Tegra2) or the relocation >> should be nothing more than setting up IOMMU page tables (Tegra3). >> >>> I'm also wondering that, if our driver understands the stuffs in the >>> commands, maybe we can find out all addresses in the command, in that >>> way, we will not need userspace tells us which are the addresses need to >>> be relocated, right? >> >> No. How will the kernel ever know which buffer gets referenced in a >> command stream? All the kernel sees is is a command stream with >> something like "blit data to address 0xADDR" in it. The only info that >> you can gather from that is that there must be some buffer to blit into. >> Neither do you know which buffer the stuff should be going to, nor can >> you know if you blit to offset zero in this buffer. It's perfectly valid >> to only use a subregion of a buffer. >> >> Or maybe I'm misunderstanding you and you mean it the other way around. >> You don't let userspace dictate the addresses, the relocation >> information just tells the kernel to find the addresses of the >> referenced buffers for you and insert them, instead of the dummy >> information, into the command stream. > > Yes, I think this is what I mean. No dummy information in the command > stream, userspace just fills the address which it uses(actually this is > cpu address of the buffer) in the command stream, and our driver must > have a HashTable or something which contains the buffer address pair -- > (cpu address, dma address), so our driver can find the dma addresses for > every buffer then modify the addresses in the command stream. Typically there would be no CPU address; there's no need in most cases to ever map most buffers to the CPU. Automatically parsing the buffer sounds like an interesting idea, but then the kernel relocation code would have to know the meaning of every single register or command-stream "method" in order to know which of them take a buffer address as an argument. I am not familiar with this HW specifically, so perhaps it's much more regular than I think and it's actually easy to do that, but I imagine it'd be a PITA to implement that (although perhaps we have to for the command-stream validation stuff anyway?). Also, it'd be a lot quicker at least for larger command buffers to go straight to the few locations in the command stream where a buffer is referenced (i.e. use the side-band metadata for relocation) rather than having the CPU re-read the entire command stream in the kernel to parse it. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/13/2012 01:57 AM, Thierry Reding wrote: > On Thu, Dec 13, 2012 at 10:48:55AM +0200, Terje Bergström wrote: >> On 12.12.2012 18:08, Thierry Reding wrote: >>> I've briefly discussed this with Stephen on IRC because I >>> thought I had remembered him objecting to the idea of adding a >>> dummy device just for this purpose. It turns out, however, that >>> what he didn't like was to add a dummy node to the DT just to >>> make this happen, but he has no (strong) objections to a dummy >>> platform device. >>> >>> While I'm not very happy about that solution, I've been going >>> over it for a week now and haven't come up with any better >>> alternative that doesn't have its own disadvantages. So perhaps >>> we should go ahead and implement that. For the host1x driver >>> this really just means creating a platform device and adding it >>> to the system, with some of the fields tweaked to make things >>> work. >> >> Even the virtual device is not too beautiful. The problem is that >> the virtual device is not physical parent for DC, HDMI, etc, so >> dev_get_drvdata(pdev->dev.parent) returns the data from host1x >> device, not the virtual device. >> >> We'll post with something that goes around this, but it's not >> going to be too pretty. Let's try to find the solution once we >> get the code out. > > After some more discussion with Stephen on IRC we came to the > conclusion that the easiest might be to have tegra-drm call into > host1x with something like: > > void host1x_set_drm_device(struct host1x *host1x, struct device > *dev); If host1x is registering the dummy device that causes tegradrm to be instantiated, then presumably there's no need for the API above, since host1x will already have the struct device * for tegradrm, since it created it? > Once the dummy device has been properly set up and have each client > use > > struct device *host1x_get_drm_device(struct host1x *host1x); > > to obtain a pointer to the dummy device, such that it can receive > the driver-private data using dev_get_drvdata() on the returned > device. As long as tegra-drm hasn't registered with host1x yet, the > above function could return ERR_PTR(-EPROBE_DEFER), so that > dependencies are automatically handled. This is required because, > tegra-drm not being the parent of the child devices, it can be > guaranteed that it is probed before any of the children. > > That way we should be able to get around any circular > dependencies, since we only call into host1x from tegra-drm but not > the other way around. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/13/2012 11:09 PM, Terje Bergström wrote: > On 13.12.2012 19:58, Stephen Warren wrote: >> On 12/13/2012 01:57 AM, Thierry Reding wrote: >>> After some more discussion with Stephen on IRC we came to the >>> conclusion that the easiest might be to have tegra-drm call into >>> host1x with something like: >>> >>> void host1x_set_drm_device(struct host1x *host1x, struct device >>> *dev); >> >> If host1x is registering the dummy device that causes tegradrm to be >> instantiated, then presumably there's no need for the API above, since >> host1x will already have the struct device * for tegradrm, since it >> created it? > > I didn't add the dummy device in my latest patch set. I first set out to > add it, and moved the drm global data to be drvdata of that device. Then > I noticed that it doesn't actually help at all. > > The critical accesses to the global data are from probes of DC, HDMI, > etc. OK > They want to get the global data by getting drvdata of their parent. There's no reason that /has/ to be so; they can get any information from wherever it is, rather than trying to require it to be somewhere it isn't. > The dummy device is not their parent - host1x is. There's no > connection between the dummy and the real client devices. It's quite possible for the client devices to ask their actual parent (host1x) for the tegradrm struct device *, by calling a host1x API, and have host1x implement that API by returning the dummy/virtual device that it created. That should be just 1-2 lines of code to implement in host1x, plus maybe a couple more to correctly return -EPROBE_DEFERRED when appropriate. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/16/2012 09:37 AM, Terje Bergström wrote: ... > ... Sure we could tell DC to ask its parent > (host1x), and call host1x driver with platform_device pointer found that > way, and host1x would return a pointer to tegradrm's data. Hanging the > data onto host1x driver is just a more complicated way of implementing > global data No it's not; at that point, the data is no longer global, but specific to the driver instance. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/20/2012 02:17 AM, Terje Bergström wrote: > On 16.12.2012 14:16, Thierry Reding wrote: >> Okay, so we're back on the topic of using globals. I need to assert >> again that this is not an option. If we were to use globals, then we >> could just as well leave out the dummy device and just do all of that in >> the tegra-drm driver's initialization function. > > I found a way of dropping the global in a straightforward way, and > introduce dummy device for drm_platform_init(). > > I added dummy device and driver, and moved the tegradrm global > (previously called struct host1x *host1x) allocation to happen in the > probe. In addition, probe calls device tree node traversal to do the > tegra_drm_add_client() calls. The dummy device is owner for this global. > > I changed the device tree node traversal so that it goes actually > through each host1x child, checks if it's supported by tegradrm, and if > so, sets its drvdata to point to the tegradrm data. I'm not sure that sounds right. drvdata is something that a driver should manage itself. What's wrong with just having each device ask the host1x (its parent) for a pointer to the (dummy) tegradrm device. That seems extremely simple, and doesn't require abusing existing stuff like drvdata for non-standard purposes. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/20/2012 10:46 AM, Terje Bergström wrote: > On 20.12.2012 19:14, Stephen Warren wrote: >> I'm not sure that sounds right. drvdata is something that a driver >> should manage itself. >> >> What's wrong with just having each device ask the host1x (its parent) >> for a pointer to the (dummy) tegradrm device. That seems extremely >> simple, and doesn't require abusing existing stuff like drvdata for >> non-standard purposes. > > This is tegradrm's own data, and it's tegradrm which accesses the > pointer. So it's entirely something that the driver takes care of itself. So it's fine for the tegradrm driver to manipulate the tegradrm (virtual) device's drvdata pointer. It's not fine for tegradrm to manipulate the drvdata pointer of other devices; the host1x children. The drvdata pointer for other devices is reserved for those devices' driver's use only. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/20/2012 02:34 PM, Terje Bergström wrote: > On 20.12.2012 22:30, Thierry Reding wrote: >> The problem with your proposed solution is that, even any of Stephen's >> valid objections aside, it won't work. Once the tegra-drm module is >> unloaded, the driver's data will be left in the current state and the >> link to the dummy device will be lost. > > The dummy device is created by tegradrm's module init, because it's used No, the tegradrm driver object might be registered by tegradrm's module init, but the dummy tegradrm platform device would need to be created/registered by host1x's probe. Otherwise, the device would get created even if there was no host1x/... in the system (disabled for some reason, multi-SoC kernel, ...) ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/20/2012 02:50 PM, Thierry Reding wrote: > On Thu, Dec 20, 2012 at 11:34:26PM +0200, Terje Bergström wrote: >> On 20.12.2012 22:30, Thierry Reding wrote: >>> The problem with your proposed solution is that, even any of >>> Stephen's valid objections aside, it won't work. Once the >>> tegra-drm module is unloaded, the driver's data will be left in >>> the current state and the link to the dummy device will be >>> lost. >> >> The dummy device is created by tegradrm's module init, because >> it's used only by tegradrm. When tegradrm is unloaded, all the >> tegradrm devices and drivers are unloaded and freed, including >> the dummy one. > > No, the children platform devices of host1x are not freed, so > their driver data will still contain the data set by the previous > call to their driver's .probe() function. > > But reading what you said again, you were proposing to set the > children driver data from the tegra-drm dummy device's .probe(). In > that case it would probably work. Many things would work, but since tegradrm isn't (or at least should not be) the driver for the platform devices which are host1x's children, it shouldn't be randomly screwing around with those devices' drvdata fields. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 12/21/2012 01:57 AM, Arto Merilainen wrote: > On 12/20/2012 07:14 PM, Stephen Warren wrote: >> >> What's wrong with just having each device ask the host1x (its parent) >> for a pointer to the (dummy) tegradrm device. That seems extremely >> > > We are talking about creating a dummy device because: > - we need to give something for drm_platform_init(), > - drm related data should be stored somewhere, Yes to those too, I believe. > - some data must be passed to all driver probe() functions. This is not > hw-related data, but just some lists that are used to ensure that all > drivers have been initialised before calling drm_platform_init(). I haven't really thought through /when/ host1x would create the dummy device. I guess if tegradrm really must initialize after all the devices that it uses (rather than using something like deferred probe) then the above may be true. > All these issues are purely tegra-drm related and solving them elsewhere > (in host1x) would be just wrong! host1x would not even use the dummy > device for anything so creating it there seems bizarre. I see the situation more like: * There's host1x hardware. * There's a low-level driver just for host1x itself; the host1x driver. * There's a high-level driver for the entire host1x complex of devices. That is tegradrm. There may be more high-level drivers in the future (e.g. v4l2 camera driver if it needs to aggregate a bunch of host1x sub-devices liek tegradrm does). * The presence of the host1x DT node logically implies that both the two drivers in the previous two points should be instantiated. * Linux instantiates a single device per DT node. * So, it's host1x's responsibility to instantiate the other device(s). I think it's reasonable for the host1x driver to know exactly what features the host1x HW complex supports; raw host1x access being one, and the higher level concept of a tegradrm being another. So, it's reasonable for host1x to trigger the instantiation of tegradrm. * If DRM core didn't stomp on the device object's drvdata (or whichever field it is), I would recommend not creating a dummy device at all, but rather having the host1x driver directly implement multiple driver interfaces. There are many examples like this already in the kernel, e.g. combined GPIO+IRQ driver, combined GPIO+IRQ+pinctrl driver, etc. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv4 5/8] drm: tegra: Remove redundant host1x
On 12/21/2012 11:50 PM, Terje Bergström wrote:
> On 21.12.2012 16:36, Thierry Reding wrote:
>> On Fri, Dec 21, 2012 at 01:39:21PM +0200, Terje Bergstrom wrote:
>>> +static struct platform_driver tegra_drm_platform_driver = {
>>> + .driver = {
>>> + .name = "tegradrm",
>>
>> This should be "tegra-drm" to match the module name.
>
> We've actually created two problems.
>
> First is that the device name should match driver name which should
> match module name. But host1x doesn't know the module name of tegradrm.
There's no hard requirement for the device/driver name to match the
module name. It's good thing to do, but nothing will blow up if it don't
(modules can use MODULE_ALIAS() to declare which drivers they expose).
But, what's the problem with host1x knowing the driver name; the host1x
driver and tegradrm driver are both part of the same code-base, so this
seems trivial to achieve.
> Second problem is that host1x driver creates tegradrm device even if
> tegradrm isn't loaded to system.
That's fine. If there's no driver, the device simply won't be probe()d.
That's just like a device node existing in device tree, but the driver
for it not being enabled in the kernel, or the relevant module not being
inserted.
> These mean that the device has to be created in tegra-drm module to have
I definitely disagree here.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 01/04/2013 03:09 AM, Terje Bergström wrote: ... > I think we have now two ways to go forward with cons and pros: > 1) Keep host1x and tegra-drm as separate driver >+ Code almost done >- we need dummy device and dummy driver >- extra code and API when host1x creates dummy device and its passed > to tegra-drm Just to play devil's advocate: I suspect that's only a few lines of code. >- tegra-drm device would need to be a child of host1x device. Having > virtual and real devices as host1x children sounds weird. And I doubt that would cause problems. > 2) Merge host1x and tegra-drm into one module. drm is a subcomponent, > and whatever other personalities we wish would also be subcomponents of > host1x. host1x calls tegra-drm directly to handle preparation for drm > initialization. As they're in the same module, circular dependency is ok. >+ Simpler conceptually (no dummy device/driver) >+ Less code >- Proposal doesn't yet exist But that said, I agree this approach would be very reasonable; it seems to me that host1x really is the main HW behind a DRM driver or a V4L2 driver or ... As such, it seems quite reasonable for a single struct device to exist that represents host1x, and for the driver for that device to register both a DRM and a V4L2 driver etc. The code could physically be organized into separate modules, and under different Kconfig options for configurability etc. But either way, I'll let you (Thierry and Terje) work out which way to go. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC v2 6/8] gpu: drm: tegra: Remove redundant host1x
On 01/07/2013 01:20 AM, Terje Bergström wrote: > On 04.01.2013 22:25, Stephen Warren wrote: >> On 01/04/2013 03:09 AM, Terje Bergström wrote: >> ... >>> I think we have now two ways to go forward with cons and pros: >>> 1) Keep host1x and tegra-drm as separate driver >>>+ Code almost done >>>- we need dummy device and dummy driver >>>- extra code and API when host1x creates dummy device and its passed >>> to tegra-drm ... >>> 2) Merge host1x and tegra-drm into one module. drm is a subcomponent, >>> and whatever other personalities we wish would also be subcomponents of >>> host1x. host1x calls tegra-drm directly to handle preparation for drm >>> initialization. As they're in the same module, circular dependency is ok. >>>+ Simpler conceptually (no dummy device/driver) >>>+ Less code >>>- Proposal doesn't yet exist >> >> But that said, I agree this approach would be very reasonable; it seems >> to me that host1x really is the main HW behind a DRM driver or a V4L2 >> driver or ... As such, it seems quite reasonable for a single struct >> device to exist that represents host1x, and for the driver for that >> device to register both a DRM and a V4L2 driver etc. The code could >> physically be organized into separate modules, and under different >> Kconfig options for configurability etc. >> >> But either way, I'll let you (Thierry and Terje) work out which way to go. > > If we want separate modules, we'd need the dummy device & dummy driver > binding between them. I haven't really thought it through, but I don't think so; I was thinking separate modules more just to allow linking smaller chunks of code at once rather than allowing optional functionality via loading (or not) various modules. Hence, simple function calls between the files/modules. Still, there may well be no need at all to split it into separate modules. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/exynos: Get HDMI version from device tree
On 01/07/2013 01:43 PM, Sean Paul wrote: > Add a property to the hdmi node so we can specify the HDMI version in > the device tree instead of just defaulting to v1.4 with the existence of > the dt node. > diff --git a/Documentation/devicetree/bindings/drm/exynos/hdmi.txt > b/Documentation/devicetree/bindings/drm/exynos/hdmi.txt > @@ -11,6 +11,8 @@ Required properties: > c) pin function mode. > d) optional flags and pull up/down. > e) drive strength. > +- samsung,supports-hdmi-1.4: Define if device supports HDMI v1.4 > +- samsung,supports-hdmi-1.3: Define if device supports HDMI v1.3 Which device; the HDMI controller in the SoC, or the HDMI sink? The HDMI sync reports what it supports in the EDID, so there shouldn't be a need to duplicate this in the device tree (besides, how can you know what the user plugged in without parsing the EDID?) ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/exynos: Get HDMI version from device tree
On 01/07/2013 04:12 PM, Sean Paul wrote: > > On Jan 7, 2013 5:32 PM, "Stephen Warren" <mailto:swar...@wwwdotorg.org>> wrote: >> >> On 01/07/2013 01:43 PM, Sean Paul wrote: >> > Add a property to the hdmi node so we can specify the HDMI version in >> > the device tree instead of just defaulting to v1.4 with the existence of >> > the dt node. >> >> > diff --git a/Documentation/devicetree/bindings/drm/exynos/hdmi.txt > b/Documentation/devicetree/bindings/drm/exynos/hdmi.txt >> >> > @@ -11,6 +11,8 @@ Required properties: >> > c) pin function mode. >> > d) optional flags and pull up/down. >> > e) drive strength. >> > +- samsung,supports-hdmi-1.4: Define if device supports HDMI v1.4 >> > +- samsung,supports-hdmi-1.3: Define if device supports HDMI v1.3 >> >> Which device; the HDMI controller in the SoC, or the HDMI sink? >> > > It's the controller. Ah OK. Is it different versions of the controller HW IP block that support the different HDMI versions, or is it some kind of fusing/configuration with the same HW block? If different versions of HW, wouldn't this difference usually be represented by different compatible values, that indicate the exact HW version? I guess if the only difference really is just the HDMI support and there are no other bugs/quirks/enhancements, a separate property might make sense. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/exynos: Get HDMI version from device tree
On 01/08/2013 01:16 PM, Sean Paul wrote: > Add a property to the hdmi node so we can specify the HDMI version in > the device tree instead of just defaulting to v1.4 with the existence of > the dt node. I guess this seems OK to me if required, although I'd certainly like to see someone familiar with the Exynos HW confirm whether this should be driven purely by DT compatible value for the HDMI IP block instead though. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv5 7/8] ARM: tegra: Add board data and 2D clocks
On 01/15/2013 04:26 AM, Terje Bergstrom wrote:
> Add a driver alias gr2d for Tegra 2D device, and assign a duplicate
> of 2D clock to that driver alias.
FYI on this one patch - it won't be applied to the Tegra tree until
after Prashant's common clock framework changes are applied. As such, it
will need some rework once those patches are applied, or perhaps won't
even be relevant any more; see below.
> diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c
> b/arch/arm/mach-tegra/board-dt-tegra20.c
> + OF_DEV_AUXDATA("nvidia,tegra20-gr2d", 0x5414, "gr2d", NULL),
I assume the only reason to add AUXDATA is to give the device a specific
name, which will then match the driver name in the clock driver:
> - CLK_DUPLICATE("2d", "tegra_grhost", "gr2d"),
> + CLK_DUPLICATE("2d", "gr2d", "gr2d"),
If so, this shouldn't be needed once the common clock framework patches
are applied, since all device clocks will be retrieved from device tree,
and hence the device name will be irrelevant; the phandle in device tree
is all that will matter.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv5 7/8] ARM: tegra: Add board data and 2D clocks
On 01/16/2013 01:10 AM, Terje Bergström wrote: > On 15.01.2013 20:44, Stephen Warren wrote: >> On 01/15/2013 04:26 AM, Terje Bergstrom wrote: >>> Add a driver alias gr2d for Tegra 2D device, and assign a duplicate >>> of 2D clock to that driver alias. >> >> FYI on this one patch - it won't be applied to the Tegra tree until >> after Prashant's common clock framework changes are applied. As such, it >> will need some rework once those patches are applied, or perhaps won't >> even be relevant any more; see below. > > It looks like I need to just drop the patch 7(8 "ARM: tegra: Add board > data and 2D clocks" and change the 2D code to access the clock without a > name. Do you agree? That sounds right to me, yes. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: avoid "mono_time_offset may be used uninitialized"
From: Stephen Warren
Silence the following:
drivers/gpu/drm/drm_irq.c: In function 'drm_calc_vbltimestamp_from_scanoutpos':
drivers/gpu/drm/drm_irq.c:583:24: warning: 'mono_time_offset.tv64' may be used
uninitialized in this function
... by always initializing mono_time_offset to zero. In practice, this
warning is false, since mono_time_offset is both set and used under the
condition if (!drm_timestamp_monotonic). However, at least my compiler
can't be coerced into realizing this; almost any code between the if
blocks that set and use the variable causes this warning.
Signed-off-by: Stephen Warren
---
I'm not sure what the current thinking is re: silencing warnings like this;
IIRC some people may dislike this kind of change. Perhaps if this change
alone is problematic, one could additionally remove the if condition on the
use of mono_time_offset, thus always using the value?
---
drivers/gpu/drm/drm_irq.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 19c01ca..b199818 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -580,7 +580,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device
*dev, int crtc,
unsigned flags,
struct drm_crtc *refcrtc)
{
- ktime_t stime, etime, mono_time_offset;
+ ktime_t stime, etime, mono_time_offset = {0};
struct timeval tv_etime;
struct drm_display_mode *mode;
int vbl_status, vtotal, vdisplay;
--
1.7.10.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
