Re: [PATCH v3 07/11] iio: core: Add new DMABUF interface infrastructure

[Lars-Peter Clausen]

On 4/4/23 00:55, Paul Cercueil wrote:

[...]

+   priv = attach->importer_priv;
+   list_del_init(>entry);
+
+   iio_buffer_dmabuf_put(attach);
+   iio_buffer_dmabuf_put(attach);
+

Is this intended? Looks suspicious...

It is intended, yes. You want to release the dma_buf_attachment that's
created in iio_buffer_attach_dmabuf(), and you need to call
iio_buffer_find_attachment() to get a pointer to it, which also gets a
second reference - so it needs to unref twice.

Let's add a comment documenting that.

Re: [PATCH 11/15] iio: buffer-dma: Boost performance using write-combine cache setting

[Lars-Peter Clausen]

On 11/27/21 5:05 PM, Jonathan Cameron wrote:

Non-coherent mapping with no cache sync:
- fileio:
 read:  156 MiB/s
 write: 123 MiB/s
- dmabuf:
 read:  234 MiB/s (capped by sample rate)
 write: 182 MiB/s

Non-coherent reads with no cache sync + write-combine writes:
- fileio:
 read:  156 MiB/s
 write: 140 MiB/s
- dmabuf:
 read:  234 MiB/s (capped by sample rate)
 write: 210 MiB/s


A few things we can deduce from this:

* Write-combine is not available on Zynq/ARM? If it was working, it
should give a better performance than the coherent mapping, but it
doesn't seem to do anything at all. At least it doesn't harm
performance.

I'm not sure it's very relevant to this sort of streaming write.
If you write a sequence of addresses then nothing stops them getting combined
into a single write whether or not it is write-combining.


There is a difference at which point they can get combined. With 
write-combine they can be coalesced into a single transaction anywhere 
in the interconnect, as early as the CPU itself. Without write-cobmine 
the DDR controller might decide to combine them, but not earlier. This 
can make a difference especially if the write is a narrow write, i.e. 
the access size is smaller than the buswidth.


Lets say you do 32-bit writes, but your bus is 64 bits wide. With WC two 
32-bits can be combined into a 64-bit write. Without WC that is not 
possible and you are potentially not using the bus to its fullest 
capacity. This is especially true if the memory bus is wider than the 
widest access size of the CPU.

Re: [PATCH 01/15] iio: buffer-dma: Get rid of incoming/outgoing queues

[Lars-Peter Clausen]

On 11/22/21 4:16 PM, Paul Cercueil wrote:

Hi Lars,

Le lun., nov. 22 2021 at 16:08:51 +0100, Lars-Peter Clausen 
 a écrit :

On 11/21/21 9:08 PM, Paul Cercueil wrote:



Le dim., nov. 21 2021 at 19:49:03 +0100, Lars-Peter Clausen 
 a écrit :

On 11/21/21 6:52 PM, Paul Cercueil wrote:

Hi Lars,

Le dim., nov. 21 2021 at 17:23:35 +0100, Lars-Peter Clausen 
 a écrit :

On 11/15/21 3:19 PM, Paul Cercueil wrote:

The buffer-dma code was using two queues, incoming and outgoing, to
manage the state of the blocks in use.

While this totally works, it adds some complexity to the code,
especially since the code only manages 2 blocks. It is much 
easier to
just check each block's state manually, and keep a counter for 
the next

block to dequeue.

Since the new DMABUF based API wouldn't use these incoming and 
outgoing

queues anyway, getting rid of them now makes the upcoming changes
simpler.

Signed-off-by: Paul Cercueil 
The outgoing queue is going to be replaced by fences, but I think 
we need to keep the incoming queue.


Blocks are always accessed in sequential order, so we now have a 
"queue->next_dequeue" that cycles between the buffers 
allocated for fileio.



[...]
@@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable);
  static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue 
*queue,

  struct iio_dma_buffer_block *block)
  {
-    if (block->state == IIO_BLOCK_STATE_DEAD) {
+    if (block->state == IIO_BLOCK_STATE_DEAD)
  iio_buffer_block_put(block);
-    } else if (queue->active) {
+    else if (queue->active)
  iio_dma_buffer_submit_block(queue, block);
-    } else {
+    else
  block->state = IIO_BLOCK_STATE_QUEUED;
-    list_add_tail(>head, >incoming);
If iio_dma_buffer_enqueue() is called with a dmabuf and the 
buffer is not active, it will be marked as queued, but we 
don't actually keep a reference to it anywhere. It will 
never be submitted to the DMA, and it will never be 
signaled as completed.


We do keep a reference to the buffers, in the queue->fileio.blocks 
array. When the buffer is enabled, all the blocks in that 
array that are in the "queued" state will be submitted to the 
DMA.


But not when used in combination with the DMA buf changes later in 
this series.




That's still the case after the DMABUF changes of the series. Or can 
you point me exactly what you think is broken?


When you allocate a DMABUF with the allocate IOCTL and then submit it 
with the enqueue IOCTL before the buffer is enabled it will end up 
marked as queued, but not actually be queued anywhere.




Ok, it works for me because I never enqueue blocks before enabling the 
buffer. I can add a requirement that blocks must be enqueued only 
after the buffer is enabled.


I don't think that is a good idea. This way you are going to potentially 
drop data at the begining of your stream when the DMA isn't ready yet.

Re: [PATCH 01/15] iio: buffer-dma: Get rid of incoming/outgoing queues

[Lars-Peter Clausen]

On 11/21/21 9:08 PM, Paul Cercueil wrote:



Le dim., nov. 21 2021 at 19:49:03 +0100, Lars-Peter Clausen 
 a écrit :

On 11/21/21 6:52 PM, Paul Cercueil wrote:

Hi Lars,

Le dim., nov. 21 2021 at 17:23:35 +0100, Lars-Peter Clausen 
 a écrit :

On 11/15/21 3:19 PM, Paul Cercueil wrote:

The buffer-dma code was using two queues, incoming and outgoing, to
manage the state of the blocks in use.

While this totally works, it adds some complexity to the code,
especially since the code only manages 2 blocks. It is much easier to
just check each block's state manually, and keep a counter for the 
next

block to dequeue.

Since the new DMABUF based API wouldn't use these incoming and 
outgoing

queues anyway, getting rid of them now makes the upcoming changes
simpler.

Signed-off-by: Paul Cercueil 
The outgoing queue is going to be replaced by fences, but I think 
we need to keep the incoming queue.


Blocks are always accessed in sequential order, so we now have a 
"queue->next_dequeue" that cycles between the buffers allocated for 
fileio.



[...]
@@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable);
  static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue 
*queue,

  struct iio_dma_buffer_block *block)
  {
-    if (block->state == IIO_BLOCK_STATE_DEAD) {
+    if (block->state == IIO_BLOCK_STATE_DEAD)
  iio_buffer_block_put(block);
-    } else if (queue->active) {
+    else if (queue->active)
  iio_dma_buffer_submit_block(queue, block);
-    } else {
+    else
  block->state = IIO_BLOCK_STATE_QUEUED;
-    list_add_tail(>head, >incoming);
If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer 
is not active, it will be marked as queued, but we don't actually 
keep a reference to it anywhere. It will never be submitted to 
the DMA, and it will never be signaled as completed.


We do keep a reference to the buffers, in the queue->fileio.blocks 
array. When the buffer is enabled, all the blocks in that array 
that are in the "queued" state will be submitted to the DMA.


But not when used in combination with the DMA buf changes later in 
this series.




That's still the case after the DMABUF changes of the series. Or can 
you point me exactly what you think is broken?


When you allocate a DMABUF with the allocate IOCTL and then submit it 
with the enqueue IOCTL before the buffer is enabled it will end up 
marked as queued, but not actually be queued anywhere.

Re: [PATCH 01/15] iio: buffer-dma: Get rid of incoming/outgoing queues

[Lars-Peter Clausen]

On 11/21/21 6:52 PM, Paul Cercueil wrote:

Hi Lars,

Le dim., nov. 21 2021 at 17:23:35 +0100, Lars-Peter Clausen 
 a écrit :

On 11/15/21 3:19 PM, Paul Cercueil wrote:

The buffer-dma code was using two queues, incoming and outgoing, to
manage the state of the blocks in use.

While this totally works, it adds some complexity to the code,
especially since the code only manages 2 blocks. It is much easier to
just check each block's state manually, and keep a counter for the next
block to dequeue.

Since the new DMABUF based API wouldn't use these incoming and outgoing
queues anyway, getting rid of them now makes the upcoming changes
simpler.

Signed-off-by: Paul Cercueil 
The outgoing queue is going to be replaced by fences, but I think we 
need to keep the incoming queue.


Blocks are always accessed in sequential order, so we now have a 
"queue->next_dequeue" that cycles between the buffers allocated for 
fileio.



[...]
@@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable);
  static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue 
*queue,

  struct iio_dma_buffer_block *block)
  {
-    if (block->state == IIO_BLOCK_STATE_DEAD) {
+    if (block->state == IIO_BLOCK_STATE_DEAD)
  iio_buffer_block_put(block);
-    } else if (queue->active) {
+    else if (queue->active)
  iio_dma_buffer_submit_block(queue, block);
-    } else {
+    else
  block->state = IIO_BLOCK_STATE_QUEUED;
-    list_add_tail(>head, >incoming);
If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer is 
not active, it will be marked as queued, but we don't actually keep a 
reference to it anywhere. It will never be submitted to the DMA, and 
it will never be signaled as completed.


We do keep a reference to the buffers, in the queue->fileio.blocks 
array. When the buffer is enabled, all the blocks in that array that 
are in the "queued" state will be submitted to the DMA.


But not when used in combination with the DMA buf changes later in this 
series.

Re: [PATCH 01/15] iio: buffer-dma: Get rid of incoming/outgoing queues

[Lars-Peter Clausen]

On 11/15/21 3:19 PM, Paul Cercueil wrote:

The buffer-dma code was using two queues, incoming and outgoing, to
manage the state of the blocks in use.

While this totally works, it adds some complexity to the code,
especially since the code only manages 2 blocks. It is much easier to
just check each block's state manually, and keep a counter for the next
block to dequeue.

Since the new DMABUF based API wouldn't use these incoming and outgoing
queues anyway, getting rid of them now makes the upcoming changes
simpler.

Signed-off-by: Paul Cercueil 
The outgoing queue is going to be replaced by fences, but I think we 
need to keep the incoming queue.

[...]
@@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable);
  static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue *queue,
struct iio_dma_buffer_block *block)
  {
-   if (block->state == IIO_BLOCK_STATE_DEAD) {
+   if (block->state == IIO_BLOCK_STATE_DEAD)
iio_buffer_block_put(block);
-   } else if (queue->active) {
+   else if (queue->active)
iio_dma_buffer_submit_block(queue, block);
-   } else {
+   else
block->state = IIO_BLOCK_STATE_QUEUED;
-   list_add_tail(>head, >incoming);
If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer is 
not active, it will be marked as queued, but we don't actually keep a 
reference to it anywhere. It will never be submitted to the DMA, and it 
will never be signaled as completed.

Re: [PATCH 01/11] ASoC: dai_dma: remove slave_id field

[Lars-Peter Clausen]

On 11/15/21 11:42 AM, Arnd Bergmann wrote:

On Mon, Nov 15, 2021 at 11:14 AM Lars-Peter Clausen  wrote:

On 11/15/21 9:53 AM, Arnd Bergmann wrote:

From: Arnd Bergmann 

This field is never set, and serves no purpose, so remove it.

I agree that we should remove it. Its been legacy support code for a
while, but the description that there is no user is not right.

The tegra20_spdif driver obviously uses it and that user is removed in
this patch. I think it makes sense to split that out into a separate
patch with a description why the driver will still work even with
slave_id removed. Maybe the best is to remove the whole tegra20_spdif
driver.

Ok, I'll split out the tegra patch and try to come up with a better
description for it. What I saw in that driver is it just passes down the
slave_id number from a 'struct resource', but there is nothing in
the kernel that sets up this resource.

Do you or someone else have more information on the state of this
driver? I can see that it does not contain any of_device_id based
probing, so it seems that this is either dead code, the platform_device
gets created by some other code that is no longer compatible with
this driver.


I've looked into this a while back, when I tried to remove slave_id. And 
as far as I can tell there were never any in-tree users of this driver, 
even back when we used platform board files. Maybe somebody from Nvidia 
knows if there are out-of-tree users.


- Lars

Re: [PATCH 01/11] ASoC: dai_dma: remove slave_id field

[Lars-Peter Clausen]

On 11/15/21 9:53 AM, Arnd Bergmann wrote:

From: Arnd Bergmann 

This field is never set, and serves no purpose, so remove it.


I agree that we should remove it. Its been legacy support code for a 
while, but the description that there is no user is not right.


The tegra20_spdif driver obviously uses it and that user is removed in 
this patch. I think it makes sense to split that out into a separate 
patch with a description why the driver will still work even with 
slave_id removed. Maybe the best is to remove the whole tegra20_spdif 
driver.



diff --git a/sound/soc/tegra/tegra20_spdif.c b/sound/soc/tegra/tegra20_spdif.c
index 9fdc82d58db3..1c3385da6f82 100644
--- a/sound/soc/tegra/tegra20_spdif.c
+++ b/sound/soc/tegra/tegra20_spdif.c
@@ -284,7 +284,6 @@ static int tegra20_spdif_platform_probe(struct 
platform_device *pdev)
spdif->playback_dma_data.addr = mem->start + TEGRA20_SPDIF_DATA_OUT;
spdif->playback_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
spdif->playback_dma_data.maxburst = 4;
-   spdif->playback_dma_data.slave_id = dmareq->start;
  

dmareq is now unused and should be removed as well.

pm_runtime_enable(>dev);
  

Re: [PATCH v2 3/5] [RFT] ARM: dts: wheat: Fix ADV7513 address usage

[Lars-Peter Clausen]

On 02/12/2018 07:11 PM, Kieran Bingham wrote:
[...]
> + /*
> +  * The adv75xx resets its addresses to defaults during low power power
> +  * mode. Because we have two ADV7513 devices on the same bus, we must
> +  * change both of them away from the defaults so that they do not
> +  * conflict.
> +  */
>   hdmi@3d {
>   compatible = "adi,adv7513";
> - reg = <0x3d>;
> + reg = <0x3d 0x2d 0x4d, 0x5d>;

To have the correct semantics this should be:
reg = <0x3d>, <0x2d>, <0x4d>, <0x5d>;

It is a set of 4 single cell addresses. The other thing is a single 4 cell
address. It will get compiled to the same bytes, but the DT tools should
complain about it, because it doesn't match #address-cells.

> + reg-names = "main", "cec", "edid", "packet";
>  
>   adi,input-depth = <8>;
>   adi,input-colorspace = "rgb";
> @@ -272,7 +279,8 @@
>  
>   hdmi@39 {
>   compatible = "adi,adv7513";
> - reg = <0x39>;
> + reg = <0x39 0x29 0x49, 0x59>;

Same here.

> + reg-names = "main", "cec", "edid", "packet";
>  
>   adi,input-depth = <8>;
>   adi,input-colorspace = "rgb";
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: adv7511: Add support for i2c_new_secondary_device

[Lars-Peter Clausen]

On 01/22/2018 01:50 PM, Kieran Bingham wrote:
> The ADV7511 has four 256-byte maps that can be accessed via the main I²C
> ports. Each map has it own I²C address and acts as a standard slave
> device on the I²C bus.
> 
> Allow a device tree node to override the default addresses so that
> address conflicts with other devices on the same bus may be resolved at
> the board description level.
> 
> Signed-off-by: Kieran Bingham 

I've been working on the same thing, but you've beat me to it! Patch looks
mostly OK, but I think you are missing this piece:

https://github.com/analogdevicesinc/linux/commit/ba9b57507cb78724a606eb24104e22fea942437d#diff-2cf1828c644e351adefabe9509410400L553

> ---
>  .../bindings/display/bridge/adi,adv7511.txt| 10 +-
>  drivers/gpu/drm/bridge/adv7511/adv7511.h   |  4 +++
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   | 36 
> ++
>  3 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt 
> b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> index 0047b1394c70..f6bb9f6d3f48 100644
> --- a/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/adi,adv7511.txt
> @@ -70,6 +70,9 @@ Optional properties:
>rather than generate its own timings for HDMI output.
>  - clocks: from common clock binding: reference to the CEC clock.
>  - clock-names: from common clock binding: must be "cec".
> +- reg-names : Names of maps with programmable addresses.
> + It can contain any map needing a non-default address.
> + Possible maps names are : "main", "edid", "cec", "packet"
>  
>  Required nodes:
>  
> @@ -88,7 +91,12 @@ Example
>  
>   adv7511w: hdmi@39 {
>   compatible = "adi,adv7511w";
> - reg = <39>;
> + /*
> +  * The EDID page will be accessible on address 0x66 on the i2c
> +  * bus. All other maps continue to use their default addresses.
> +  */
> + reg = <0x39 0x66>;
> + reg-names = "main", "edid";
>   interrupt-parent = <>;
>   interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>   clocks = <_clock>;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h 
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index d034b2cb5eee..7d81ce3808e0 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -53,8 +53,10 @@
>  #define ADV7511_REG_POWER0x41
>  #define ADV7511_REG_STATUS   0x42
>  #define ADV7511_REG_EDID_I2C_ADDR0x43
> +#define ADV7511_REG_EDID_I2C_ADDR_DEFAULT0x3f
>  #define ADV7511_REG_PACKET_ENABLE1   0x44
>  #define ADV7511_REG_PACKET_I2C_ADDR  0x45
> +#define ADV7511_REG_PACKET_I2C_ADDR_DEFAULT  0x38
>  #define ADV7511_REG_DSD_ENABLE   0x46
>  #define ADV7511_REG_VIDEO_INPUT_CFG2 0x48
>  #define ADV7511_REG_INFOFRAME_UPDATE 0x4a
> @@ -89,6 +91,7 @@
>  #define ADV7511_REG_TMDS_CLOCK_INV   0xde
>  #define ADV7511_REG_ARC_CTRL 0xdf
>  #define ADV7511_REG_CEC_I2C_ADDR 0xe1
> +#define ADV7511_REG_CEC_I2C_ADDR_DEFAULT 0x3c
>  #define ADV7511_REG_CEC_CTRL 0xe2
>  #define ADV7511_REG_CHIP_ID_HIGH 0xf5
>  #define ADV7511_REG_CHIP_ID_LOW  0xf6
> @@ -322,6 +325,7 @@ struct adv7511 {
>   struct i2c_client *i2c_main;
>   struct i2c_client *i2c_edid;
>   struct i2c_client *i2c_cec;
> + struct i2c_client *i2c_packet;
>  
>   struct regmap *regmap;
>   struct regmap *regmap_cec;
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index efa29db5fc2b..7ec33837752b 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -969,8 +969,8 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
>  {
>   int ret;
>  
> - adv->i2c_cec = i2c_new_dummy(adv->i2c_main->adapter,
> -  adv->i2c_main->addr - 1);
> + adv->i2c_cec = i2c_new_secondary_device(adv->i2c_main, "cec",
> + ADV7511_REG_CEC_I2C_ADDR_DEFAULT);
>   if (!adv->i2c_cec)
>   return -ENOMEM;
>   i2c_set_clientdata(adv->i2c_cec, adv);
> @@ -1082,8 +1082,6 @@ static int adv7511_probe(struct i2c_client *i2c, const 
> struct i2c_device_id *id)
>   struct adv7511_link_config link_config;
>   struct adv7511 *adv7511;
>   struct device *dev = >dev;
> - unsigned int main_i2c_addr = i2c->addr << 1;
> - unsigned int edid_i2c_addr = main_i2c_addr + 4;
>   unsigned int val;
>   int ret;
>  
> @@ -1153,24 +1151,35 @@ static int adv7511_probe(struct i2c_client *i2c, 
> const struct 

[PATCH 1/4] drm/bridge: adv7511: Properly update EDID when no EDID was found

[Lars-Peter Clausen]

Currently adv7511_get_modes() bails out early when no EDID could be
retrieved. This leaves the previous EDID in place, which is typically not
the intended behavior and might confuse applications. Instead the EDID
should be cleared when no EDID could be retrieved.

All functions that are called after the EDID check handle the case where
the EDID is NULL just fine and exhibit the expected behavior, so just drop
the check.

Signed-off-by: Lars-Peter Clausen <l...@metafoo.de>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index b2431aee7887..fb8f4cd29e15 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -591,8 +591,6 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
 
kfree(adv7511->edid);
adv7511->edid = edid;
-   if (!edid)
-   return 0;
 
drm_mode_connector_update_edid_property(connector, edid);
count = drm_add_edid_modes(connector, edid);
-- 
2.11.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/bridge: adv7511: Constify HDMI CODEC platform data

[Lars-Peter Clausen]

The HDMI codec platform data is global driver state shared by all
instances. As such it should not be modified (and is not), to make this
explicit declare it as const.

Signed-off-by: Lars-Peter Clausen <l...@metafoo.de>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
index 67469c26bae8..1b4783d45c53 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
@@ -210,7 +210,7 @@ static const struct hdmi_codec_ops adv7511_codec_ops = {
.get_dai_id = adv7511_hdmi_i2s_get_dai_id,
 };
 
-static struct hdmi_codec_pdata codec_data = {
+static const struct hdmi_codec_pdata codec_data = {
.ops = _codec_ops,
.max_i2s_channels = 2,
.i2s = 1,
-- 
2.11.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/bridge: adv7511: Remove private copy of the EDID

[Lars-Peter Clausen]

The adv7511 driver keeps a private copy of the EDID in its driver state
struct. But this copy is only used in adv7511_get_modes() where it is also
retrieved, so there is no need to keep this extra copy around.

If a need to access the EDID elsewhere in the driver ever arises the copy
that is stored in the connector can be used. This copy is accessible
through drm_connector_get_edid().

Note, this patch removes the NULL check of the EDID before passing it to
drm_detect_hdmi_monitor(), but that is fine since the function correctly
handles the case where the EDID is NULL.

Signed-off-by: Lars-Peter Clausen <l...@metafoo.de>
---
 drivers/gpu/drm/bridge/adv7511/adv7511.h |  2 --
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 16 ++--
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h 
b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index fe18a5d2d84b..12ef2d8ee110 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -328,8 +328,6 @@ struct adv7511 {
enum adv7511_sync_polarity hsync_polarity;
bool rgb;
 
-   struct edid *edid;
-
struct gpio_desc *gpio_pd;
 
struct regulator_bulk_data *supplies;
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index fb8f4cd29e15..94d598d8aedf 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -199,17 +199,14 @@ static const uint16_t adv7511_csc_ycbcr_to_rgb[] = {
 
 static void adv7511_set_config_csc(struct adv7511 *adv7511,
   struct drm_connector *connector,
-  bool rgb)
+  bool rgb, bool hdmi_mode)
 {
struct adv7511_video_config config;
bool output_format_422, output_format_ycbcr;
unsigned int mode;
uint8_t infoframe[17];
 
-   if (adv7511->edid)
-   config.hdmi_mode = drm_detect_hdmi_monitor(adv7511->edid);
-   else
-   config.hdmi_mode = false;
+   config.hdmi_mode = hdmi_mode;
 
hdmi_avi_infoframe_init(_infoframe);
 
@@ -589,13 +586,14 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
if (!adv7511->powered)
__adv7511_power_off(adv7511);
 
-   kfree(adv7511->edid);
-   adv7511->edid = edid;
 
drm_mode_connector_update_edid_property(connector, edid);
count = drm_add_edid_modes(connector, edid);
 
-   adv7511_set_config_csc(adv7511, connector, adv7511->rgb);
+   adv7511_set_config_csc(adv7511, connector, adv7511->rgb,
+  drm_detect_hdmi_monitor(edid));
+
+   kfree(edid);
 
return count;
 }
@@ -1156,8 +1154,6 @@ static int adv7511_remove(struct i2c_client *i2c)
 
i2c_unregister_device(adv7511->i2c_edid);
 
-   kfree(adv7511->edid);
-
return 0;
 }
 
-- 
2.11.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/bridge: adv7511: Enable connector polling when no interrupt is specified

[Lars-Peter Clausen]

Fall back to polling the connector for connect and disconnect events when
no interrupt is specified. Otherwise these events will not be noticed and
monitor hotplug does not work.

Signed-off-by: Lars-Peter Clausen <l...@metafoo.de>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 94d598d8aedf..bd7dbae1119e 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -829,7 +829,11 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
return -ENODEV;
}
 
-   adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
+   if (adv->i2c_main->irq)
+   adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
+   else
+   adv->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
+   DRM_CONNECTOR_POLL_DISCONNECT;
 
ret = drm_connector_init(bridge->dev, >connector,
 _connector_funcs,
-- 
2.11.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2 v3] drm/bridge: adv7511: Add Audio support.

[Lars-Peter Clausen]

On 09/07/2016 01:22 AM, John Stultz wrote:
> This patch adds support to Audio for both adv7511 and adv7533
> bridge chips.
> 
> This patch was originally from [1] by Lars-Peter Clausen 
> and was adapted by Archit Taneja  and
> Srinivas Kandagatla .
> 
> Then I heavily reworked it to use the hdmi-codec driver. And also
> folded in some audio packet initialization done by Andy Green
> . So credit to them, but blame to me.
> 
> [1] 
> https://github.com/analogdevicesinc/linux/blob/xcomm_zynq/drivers/gpu/drm/i2c/adv7511_audio.c
> 
> Cc: David Airlie 
> Cc: Archit Taneja 
> Cc: Laurent Pinchart 
> Cc: Wolfram Sang <wsa+renesas at sang-engineering.com>
> Cc: Srinivas Kandagatla 
> Cc: "Ville Syrjälä" 
> Cc: Boris Brezillon 
> Cc: Andy Green 
> Cc: Dave Long 
> Cc: Guodong Xu 
> Cc: Zhangfei Gao 
> Cc: Mark Brown 
> Cc: Lars-Peter Clausen 
> Cc: Jose Abreu 
> Cc: dri-devel at lists.freedesktop.org
> Signed-off-by: John Stultz 

I'm not to fond of the hdmi-codec stuff, but within its context this looks
ok. Thanks for getting this upstream ready. Just one tiny bit regarding the
Kconfig entry.

If that is fixed feel free to add on the next revision:

Acked-by: Lars-Peter Clausen 

> ---
> v3:
> * Allowed audio support to be configured in or out, as suggested by Laurent
> * Minor cleanups suggested by Laurent
> * Folded in Andy's audio packet initialization patch, as suggested by Archit
> 
>  drivers/gpu/drm/bridge/adv7511/Kconfig |   9 ++
>  drivers/gpu/drm/bridge/adv7511/Makefile|   1 +
>  drivers/gpu/drm/bridge/adv7511/adv7511.h   |  16 ++
>  drivers/gpu/drm/bridge/adv7511/adv7511_audio.c | 213 
> +
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c   |   4 +
>  5 files changed, 243 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/adv7511/adv7511_audio.c
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/Kconfig 
> b/drivers/gpu/drm/bridge/adv7511/Kconfig
> index d2b0499..b303ad1 100644
> --- a/drivers/gpu/drm/bridge/adv7511/Kconfig
> +++ b/drivers/gpu/drm/bridge/adv7511/Kconfig
> @@ -3,9 +3,18 @@ config DRM_I2C_ADV7511
>   depends on OF
>   select DRM_KMS_HELPER
>   select REGMAP_I2C
> + select SND_SOC_HDMI_CODEC if SND_SOC

This seems to be redundant considering the select done by DRM_I2C_ADV7511_AUDIO.

>   help
> Support for the Analog Device ADV7511(W) and ADV7513 HDMI encoders.
>  
> +config DRM_I2C_ADV7511_AUDIO
> + tristate "ADV7511 HDMI Audio driver"

This should be bool. It either gets built into the ADV7511 driver (which
itself could either be a module or built-in) or not. But setting this option
itself to module wont work.

> + depends on DRM_I2C_ADV7511 && SND_SOC
> + select SND_SOC_HDMI_CODEC
> + help
> +   Support the ADV7511 HDMI Audio interface. This is used in
> +   conjunction with the AV7511  HDMI driver.
> +

[...]
> +static void audio_shutdown(struct device *dev, void *data)
> +{
> +}

Unrelated to this patch, but it looks like the shutdown callback should
maybe be made optional.

> +static const struct hdmi_codec_ops adv7511_codec_ops = {
> + .hw_params  = adv7511_hdmi_hw_params,
> + .audio_shutdown = audio_shutdown,
> + .audio_startup  = audio_startup,
> +};

[alsa-devel] [PATCH 2/5 v4] drm/i2c/adv7511: Add audio support

[Lars-Peter Clausen]

On 04/11/2016 04:08 PM, Jose Abreu wrote:
[...]
>> Currently there is also zero support of of-graph in ASoC, so a bit of work
>> is required to get this integrated properly.
>>
> 
> I also believe this would be the better option but in the meantime can't I
> integrate the audio like it is being done in the dw-hdmi driver[1]? In this
> driver the audio is registered as a sound card and is conditionally built 
> using
> Kconfig, just like I was doing in the previous versions. I know you said the
> HDMI audio is still an open issue but it seems that for this case it was 
> accepted.
> 
> [1] 
> http://lxr.free-electrons.com/source/drivers/gpu/drm/bridge/dw-hdmi-ahb-audio.c

Maybe, but I'm not sure it would work in this case. Resources are probably
better invested in working towards a proper solution.

[alsa-devel] [PATCH 2/5 v4] drm/i2c/adv7511: Add audio support

[Lars-Peter Clausen]

On 04/11/2016 01:32 PM, Jose Abreu wrote:
> Hi Lars,
> 
> 
> On 11-04-2016 10:33, Lars-Peter Clausen wrote:
>> On 04/11/2016 11:27 AM, Jose Abreu wrote:
>>> Hi Lars,
>>>
>>>
>>> On 09-04-2016 16:02, Lars-Peter Clausen wrote:
>>>> On 04/08/2016 06:12 PM, Jose Abreu wrote:
>>>> [...]
>>>>>> [...]
>>>>>>> +- adi,enable-audio: If set the ADV7511 driver will register a codec 
>>>>>>> interface
>>>>>>> +  into ALSA SoC.
>>>>>> This is not a description of the hardware.
>>>>> Is this okay: "adi,enable-audio: Set this boolean parameter if ADV7511
>>>>> transmitter routes audio signals" ?
>>>> I don't think we need this property. There is no problem with registering
>>>> the audio part unconditionally. As long as there is no connection we wont
>>>> create a sound card that is exposed to userspace.
>>>>
>>> This change was suggested by Laurent Pinchart and was introduced in v3. 
>>> Quoting
>>> Laurent:
>>> "The idea is that enabling support for ADV7511 audio in the kernel isn't 
>>> coupled
>>> with whether the system includes audio support. It would be confusing, and 
>>> would
>>> also waste resources, to create a Linux sound device when no sound channel 
>>> is
>>> routed on the board."
>> I wouldn't care too much about this at this point, the extra amount of
>> resources required for registering the CODEC (but not the sound card) is
>> just a few bytes (sizeof(struct snd_soc_codec)).
>>
>> Nevertheless what we should do is describe the hardware and from this
>> information infer whether there is a audio connection or not and if there is
>> none we might skip registering the CODEC. In my opinion this hardware
>> description should be modeled using of-graph, having a connection between
>> the SoC side and the adv7511 SPDIF or I2S port.
>>
> 
> You mean something like this:
> 
> sound_playback: sound_playback {
> compatible = "simple-audio-card";
> [...]
> simple-audio-card,format = "i2s";
> [...]
> }
> 
> adv7511 at xx {
> compatible = "adi,adv7511";
> [...]
> 
> ports {
> [...]
> /* Audio Output */
> port at x {
> reg = ;
> endpoint {
> remote-endpoint = <_playback>;
> }
> }
> }
> }

Yes, something like that. Not exactly like that, but similar. One of the
core concepts of of-graph is that there is always a description of the
connection from both sides, this way each side can independently figure out
where it is connected.

Currently there is also zero support of of-graph in ASoC, so a bit of work
is required to get this integrated properly.

[alsa-devel] [PATCH 2/5 v4] drm/i2c/adv7511: Add audio support

[Lars-Peter Clausen]

On 04/11/2016 11:27 AM, Jose Abreu wrote:
> Hi Lars,
> 
> 
> On 09-04-2016 16:02, Lars-Peter Clausen wrote:
>> On 04/08/2016 06:12 PM, Jose Abreu wrote:
>> [...]
>>>> [...]
>>>>> +- adi,enable-audio: If set the ADV7511 driver will register a codec 
>>>>> interface
>>>>> +  into ALSA SoC.
>>>> This is not a description of the hardware.
>>> Is this okay: "adi,enable-audio: Set this boolean parameter if ADV7511
>>> transmitter routes audio signals" ?
>> I don't think we need this property. There is no problem with registering
>> the audio part unconditionally. As long as there is no connection we wont
>> create a sound card that is exposed to userspace.
>>
> 
> This change was suggested by Laurent Pinchart and was introduced in v3. 
> Quoting
> Laurent:
> "The idea is that enabling support for ADV7511 audio in the kernel isn't 
> coupled
> with whether the system includes audio support. It would be confusing, and 
> would
> also waste resources, to create a Linux sound device when no sound channel is
> routed on the board."

I wouldn't care too much about this at this point, the extra amount of
resources required for registering the CODEC (but not the sound card) is
just a few bytes (sizeof(struct snd_soc_codec)).

Nevertheless what we should do is describe the hardware and from this
information infer whether there is a audio connection or not and if there is
none we might skip registering the CODEC. In my opinion this hardware
description should be modeled using of-graph, having a connection between
the SoC side and the adv7511 SPDIF or I2S port.

[alsa-devel] [PATCH 2/5 v4] drm/i2c/adv7511: Add audio support

[Lars-Peter Clausen]

On 04/08/2016 06:12 PM, Jose Abreu wrote:
[...]
>>
>> [...]
>>> +- adi,enable-audio: If set the ADV7511 driver will register a codec 
>>> interface
>>> +  into ALSA SoC.
>> This is not a description of the hardware.
> 
> Is this okay: "adi,enable-audio: Set this boolean parameter if ADV7511
> transmitter routes audio signals" ?

I don't think we need this property. There is no problem with registering
the audio part unconditionally. As long as there is no connection we wont
create a sound card that is exposed to userspace.

[alsa-devel] [PATCH 5/5 v4] ASoC: dwc: Update DOCUMENTATION for I2S Driver

[Lars-Peter Clausen]

On 04/08/2016 06:08 PM, Jose Abreu wrote:
> Hi Lars,
> 
> 
> On 08-04-2016 16:52, Lars-Peter Clausen wrote:
>> On 04/08/2016 12:06 PM, Jose Abreu wrote:
>>> Hi Mark,
>>>
>>>
>>> On 07-04-2016 18:53, Mark Brown wrote:
>>>> On Thu, Apr 07, 2016 at 05:53:59PM +0100, Jose Abreu wrote:
>>>>
>>>>> + Optional properties:
>>>>> + - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
>>>>> +   it is required to use the properties 'dmas' and 'dma-names'.
>>>> This is not a good interface, it's describing Linux internal APIs.  If
>>>> the device needs to operate in PIO mode it should just do that.
>>> I added this interface because there is no direct way to check if DMA is
>>> available on the I2S controller so it is not possible to automatically 
>>> change
>>> between DMA and PIO mode. As the I2S controller can be built with or 
>>> without DMA
>>> support it is necessary to somehow check if DMA is enabled or not and 
>>> according
>>> to that use either ALSA DMA engine or the custom platform driver sent in 
>>> these
>>> patches. I did not want to remove drivers functionality so I added this 
>>> property
>>> to the DT. This way a user can select between DMA and PIO mode.
>> That's OK, but you need to describe the hardware, not the indented behavior
>> of the software driver.
>>
> 
> Is this okay: "snps,use-dmaengine: Set this boolean paramater if I2S 
> controller
> has DMA support. If set the properties 'dmas' and 'dma-names' must be also 
> set" ?

The description is better. But the name of the property is still imperative
rather then descriptive. It tells the software what should be done rather
then describing what the hardware looks like.

Since there is already the dmas property which is present if a DMA is
connected and is absent when no DMA is present it should be enough to just
check that property rather than requiring an additional one.

[alsa-devel] [PATCH 5/5 v4] ASoC: dwc: Update DOCUMENTATION for I2S Driver

[Lars-Peter Clausen]

On 04/08/2016 12:06 PM, Jose Abreu wrote:
> Hi Mark,
> 
> 
> On 07-04-2016 18:53, Mark Brown wrote:
>> On Thu, Apr 07, 2016 at 05:53:59PM +0100, Jose Abreu wrote:
>>
>>> + Optional properties:
>>> + - snps,use-dmaengine: If set the driver will use ALSA DMA engine. If set
>>> +   it is required to use the properties 'dmas' and 'dma-names'.
>> This is not a good interface, it's describing Linux internal APIs.  If
>> the device needs to operate in PIO mode it should just do that.
> 
> I added this interface because there is no direct way to check if DMA is
> available on the I2S controller so it is not possible to automatically change
> between DMA and PIO mode. As the I2S controller can be built with or without 
> DMA
> support it is necessary to somehow check if DMA is enabled or not and 
> according
> to that use either ALSA DMA engine or the custom platform driver sent in these
> patches. I did not want to remove drivers functionality so I added this 
> property
> to the DT. This way a user can select between DMA and PIO mode.

That's OK, but you need to describe the hardware, not the indented behavior
of the software driver.

[alsa-devel] [PATCH 2/5 v4] drm/i2c/adv7511: Add audio support

[Lars-Peter Clausen]

On 04/07/2016 06:53 PM, Jose Abreu wrote:
> This patch adds audio support for the ADV7511 HDMI transmitter
> using ALSA SoC.
> 
> The code was ported from Analog Devices linux tree from
> commit 1770c4a1e32b ("Merge remote-tracking branch
> 'xilinx/master' into xcomm_zynq"), which is available at:
>   - https://github.com/analogdevicesinc/linux/

The reason why this driver is still out of tree, is because there has been
no conclusion yet on how to go forward with the whole HDMI integration. So
this is not going to get merged until that issue has been addressed.

[...]
> +- adi,enable-audio: If set the ADV7511 driver will register a codec interface
> +  into ALSA SoC.

This is not a description of the hardware.

[...]
> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
> index 964b7de..539c091 100644
> --- a/include/sound/soc-dai.h
> +++ b/include/sound/soc-dai.h
> @@ -33,6 +33,7 @@ struct snd_compr_stream;
>  #define SND_SOC_DAIFMT_DSP_B 5 /* L data MSB during FRM LRC */
>  #define SND_SOC_DAIFMT_AC97  6 /* AC97 */
>  #define SND_SOC_DAIFMT_PDM   7 /* Pulse density modulation */
> +#define SND_SOC_DAIFMT_SPDIF 8 /* SPDIF */

This needs to be a separate patch.

[PATCH] adv7511: Added mode_fixup function.

[Lars-Peter Clausen]

On 02/04/2016 04:22 PM, Carlos Palminha wrote:
> Hi guys,
> 
> any feedback? patch will be accepted for adv7511 driver?

Hi,

Thanks for the patch, but please try to find and fix the call site that is
trying to invoke the callback even though it does not exist.

This is most likely drm_i2c_encoder_mode_fixup().

- Lars

> 
> Regards,
> C.Palminha
> 
> On 01-02-2016 12:37, Carlos Palminha wrote:
>> Hi Laurent
>>
>> On 29-01-2016 17:48, Laurent Pinchart wrote:
>>> Hi Carlos,
>>>
>>> Thank you for the patch.
>>>
>>> On Friday 29 January 2016 10:33:47 Carlos Palminha wrote:
 The mode_fixup is necessary when using it in a DRM FB driver pipeline.
>>>
>>> Instead of implementing stubs in encoder drivers, wouldn't it be better to 
>>> make mode_fixup optional ?
>> Probably you are right but i don't have enough knowledge or time to do that 
>> for the DRM framework. :(
>> I limited myself to do what the other drivers already implement.
>>
>> The patch is mandatory to have to the ADV working or else will get some NULL 
>> pointer crash.
>>
>> Regards,
>> C.Palminha
>>
>>>
 Signed-off-by: Carlos Palminha 
 ---
  drivers/gpu/drm/i2c/adv7511.c | 8 
  1 file changed, 8 insertions(+)

 diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
 index 533d1e3..90082d2 100644
 --- a/drivers/gpu/drm/i2c/adv7511.c
 +++ b/drivers/gpu/drm/i2c/adv7511.c
 @@ -648,6 +648,13 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
return status;
  }

 +static bool adv7511_encoder_mode_fixup(struct drm_encoder *encoder,
 +  const struct drm_display_mode *mode,
 +  struct drm_display_mode *adjusted_mode)
 +{
 +  return true;
 +}
 +
  static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
  struct drm_display_mode *mode)
  {
 @@ -754,6 +761,7 @@ static void adv7511_encoder_mode_set(struct drm_encoder
 *encoder,

  static const struct drm_encoder_slave_funcs adv7511_encoder_funcs = {
.dpms = adv7511_encoder_dpms,
 +  .mode_fixup = adv7511_encoder_mode_fixup,
.mode_valid = adv7511_encoder_mode_valid,
.mode_set = adv7511_encoder_mode_set,
.detect = adv7511_encoder_detect,
>>>
>>
>>
>> On 29-01-2016 17:48, Laurent Pinchart wrote:
>>> Hi Carlos,
>>>
>>> Thank you for the patch.
>>>
>>> On Friday 29 January 2016 10:33:47 Carlos Palminha wrote:
 The mode_fixup is necessary when using it in a DRM FB driver pipeline.
>>>
>>> Instead of implementing stubs in encoder drivers, wouldn't it be better to 
>>> make mode_fixup optional ?
>>>
 Signed-off-by: Carlos Palminha 
 ---
  drivers/gpu/drm/i2c/adv7511.c | 8 
  1 file changed, 8 insertions(+)

 diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
 index 533d1e3..90082d2 100644
 --- a/drivers/gpu/drm/i2c/adv7511.c
 +++ b/drivers/gpu/drm/i2c/adv7511.c
 @@ -648,6 +648,13 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
return status;
  }

 +static bool adv7511_encoder_mode_fixup(struct drm_encoder *encoder,
 +  const struct drm_display_mode *mode,
 +  struct drm_display_mode *adjusted_mode)
 +{
 +  return true;
 +}
 +
  static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
  struct drm_display_mode *mode)
  {
 @@ -754,6 +761,7 @@ static void adv7511_encoder_mode_set(struct drm_encoder
 *encoder,

  static const struct drm_encoder_slave_funcs adv7511_encoder_funcs = {
.dpms = adv7511_encoder_dpms,
 +  .mode_fixup = adv7511_encoder_mode_fixup,
.mode_valid = adv7511_encoder_mode_valid,
.mode_set = adv7511_encoder_mode_set,
.detect = adv7511_encoder_detect,
>>>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

[PATCH RESEND 3/3] drm: adv7511: it's HPD, not HDP

[Lars-Peter Clausen]

On 01/04/2016 03:33 AM, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas at sang-engineering.com>
> 
> Fix this typo, consequently used over both files :)
> 
> Signed-off-by: Wolfram Sang <wsa+renesas at sang-engineering.com>

Acked-by: Lars-Peter Clausen 

Thanks.

[PATCH] drm: adv7511: really enable interrupts for EDID detection

[Lars-Peter Clausen]

On 11/25/2015 09:27 AM, Wolfram Sang wrote:
> 
>> I guess you mean that the GPIO callbacks include Runtime PM handling
>> however for irq_chip Runtime PM may not be hooked up so the GPIO block
>> is in such case is not powered on / get clock enabled?
> 
> Yes. There is another drawback when GPIOs are not properly requested. It
> is still possible to request them from userspace although a kernel
> driver is using them. I am playing with the idea that the GPIO core
> auto-requests GPIOs which are not already requested but still set up as
> interrupts.

I think the GPIO core already reserves the pins that are requested as IRQs.
See gpiochip_lock_as_irq().

As for PM see this discussion
http://lkml.iu.edu/hypermail/linux/kernel/1511.1/01645.html

- Lars


-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 

HDMI codec, way forward?

[Lars-Peter Clausen]

On 10/16/2015 01:50 PM, Jyri Sarha wrote:
> After reading the ELCE Audio mini conf minutes [1] I gather that HDMI
> audio was not discussed after all.

It was discussed, but rather shortly and only in the context of the HDA.
(Adding Vinod to Cc, who presented yet another approach to the problem last
week)

> 
> My conclusion from the Lars-Peter's latest mail [2] related to the
> subject is that the wind is currently blowing slightly in favour of my
> version of the hdmi codec [3], or at least not in favour of Arnaud's
> version [4].
> 
> So how should we proceed? Are we still waiting for some feedback from
> DRM/video side?
>

What needs to happen is that everybody who is working on HDMI audio support
should get their heads together and come up with a common solution that
works for everybody, rather than having everybody trying to push their own
solution and put the maintainer in the spotlight of having to choose one
(Mark has been asking for this for the past half year or so).

- Lars

[alsa-devel] [PATCH RFC V2 0/5] another generic audio hdmi codec proposal

[Lars-Peter Clausen]

Added Hans, who's working a lot on the HDMI transmitter drivers (including
audio support) as well as the media list to Cc.

On 10/07/2015 10:19 AM, Arnaud Pouliquen wrote:
> 
> 
>>> My approach is the reverse: DRM driver does not need to know anything
>>> about audio side. As ALSA is the client of DRM, seems more logical from
>>> my point of view ...
>>> Now if a generic solution must be found for all video drivers, sure,
>>> your solution is more flexible.
>>> But if i well understood fbdev drivers are no more accepted for upstream
>>> (please correct me if I'm wrong).
>>> So i don't know we have to keep fbdev in picture...
>>>
>>
>> I am not promoting fbdev support. I am merely asking if we want to force
>> all HDMI drivers to implement a drm_bridge if they want to support audio.
>>
> Yes this is a good point... My implementation is based on hypothesis that
> HDMI drivers are now upstreamed as DRM drivers.

The other place where you can find HDMI support is in V4L2, both receive as
well as transmit. And while the hope for fbdev is that it will be phased out
V4L2 will stay around for a while. And we probably want to have a common API
that can take care of both DRM and V4L so we do not need two sets of helper
functions for things like EDID parsing etc.

- Lars

[alsa-devel] [PATCH 01/12] drm/amdgpu: add amd_gnb_bus support

[Lars-Peter Clausen]

> @@ -134,6 +136,7 @@ config DRM_AMDGPU
>   select HWMON
>   select BACKLIGHT_CLASS_DEVICE
>   select INTERVAL_TREE
> + select DRM_AMD_GNB_BUS

Here you select the symbol.

[...]

> +config DRM_AMD_GNB_BUS
> + tristate "AMD GNB bus - used for GNB IPs such as ACP and ISP"

Here you make it user selectable. Use either or having both doesn't work too
well.

> +
> +endmenu
> diff --git a/drivers/gpu/drm/amd/bus/Makefile 
> b/drivers/gpu/drm/amd/bus/Makefile
> new file mode 100644
> index 000..c41ffc9
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/bus/Makefile
> @@ -0,0 +1,4 @@
> +#
> +ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/amd/include/bus/
> +
> +obj-$(CONFIG_DRM_AMD_GNB_BUS) := amd_gnb_bus.o
> diff --git a/drivers/gpu/drm/amd/bus/amd_gnb_bus.c 
> b/drivers/gpu/drm/amd/bus/amd_gnb_bus.c
> new file mode 100644
> index 000..071b16c
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/bus/amd_gnb_bus.c
> @@ -0,0 +1,266 @@
[..]
> +#ifdef CONFIG_PM_SLEEP
> +static int amd_gnb_bus_legacy_suspend(struct device *dev, pm_message_t mesg)
> +{
> + struct amd_gnb_bus_dev *amd_gnb_bus_dev = to_amd_gnb_bus_device(dev);
> + struct amd_gnb_bus_driver *driver;
> +
> + if (!amd_gnb_bus_dev || !dev->driver)
> + return 0;
> + driver = to_amd_gnb_bus_driver(dev->driver);
> + if (!driver->suspend)
> + return 0;
> + return driver->suspend(amd_gnb_bus_dev, mesg);
> +}
> +
> +static int amd_gnb_bus_legacy_resume(struct device *dev)
> +{
> + struct amd_gnb_bus_dev *amd_gnb_bus_dev = to_amd_gnb_bus_device(dev);
> + struct amd_gnb_bus_driver *driver;
> +
> + if (!amd_gnb_bus_dev || !dev->driver)
> + return 0;
> + driver = to_amd_gnb_bus_driver(dev->driver);
> + if (!driver->resume)
> + return 0;
> + return driver->resume(amd_gnb_bus_dev);
> +}
[...]


Preferably don't add support for legacy suspend/resume to new subsystems.
Support for this is supposed to be removed from the kernel. Just use
dev_pm_ops and then you can drop all of the above since the PM core does the
right thing on its own for dev_pm_ops. No need to have support at the bus
level if there is nothing special to do at the bus level.

[...]

[PATCH] drm: adv7511: Fix crash in IRQ handler when no encoder is associated

[Lars-Peter Clausen]

On 05/14/2015 02:29 PM, Laurent Pinchart wrote:
> The ADV7511 is probed before its slave encoder init function associates
> it with an encoder. This creates a time window during which hot plug
> detection interrupts can occur with an encoder, resulting in a crash in
> the IRQ handler.
>
> Fix this by ignoring hot plug detection IRQs when no encoder is
> associated yet.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas at ideasonboard.com>

Acked-by: Lars-Peter Clausen 

> ---
>   drivers/gpu/drm/i2c/adv7511.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
> index b728523e194f..2aaa3c88999e 100644
> --- a/drivers/gpu/drm/i2c/adv7511.c
> +++ b/drivers/gpu/drm/i2c/adv7511.c
> @@ -438,7 +438,7 @@ static int adv7511_irq_process(struct adv7511 *adv7511)
>   regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
>   regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
>
> - if (irq0 & ADV7511_INT0_HDP)
> + if (irq0 & ADV7511_INT0_HDP && adv7511->encoder)
>   drm_helper_hpd_irq_event(adv7511->encoder->dev);
>
>   if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
>

[alsa-devel] [PATCH RFC v2 10/13] sound/core: add DRM ELD helper

[Lars-Peter Clausen]

On 05/07/2015 12:41 PM, Russell King - ARM Linux wrote:
[...]
>>> What I'm concerned about is that when the ALSA parameter refining
>>> starts, we start with (eg) 2-8 channels, 32-192kHz.  Given that,
>>> if we invoke the channel restriction before the rate restriction,
>>> we would end up limiting to 2 channel at 32-192kHz.  If we apply
>>> the restrictions in the opposite order, we'd restrict to 6
>>> channel, 32-48kHz.  Neither are obviously correct in this
>>> circumstance, and I don't really see a way to solve it given my
>>> understanding of the way ALSA's parameter refinement works.
>>>
>>> I suspect this is why most HDMI drivers are implemented such that
>>> they take the maximum capabilities over all SADs, which would end
>>> up restricting audio in the above case to: up to 6 channels, at
>>> 32, 44.1, 48, 96 and 192kHz, even though 6 channel @ 192kHz isn't
>>> hasn't been indicated as supported.
>>>
>>> Most of this is speculation though, based off what is in the
>>> documentation.  As I say, I don't have enough real-world examples
>>> to get a feel for what manufacturers _actually_ do to give a hint
>>> as to how the documentation should be interpreted.
>
> ... so this is probably less than speculation.
>
> So, ALSA gurus, how do we handle this?  How do we arrange the parameter
> resolution such that ALSA can permit _either_ 2 channels at 192kHz or 6
> channels at 48kHz, but not 6 channels at 192kHz?  Right now, I don't
> see how that's possible.

That's pretty straight forward and can be done using custom rules linking 
the number of channels to the sample rate. Have a look at 
snd_ac97_pcm_double_rate_rules() this is pretty much the same constraint.

- Lars

[PATCH] drm: adv7511: Refactor power management

[Lars-Peter Clausen]

On 03/02/2015 02:19 PM, Laurent Pinchart wrote:
> From: Laurent Pinchart 
>
> Remove the internal dependency on DPMS mode for power management by
> using a by a powered state boolean instead, and use the new power off
> handler at probe time. This ensure that the regmap cache is properly
> marked as dirty when the device is probed, and the registers properly
> synced during the first power up.
>
> As a side effect this removes the initialization of current_edid_segment
> at probe time, as the field will be initialized when the device is
> powered on, at the latest right before reading EDID data.
>
> Signed-off-by: Laurent Pinchart 

Tested-by: Lars-Peter Clausen 
Acked-by: Lars-Peter Clausen 

Thanks.

[PATCH 5/6] dt-bindings: Add documentation for Rockchip hdmi-audio

[Lars-Peter Clausen]

On 12/15/2014 03:55 AM, Yakir Yang wrote:
[...]
> +- codec-name: the dw-hdmi codec's device name
> +- codec-dai-name: the dw-hdmi codec's dai name

Please you phandles for this, the names are Linux driver and framework 
specific details. That should not leak into the DT bindings.

[..]

[GIT PULL FOR v3.19] R-Car DU changes

[Lars-Peter Clausen]

On 11/24/2014 09:01 PM, Dave Airlie wrote:
> On 25 November 2014 at 00:18, Lars-Peter Clausen  wrote:
>> On 11/24/2014 03:00 PM, Laurent Pinchart wrote:
>>>
>>> Hi Daniel,
>>>
>>> (CC'ing Rob Clark and Lars-Peter. As a reminder we're discussing the "drm:
>>> Decouple EDID parsing from I2C adapter" patch available at
>>> git://linuxtv.org/pinchartl/fbdev.git drm/next/du)
>>>
>>> On Monday 24 November 2014 14:09:39 Daniel Vetter wrote:
>>>>
>>>> On Mon, Nov 24, 2014 at 11:46:18AM +0200, Laurent Pinchart wrote:
>>>>>>
>>>>>> - the interface looks rather backwards: Either this still does i2c
>>>>>> reads, and then you'd just need a i2c-over-whatever adapter to make
>>>>>> it
>>>>>> work. Or you have other magic means to optain an edid block, in
>>>>>> which
>>>>>> case just do that and then feed the edid drm_add_edid_modes.
>>>>>
>>>>>
>>>>> I have a magic way to get EDID over I2C :-) Basically the ADV7511
>>>>> controls
>>>>> the DDC bus, and exposes EDID data over I2C using vendor commands. To
>>>>> read an EDID block I have to write an ADV7511 register over I2C with the
>>>>> block number, wait for an interrupt, read a status register to check
>>>>> whether EDID data is available or whether an error occurred, and then
>>>>> read EDID data from the ADV7511 over I2C in 64-bytes chunks. This needs
>>>>> to be repeated for every block. I thus can't use drm_get_edid()
>>>>> directly.
>>>>
>>>>
>>>> Sounds familiar. See the special ddc-over-sdvo i2c bus we register in
>>>> intel_sdvo.c, specifically look at intel_sdvo_init_ddc_proxy. It is a bit
>>>> of boilerplate, but in the end just amounts to 3 small functions and one
>>>> tiny vtable to wire it all up cleanly.
>>>
>>>
>>> That's what I would have done as well if I had a device-specific I2C
>>> adapter
>>> connected to the DDC bus, but in this case the interface exposed by the
>>> ADV7511 to the SoC over I2C consists of higher level device-specific I2C
>>> commands to read EDID data. There is no low-level I2C read/write
>>> primitives
>>> available. I would thus need to expose a fake adapter that would receive
>>> I2C
>>> commands, parse them to detect an EDID block read, retrieve the EDID data
>>> and
>>> return them from the fake read. That doesn't make much sense to me.
>>
>>
>> The intel sdvo looks just like a simple I2C mux which will just pass-through
>> messages from the master to the EDID EEPROM. The ADV7511 is unfortunately a
>> bit different. You tell it to fetch the EDID information, then it will do
>> some magic and then you can read the EDID back. Abstracting this as a this
>> as a I2C controller will, while possible, result in a fair amount of boiler
>> plate code that will not look particularly pretty.
>
> It sounds also a bit like DP auxch also, or even how on UDL we get the edid
> over USB.
>
> I'd rather see not pretty code that only one person had to look at though :-)
> with lots of comments on the hw design that demands ugly.

I'd rather not see that if I'm the person who has to look at it ;)

But looking around it appears as if this is a more common thing among 
external HDMI encoders though. E.g. the tda998x has the exact same issue and 
the current driver just copy drm_do_get_edid() and plugs in its own 
EDID block retrieval method.

On the other hand the patch that makes it possible to use a different 
backend then a raw I2C adapter to fetch the EDID is extremely trivial and 
doesn't make the existing code more complex in any way.

[GIT PULL FOR v3.19] R-Car DU changes

[Lars-Peter Clausen]

On 11/24/2014 03:00 PM, Laurent Pinchart wrote:
> Hi Daniel,
>
> (CC'ing Rob Clark and Lars-Peter. As a reminder we're discussing the "drm:
> Decouple EDID parsing from I2C adapter" patch available at
> git://linuxtv.org/pinchartl/fbdev.git drm/next/du)
>
> On Monday 24 November 2014 14:09:39 Daniel Vetter wrote:
>> On Mon, Nov 24, 2014 at 11:46:18AM +0200, Laurent Pinchart wrote:
 - the interface looks rather backwards: Either this still does i2c
reads, and then you'd just need a i2c-over-whatever adapter to make it
work. Or you have other magic means to optain an edid block, in which
case just do that and then feed the edid drm_add_edid_modes.
>>>
>>> I have a magic way to get EDID over I2C :-) Basically the ADV7511 controls
>>> the DDC bus, and exposes EDID data over I2C using vendor commands. To
>>> read an EDID block I have to write an ADV7511 register over I2C with the
>>> block number, wait for an interrupt, read a status register to check
>>> whether EDID data is available or whether an error occurred, and then
>>> read EDID data from the ADV7511 over I2C in 64-bytes chunks. This needs
>>> to be repeated for every block. I thus can't use drm_get_edid() directly.
>>
>> Sounds familiar. See the special ddc-over-sdvo i2c bus we register in
>> intel_sdvo.c, specifically look at intel_sdvo_init_ddc_proxy. It is a bit
>> of boilerplate, but in the end just amounts to 3 small functions and one
>> tiny vtable to wire it all up cleanly.
>
> That's what I would have done as well if I had a device-specific I2C adapter
> connected to the DDC bus, but in this case the interface exposed by the
> ADV7511 to the SoC over I2C consists of higher level device-specific I2C
> commands to read EDID data. There is no low-level I2C read/write primitives
> available. I would thus need to expose a fake adapter that would receive I2C
> commands, parse them to detect an EDID block read, retrieve the EDID data and
> return them from the fake read. That doesn't make much sense to me.

The intel sdvo looks just like a simple I2C mux which will just pass-through 
messages from the master to the EDID EEPROM. The ADV7511 is unfortunately a 
bit different. You tell it to fetch the EDID information, then it will do 
some magic and then you can read the EDID back. Abstracting this as a this 
as a I2C controller will, while possible, result in a fair amount of boiler 
plate code that will not look particularly pretty.

- Lars

[PATCH v2 00/11] ARM: dts: zynq: Prepare Parallella

[Lars-Peter Clausen]

On 07/28/2014 06:17 PM, Andreas F?rber wrote:
> Hi Lars-Peter,
>
> Am 25.07.2014 01:00, schrieb Andreas F?rber:
>> most notably I'm missing
>> ADI ADV7513 and AXI-HDMI support
> [...]
>> Cc: Lars-Peter Clausen  (HDMI)
>
> Could you please enlighten us what the status of upstreaming
> ADV7511/ADV7513 support is? It is declared "work in progress" here:
>
> http://wiki.analog.com/resources/tools-software/linux-drivers/drm/adv7511
>
> I see some adv7511 V4L bits in drivers/media/i2c/adv7511.c, but no
> drivers/gpu/drm/i2c/adv7511_{core,audio}.c as on the xcomm_zynq branch,
> nor any devicetree documentation. Patchwork doesn't show any recent
> submissions to LKML.
>
> Is any major rework needed for you to get the 3.14.12 based driver upstream?
>

It's complicated. The plan for the driver was to wait for the common display 
framework (CDF) and convert it to use CDF and then submit it upstream. The 
CDF has been rejected though. Meanwhile the V4L2 adv7511 driver has been 
merged. So now we are in the ugly situation that we have two different 
drivers for two different frameworks. To fix this we need to merge these two 
drivers while still exposing the interfaces to both frameworks.

> AXI SPDIF I found in 3.16, as you noticed; what about AXI HDMI? [*]
> Is there any work ongoing to get that upstream as well?

We need to teach the DMAengine framework about cyclic interleaved transfers 
before the AXI HDMI driver can be submitted upstream.

- Lars

[alsa-devel] [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Lars-Peter Clausen]

On 02/05/2014 07:07 PM, Jean-Francois Moine wrote:
> On Wed, 05 Feb 2014 10:19:22 +0100
> Lars-Peter Clausen  wrote:
>
>>> So, in the CODEC, I don't see how I could update the parameters
>>> dictated by the EDID otherwise in changing the DAI driver parameters.
>>>
>>
>> The startup function is the right place. But instead of modifying the DAI
>> use snd_pcm_hw_constraint_mask64(), snd_pcm_hw_constraint_list(), etc. to
>> setup the additional constraints that come from the EDID.
>
> It is more complicated, but it works. Nevertheless, I have 2 problems:
>
> - snd_pcm_hw_constraint_list() keeps a pointer to the list, so, it
>cannot be in the stack. It fix this with static struct and rate array.
>

Right. If the struct is modified though it should be per device and not 
global. I think the best way to implement this is to make the array static 
and specify a mask for the constraint based on the EDID. E.g.

static unsigned int hdmi_rates[] = {
32000,
44100,
48000,
88200,
96000,
176400,
192000,
};

rate_mask = 0;

while (...) {
...
rate_mask |= 1 << sad[1];
}

rate_constraints->list = hdmi_rates;
rate_constraints->count = ARRAY_SIZE(hdmi_rates);
rate_constraints->mask = rate_mask;

> - snd_pcm_hw_constraint_mask64() is not exported.
>Is there an other way to set constraints on the formats/sample widths?

I think that's a bug. Both snd_pcm_hw_constraint_mask() and 
snd_pcm_hw_constraint_mask64() should be exported. Can you send a patch?

- Lars

[alsa-devel] [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Lars-Peter Clausen]

On 02/05/2014 12:18 PM, Mark Brown wrote:
> On Wed, Feb 05, 2014 at 10:19:22AM +0100, Lars-Peter Clausen wrote:
[..]
>> Bonus points for making this a generic helper function that takes a
>> runtime and a EDID and then applies the EDID constraints on the
>> runtime.
>
> ...as previously requested.  I know there was some discusion of broader
> moves to factor this stuff out but it'd still be good to keep it
> separated out even prior to a final non-EDID based solution so it's
> easier to refactor.

I think it will always be EDID (or ELD) based. As I understood it the 
re-factoring Takashi was talking about is related to how this data is passed 
from the graphics driver to the audio driver. The way things work right now 
in HDA land is that the graphics driver reads the EDID from the monitor, 
converts it to ELD and writes it to a special memory region in the graphics 
controller. This generates an interrupt in the audio driver and the audio 
driver reads the ELD from the hardware and sets up the constraints based on 
that. And I think that the plan is to change this to pass the EDID directly 
from the graphics driver to the audio driver without taking the detour 
through the hardware. This is what we'll need for embedded systems anyway. A 
system that allows to associate a sound driver with a specific HDMI port and 
status updates for that port, e.g. new EDID or cable connected/disconnected 
are passed from the graphics driver handling that port to the audio driver.

- Lars

[alsa-devel] [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Lars-Peter Clausen]

On 02/05/2014 10:11 AM, Jean-Francois Moine wrote:
> On Tue, 4 Feb 2014 18:06:25 +
> Mark Brown  wrote:
>
>> On Mon, Jan 27, 2014 at 09:48:54AM +0100, Jean-Francois Moine wrote:
>>
>>> +   /* change the snd_soc_pcm_stream values of the driver */
>>> +   stream->rates = rate_mask;
>>> +   stream->channels_max = max_channels;
>>> +   stream->formats = formats;
>>
>>> +   /* copy the DAI driver to a writable area */
>>> +   dai_drv = devm_kzalloc(>dev, sizeof(tda998x_dai), GFP_KERNEL);
>>> +   if (!dai_drv)
>>> +   return -ENOMEM;
>>> +   memcpy(dai_drv, tda998x_dai, sizeof(tda998x_dai));
>>> +
>>
>> The code should be doing this by setting constraints based on the
>> current setup rather than by editing the data structure - the expecation
>> is very much that the data won't change so this prevents surprises with
>> future work on the core.
>
> As it is done in the soc core, in soc_pcm_open(), the runtime hw_params
> are initialized after the call to the CODEC startup, and the next CODEC
> event is hw_params() when the user has already chosen all the parameters.
>
> So, in the CODEC, I don't see how I could update the parameters
> dictated by the EDID otherwise in changing the DAI driver parameters.
>

The startup function is the right place. But instead of modifying the DAI 
use snd_pcm_hw_constraint_mask64(), snd_pcm_hw_constraint_list(), etc. to 
setup the additional constraints that come from the EDID.

Bonus points for making this a generic helper function that takes a runtime 
and a EDID and then applies the EDID constraints on the runtime.

- Lars

[alsa-devel] [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x

[Lars-Peter Clausen]

On 02/04/2014 02:30 PM, Mark Brown wrote:
[...]
>
> What does this actually do?  No information is being passed in to the
> core function here, not even any information on if it's starting or
> stopping.  Looking at the rest of the code I can't help thinking it
> might be clearer to inline this possibly with a lookup helper, the code
> is very small and the lack of parameters makes it hard to follow.
>
>> +static const struct snd_soc_dapm_route tda_routes[] = {
>> +{ "hdmi-out", NULL, "HDMI I2S Playback" },
>> +{ "hdmi-out", NULL, "HDMI SPDIF Playback" },
>> +};
>
> S/PDIF.

Won't this cause issues with the debugfs widget entries? It's fixable by 
escaping it (replace it by a dash or something) in the debugfs widget 
filename, but I don't think we do this right now.

- Lars

[alsa-devel] [PATCH RFC v3 0/8] Beaglebone-Black HDMI audio

[Lars-Peter Clausen]

On 01/27/2014 08:40 PM, Jyri Sarha wrote:
> On 01/27/2014 06:32 PM, Lars-Peter Clausen wrote:
>> On 01/27/2014 05:17 PM, Jyri Sarha wrote:
>>> On 01/27/2014 05:51 PM, Lars-Peter Clausen wrote:
>>>> Hi,
>>>>
>>>> I think you should try to get this in sync with the work Jean-Francois is
>>>> currently doing[1]. Having the HDMI transmitter register a CODEC device is
>>>> definitely the right approach, compared to the rather 'interesting'
>>>> constraints stuff you do in patch 4.
>>>>
>>>
>>> Oh, Jean-Francois's patches are now out. I'll take those into use, but I am
>>> afraid it still does not save me from the constraint stuff.
>>>
>>> The most complex piece of code comes from limited sample-rate availability,
>>> which is coming Beaglebone-Black desing (available i2s master clock), not
>>> from the HDMI encoder itself. It does not help with the stereo only
>>> limitation either, because it comes from the one wire only audio data
>>> connection on BBB.
>>>
>>> Support for S16_LE could maybe be added if the tda998x specific codec would
>>> fiddle with CTS_N predivider-setting (K select) according to the used sample
>>> width. But it appears Cobox plays all the sample formats fine without this
>>> change, so the question is if playing around with CTS_N predivider would
>>> break already working support there (something to be tested).
>>
>> The ASoC core will set the constraints for the audio format/rate to the
>> intersection of what is supported by the CODEC and the CPU DAI. So if the
>> limitation comes from the CPU DAI the constraints should be applied there
>> and not in the machine driver. Similar if the tda998x only supports 32 bit
>> samples it should advertise this and the compound card will only support 32
>> bit samples.
>>
>
> Yes. I know. But these limitations come from the run time setup of the audio
> serial bus and those details are not available to the cpu dai at the register
> time.
>
> If more of the McASP configuration data would be moved to DT, instead of 
> giving
> it in set_sysclk, set_fmt, etc. calls it would be possible for the driver code
> produce more accurate constraints at register time. However that would change
> McASP driver a lot and would possibly break some of the legacy support.

There is nothing wrong with setting the constraints based on the parameters 
passed to set_sysclk or set_fmt, etc. In fact this is something that is often 
done by drivers.

- Lars

[alsa-devel] [PATCH RFC v3 0/8] Beaglebone-Black HDMI audio

[Lars-Peter Clausen]

On 01/27/2014 05:17 PM, Jyri Sarha wrote:
> On 01/27/2014 05:51 PM, Lars-Peter Clausen wrote:
>> Hi,
>>
>> I think you should try to get this in sync with the work Jean-Francois is
>> currently doing[1]. Having the HDMI transmitter register a CODEC device is
>> definitely the right approach, compared to the rather 'interesting'
>> constraints stuff you do in patch 4.
>>
> 
> Oh, Jean-Francois's patches are now out. I'll take those into use, but I am
> afraid it still does not save me from the constraint stuff.
> 
> The most complex piece of code comes from limited sample-rate availability,
> which is coming Beaglebone-Black desing (available i2s master clock), not
> from the HDMI encoder itself. It does not help with the stereo only
> limitation either, because it comes from the one wire only audio data
> connection on BBB.
> 
> Support for S16_LE could maybe be added if the tda998x specific codec would
> fiddle with CTS_N predivider-setting (K select) according to the used sample
> width. But it appears Cobox plays all the sample formats fine without this
> change, so the question is if playing around with CTS_N predivider would
> break already working support there (something to be tested).

The ASoC core will set the constraints for the audio format/rate to the
intersection of what is supported by the CODEC and the CPU DAI. So if the
limitation comes from the CPU DAI the constraints should be applied there
and not in the machine driver. Similar if the tda998x only supports 32 bit
samples it should advertise this and the compound card will only support 32
bit samples.

- Lars

[alsa-devel] [PATCH RFC v3 0/8] Beaglebone-Black HDMI audio

[Lars-Peter Clausen]

Hi,

I think you should try to get this in sync with the work Jean-Francois is
currently doing[1]. Having the HDMI transmitter register a CODEC device is
definitely the right approach, compared to the rather 'interesting'
constraints stuff you do in patch 4.

- Lars

[1] http://lkml.org/lkml/2014/1/27/85


On 01/27/2014 04:37 PM, Jyri Sarha wrote:
> Changes since RFC v2 version of the patches:
> - Dropped out already applied:
>   ASoC: hdmi-codec: Add devicetree binding with documentation
> - Addresses Mark's comments here:
>   
> http://mailman.alsa-project.org/pipermail/alsa-devel/2013-December/070605.html
>   - ASoC: davinci-evm: Add named clock reference to DT bindings
> - Get rid of unnecessary castings
> - Add mclk NULL checks
> - Use devm_clk_get()
> - Change clock name from "ti,codec-clock" to "mclk"
> - Address Mark's comments here:
>   
> http://mailman.alsa-project.org/pipermail/alsa-devel/2013-December/070606.html
>   - ASoC: davinci-evm: HDMI audio support for TDA998x trough McASP I2S bus
> - Get rid of unnecessary castings
> - Update commit message
>   - Add: ASoC: davinci-mcasp: Set BCLK divider if McASP is BCLK master
> - Use snd_soc_params_to_bclk()
> 
> Changes since the first RFC version of the patches:
> - Drop out already applied: 
>   ASoC: hdmi-codec: Add SNDRV_PCM_FMTBIT_32_LE playback format
> - Change sound node's compatible property
>   form: "ti,am33xx-beaglebone-black" to "ti,am33xx-beaglebone-black-audio"
> - Some minor style issue fixes from TI internal review
> 
> Jyri Sarha (8):
>   clk: add gpio controlled clock
>   ASoC: davinci-evm: Add named clock reference to DT bindings
>   ASoC: davinci-mcasp: Set BCLK divider if McASP is BCLK master
>   ASoC: davinci-evm: HDMI audio support for TDA998x trough McASP I2S
> bus
>   ASoC: davinci: HDMI audio build for AM33XX and TDA998x
>   drm/tilcdc: Add I2C HDMI audio config for tda998x
>   ARM: OMAP2+: omap2plus_defconfig: Enable tilcdc and TDA998X HDMI
> support
>   ARM: OMAP2+: omap2plus_defconfig: Enable BeagleBone Black HDMI audio
> support
> 
>  .../devicetree/bindings/clock/gpio-clock.txt   |   21 ++
>  .../bindings/sound/davinci-evm-audio.txt   |   13 +-
>  arch/arm/configs/omap2plus_defconfig   |5 +
>  drivers/clk/Makefile   |1 +
>  drivers/clk/clk-gpio.c |  210 +++
>  drivers/gpu/drm/tilcdc/tilcdc_slave.c  |   24 ++-
>  include/linux/clk-provider.h   |   25 +++
>  sound/soc/davinci/Kconfig  |   12 ++
>  sound/soc/davinci/Makefile |1 +
>  sound/soc/davinci/davinci-evm.c|  211 
> +++-
>  sound/soc/davinci/davinci-mcasp.c  |   20 ++
>  11 files changed, 534 insertions(+), 9 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/gpio-clock.txt
>  create mode 100644 drivers/clk/clk-gpio.c
> 

[PATCH 0/8] i2c: Remove redundant driver field from the i2c_client struct

[Lars-Peter Clausen]

Hi,

This series removes the redundant driver field from the i2c_client struct. The
field is redundant since the same pointer can be accessed through
to_i2c_driver(client-dev.driver). The commit log suggests that the field has
been around since forever (since before v2.6.12-rc2) and it looks as if it was
simply forgotten to remove it during the conversion of the i2c framework to the
generic device driver model.

Nevertheless there are a still a few users of the field around. This series
first updates all users to use an alternative method of accessing the same data
and then the last patch removes the driver field from the i2c_client struct.

Note that due to this changes on most architectures neither the code size nor
the type of generated instructions will change. This is due to the fact that we
aren't really interested in the pointer value itself, but rather want to
dereference it to access one of the fields of the struct. offset_of() (and hence
to_i2c_driver) works by subtracting a offset from the pointer, so the compiler
can internally create the sum of these two offsets and use that to access the
field.

E.g. on ARM the expression client-driver-command(...) generates

...
ldr r3, [r0, #28]
ldr r3, [r3, #32]
blx r3
...

and the expression to_i2c_driver(client-dev.driver)-command(...) generates

...
ldr r3, [r0, #160]
ldr r3, [r3, #-4]
blx r3
...

Other architectures will generate similar code.

The most common pattern is to use the i2c_driver to get to the device_driver
struct embedded in it. The same struct can easily be accessed through the device
struct embedded in the i2c_client struct.  E.g. client-driver-driver.field can
be replaced by client-dev.driver-field. Here again the generated code is
almost identical and only the offsets differ.

E.g. on ARM the expression 'client-driver-driver.owner' generates

ldr r3, [r0, #28]
ldr r0, [r3, #44]

and 'client-dev.driver-owner' generates

ldr r3, [r0, #160]
ldr r0, [r3, #8]

The kernel overall code size is slightly reduced since the code that manages the
driver field is removed and of course also the runtime memory footprint of the
i2c_client struct is reduced.

- Lars

Lars-Peter Clausen (8):
  [media] s5c73m3: Don't use i2c_client-driver
  [media] exynos4-is: Don't use i2c_client-driver
  [media] core: Don't use i2c_client-driver
  drm: encoder_slave: Don't use i2c_client-driver
  drm: nouveau: Don't use i2c_client-driver
  ALSA: ppc: keywest: Don't use i2c_client-driver
  ASoC: imx-wm8962: Don't use i2c_client-driver
  i2c: Remove redundant 'driver' field from the i2c_client struct

 drivers/gpu/drm/drm_encoder_slave.c|  8 
 drivers/gpu/drm/nouveau/core/subdev/therm/ic.c |  3 ++-
 drivers/i2c/i2c-core.c | 21 -
 drivers/i2c/i2c-smbus.c| 10 ++
 drivers/media/i2c/s5c73m3/s5c73m3-core.c   |  2 +-
 drivers/media/platform/exynos4-is/media-dev.c  |  6 +++---
 drivers/media/v4l2-core/tuner-core.c   |  6 +++---
 drivers/media/v4l2-core/v4l2-common.c  | 10 +-
 include/linux/i2c.h|  2 --
 include/media/v4l2-common.h|  2 +-
 sound/ppc/keywest.c|  4 ++--
 sound/soc/fsl/imx-wm8962.c |  2 +-
 12 files changed, 40 insertions(+), 36 deletions(-)

-- 
1.8.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/8] [media] s5c73m3: Don't use i2c_client-driver

[Lars-Peter Clausen]

The 'driver' field of the i2c_client struct is redundant and is going to be
removed. The results of the expressions 'client-driver.driver-field' and
'client-dev.driver-field' are identical, so replace all occurrences of the
former with the later.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Andrzej Hajda a.ha...@samsung.com
---
 drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c 
b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
index b76ec0e..1083890 100644
--- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
+++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
@@ -1581,7 +1581,7 @@ static int s5c73m3_probe(struct i2c_client *client,
oif_sd = state-oif_sd;
 
v4l2_subdev_init(sd, s5c73m3_subdev_ops);
-   sd-owner = client-driver-driver.owner;
+   sd-owner = client-dev.driver-owner;
v4l2_set_subdevdata(sd, state);
strlcpy(sd-name, S5C73M3, sizeof(sd-name));
 
-- 
1.8.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/8] [media] exynos4-is: Don't use i2c_client-driver

[Lars-Peter Clausen]

The 'driver' field of the i2c_client struct is redundant and is going to be
removed. The results of the expressions 'client-driver.driver-field' and
'client-dev.driver-field' are identical, so replace all occurrences of the
former with the later.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Sylwester Nawrocki s.nawro...@samsung.com
---
 drivers/media/platform/exynos4-is/media-dev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c 
b/drivers/media/platform/exynos4-is/media-dev.c
index a835112..7a4ee4c 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -411,8 +411,8 @@ static int fimc_md_of_add_sensor(struct fimc_md *fmd,
 
device_lock(client-dev);
 
-   if (!client-driver ||
-   !try_module_get(client-driver-driver.owner)) {
+   if (!client-dev.driver ||
+   !try_module_get(client-dev.driver-owner)) {
ret = -EPROBE_DEFER;
v4l2_info(fmd-v4l2_dev, No driver found for %s\n,
node-full_name);
@@ -442,7 +442,7 @@ static int fimc_md_of_add_sensor(struct fimc_md *fmd,
fmd-num_sensors++;
 
 mod_put:
-   module_put(client-driver-driver.owner);
+   module_put(client-dev.driver-owner);
 dev_put:
device_unlock(client-dev);
put_device(client-dev);
-- 
1.8.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/8] drm: encoder_slave: Don't use i2c_client-driver

[Lars-Peter Clausen]

The 'driver' field of the i2c_client struct is redundant and is going to be
removed. The results of the expressions 'client-driver.driver-field' and
'client-dev.driver-field' are identical, so replace all occurrences of the
former with the later. To get direct access to the i2c_driver struct use
'to_i2c_driver(client-dev.driver)'.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
 drivers/gpu/drm/drm_encoder_slave.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_encoder_slave.c 
b/drivers/gpu/drm/drm_encoder_slave.c
index 0cfb60f..d18b88b 100644
--- a/drivers/gpu/drm/drm_encoder_slave.c
+++ b/drivers/gpu/drm/drm_encoder_slave.c
@@ -67,12 +67,12 @@ int drm_i2c_encoder_init(struct drm_device *dev,
goto fail;
}
 
-   if (!client-driver) {
+   if (!client-dev.driver) {
err = -ENODEV;
goto fail_unregister;
}
 
-   module = client-driver-driver.owner;
+   module = client-dev.driver-owner;
if (!try_module_get(module)) {
err = -ENODEV;
goto fail_unregister;
@@ -80,7 +80,7 @@ int drm_i2c_encoder_init(struct drm_device *dev,
 
encoder-bus_priv = client;
 
-   encoder_drv = to_drm_i2c_encoder_driver(client-driver);
+   encoder_drv = 
to_drm_i2c_encoder_driver(to_i2c_driver(client-dev.driver));
 
err = encoder_drv-encoder_init(client, dev, encoder);
if (err)
@@ -111,7 +111,7 @@ void drm_i2c_encoder_destroy(struct drm_encoder 
*drm_encoder)
 {
struct drm_encoder_slave *encoder = to_encoder_slave(drm_encoder);
struct i2c_client *client = drm_i2c_encoder_get_client(drm_encoder);
-   struct module *module = client-driver-driver.owner;
+   struct module *module = client-dev.driver-owner;
 
i2c_unregister_device(client);
encoder-bus_priv = NULL;
-- 
1.8.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/8] drm: nouveau: Don't use i2c_client-driver

[Lars-Peter Clausen]

The 'driver' field of the i2c_client struct is redundant and is going to be
removed. Use 'to_i2c_driver(client-dev.driver)' instead to get direct access to
the i2c_driver struct.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
Cc: Martin Peres martin.pe...@labri.fr
---
 drivers/gpu/drm/nouveau/core/subdev/therm/ic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/core/subdev/therm/ic.c 
b/drivers/gpu/drm/nouveau/core/subdev/therm/ic.c
index 8b3adec..eae939d 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/therm/ic.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/therm/ic.c
@@ -41,7 +41,8 @@ probe_monitoring_device(struct nouveau_i2c_port *i2c,
if (!client)
return false;
 
-   if (!client-driver || client-driver-detect(client, info)) {
+   if (!client-dev.driver ||
+   to_i2c_driver(client-dev.driver)-detect(client, info)) {
i2c_unregister_device(client);
return false;
}
-- 
1.8.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/8] ALSA: ppc: keywest: Don't use i2c_client-driver

[Lars-Peter Clausen]

The 'driver' field of the i2c_client struct is redundant and is going to be
removed. Use 'to_i2c_driver(client-dev.driver)' instead to get direct
access to the i2c_driver struct.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
 sound/ppc/keywest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
index 01aecc2..0d1c27e 100644
--- a/sound/ppc/keywest.c
+++ b/sound/ppc/keywest.c
@@ -65,7 +65,7 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter)
 * already bound. If not it means binding failed, and then there
 * is no point in keeping the device instantiated.
 */
-   if (!keywest_ctx-client-driver) {
+   if (!keywest_ctx-client-dev.driver) {
i2c_unregister_device(keywest_ctx-client);
keywest_ctx-client = NULL;
return -ENODEV;
@@ -76,7 +76,7 @@ static int keywest_attach_adapter(struct i2c_adapter *adapter)
 * This is safe because i2c-core holds the core_lock mutex for us.
 */
list_add_tail(keywest_ctx-client-detected,
- keywest_ctx-client-driver-clients);
+ to_i2c_driver(keywest_ctx-client-dev.driver)-clients);
return 0;
 }
 
-- 
1.8.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/8] [media] core: Don't use i2c_client-driver

[Lars-Peter Clausen]

The 'driver' field of the i2c_client struct is redundant and is going to be
removed. The results of the expressions 'client-driver.driver-field' and
'client-dev.driver-field' are identical, so replace all occurrences of the
former with the later.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
 drivers/media/v4l2-core/tuner-core.c  |  6 +++---
 drivers/media/v4l2-core/v4l2-common.c | 10 +-
 include/media/v4l2-common.h   |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/tuner-core.c 
b/drivers/media/v4l2-core/tuner-core.c
index ddc9379..4133af0 100644
--- a/drivers/media/v4l2-core/tuner-core.c
+++ b/drivers/media/v4l2-core/tuner-core.c
@@ -43,7 +43,7 @@
 
 #define UNSET (-1U)
 
-#define PREFIX (t-i2c-driver-driver.name)
+#define PREFIX (t-i2c-dev.driver-name)
 
 /*
  * Driver modprobe parameters
@@ -452,7 +452,7 @@ static void set_type(struct i2c_client *c, unsigned int 
type,
}
 
tuner_dbg(%s %s I2C addr 0x%02x with type %d used for 0x%02x\n,
- c-adapter-name, c-driver-driver.name, c-addr  1, type,
+ c-adapter-name, c-dev.driver-name, c-addr  1, type,
  t-mode_mask);
return;
 
@@ -556,7 +556,7 @@ static void tuner_lookup(struct i2c_adapter *adap,
int mode_mask;
 
if (pos-i2c-adapter != adap ||
-   strcmp(pos-i2c-driver-driver.name, tuner))
+   strcmp(pos-i2c-dev.driver-name, tuner))
continue;
 
mode_mask = pos-mode_mask;
diff --git a/drivers/media/v4l2-core/v4l2-common.c 
b/drivers/media/v4l2-core/v4l2-common.c
index 037d7a5..433d6d7 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -236,14 +236,14 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct 
i2c_client *client,
v4l2_subdev_init(sd, ops);
sd-flags |= V4L2_SUBDEV_FL_IS_I2C;
/* the owner is the same as the i2c_client's driver owner */
-   sd-owner = client-driver-driver.owner;
+   sd-owner = client-dev.driver-owner;
sd-dev = client-dev;
/* i2c_client and v4l2_subdev point to one another */
v4l2_set_subdevdata(sd, client);
i2c_set_clientdata(client, sd);
/* initialize name */
snprintf(sd-name, sizeof(sd-name), %s %d-%04x,
-   client-driver-driver.name, i2c_adapter_id(client-adapter),
+   client-dev.driver-name, i2c_adapter_id(client-adapter),
client-addr);
 }
 EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
@@ -274,11 +274,11 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct 
v4l2_device *v4l2_dev,
   loaded. This delay-load mechanism doesn't work if other drivers
   want to use the i2c device, so explicitly loading the module
   is the best alternative. */
-   if (client == NULL || client-driver == NULL)
+   if (client == NULL || client-dev.driver == NULL)
goto error;
 
/* Lock the module so we can safely get the v4l2_subdev pointer */
-   if (!try_module_get(client-driver-driver.owner))
+   if (!try_module_get(client-dev.driver-owner))
goto error;
sd = i2c_get_clientdata(client);
 
@@ -287,7 +287,7 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct 
v4l2_device *v4l2_dev,
if (v4l2_device_register_subdev(v4l2_dev, sd))
sd = NULL;
/* Decrease the module use count to match the first try_module_get. */
-   module_put(client-driver-driver.owner);
+   module_put(client-dev.driver-owner);
 
 error:
/* If we have a client but no subdev, then something went wrong and
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 16550c4..a707529 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -35,7 +35,7 @@
printk(level %s %d-%04x:  fmt, name, i2c_adapter_id(adapter), addr , 
## arg)
 
 #define v4l_client_printk(level, client, fmt, arg...)  \
-   v4l_printk(level, (client)-driver-driver.name, (client)-adapter, \
+   v4l_printk(level, (client)-dev.driver-name, (client)-adapter, \
   (client)-addr, fmt , ## arg)
 
 #define v4l_err(client, fmt, arg...) \
-- 
1.8.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/8] ASoC: imx-wm8962: Don't use i2c_client-driver

[Lars-Peter Clausen]

The 'driver' field of the i2c_client struct is redundant and is going to be
removed. Check i2c_client-dev.driver instead to see if a driver is bound to the
device.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
 sound/soc/fsl/imx-wm8962.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/imx-wm8962.c b/sound/soc/fsl/imx-wm8962.c
index 6c60666..f84ecbf 100644
--- a/sound/soc/fsl/imx-wm8962.c
+++ b/sound/soc/fsl/imx-wm8962.c
@@ -215,7 +215,7 @@ static int imx_wm8962_probe(struct platform_device *pdev)
goto fail;
}
codec_dev = of_find_i2c_device_by_node(codec_np);
-   if (!codec_dev || !codec_dev-driver) {
+   if (!codec_dev || !codec_dev-dev.driver) {
dev_err(pdev-dev, failed to find codec platform device\n);
ret = -EINVAL;
goto fail;
-- 
1.8.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 8/8] i2c: Remove redundant 'driver' field from the i2c_client struct

[Lars-Peter Clausen]

The 'driver' field of the i2c_client struct is redundant. The same data can be
accessed through to_i2c_driver(client-dev.driver). The generated code for both
approaches in more or less the same.

E.g. on ARM the expression client-driver-command(...) generates

...
ldr r3, [r0, #28]
ldr r3, [r3, #32]
blx r3
...

and the expression to_i2c_driver(client-dev.driver)-command(...) generates

...
ldr r3, [r0, #160]
ldr r3, [r3, #-4]
blx r3
...

Other architectures will generate similar code.

All users of the 'driver' field outside of the I2C core have already been
converted. So this only leaves the core itself. This patch converts the
remaining few users in the I2C core and then removes the 'driver' field from the
i2c_client struct.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
 drivers/i2c/i2c-core.c  | 21 -
 drivers/i2c/i2c-smbus.c | 10 ++
 include/linux/i2c.h |  2 --
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 29d3f04..430c001 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -248,17 +248,16 @@ static int i2c_device_probe(struct device *dev)
driver = to_i2c_driver(dev-driver);
if (!driver-probe || !driver-id_table)
return -ENODEV;
-   client-driver = driver;
+
if (!device_can_wakeup(client-dev))
device_init_wakeup(client-dev,
client-flags  I2C_CLIENT_WAKE);
dev_dbg(dev, probe\n);
 
status = driver-probe(client, i2c_match_id(driver-id_table, client));
-   if (status) {
-   client-driver = NULL;
+   if (status)
i2c_set_clientdata(client, NULL);
-   }
+
return status;
 }
 
@@ -279,10 +278,9 @@ static int i2c_device_remove(struct device *dev)
dev-driver = NULL;
status = 0;
}
-   if (status == 0) {
-   client-driver = NULL;
+   if (status == 0)
i2c_set_clientdata(client, NULL);
-   }
+
return status;
 }
 
@@ -1606,9 +1604,14 @@ static int i2c_cmd(struct device *dev, void *_arg)
 {
struct i2c_client   *client = i2c_verify_client(dev);
struct i2c_cmd_arg  *arg = _arg;
+   struct i2c_driver   *driver;
+
+   if (!client || !client-dev.driver)
+   return 0;
 
-   if (client  client-driver  client-driver-command)
-   client-driver-command(client, arg-cmd, arg-arg);
+   driver = to_i2c_driver(client-dev.driver);
+   if (driver-command)
+   driver-command(client, arg-cmd, arg-arg);
return 0;
 }
 
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 44d4c60..c99b229 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -46,6 +46,7 @@ static int smbus_do_alert(struct device *dev, void *addrp)
 {
struct i2c_client *client = i2c_verify_client(dev);
struct alert_data *data = addrp;
+   struct i2c_driver *driver;
 
if (!client || client-addr != data-addr)
return 0;
@@ -54,12 +55,13 @@ static int smbus_do_alert(struct device *dev, void *addrp)
 
/*
 * Drivers should either disable alerts, or provide at least
-* a minimal handler.  Lock so client-driver won't change.
+* a minimal handler.  Lock so the driver won't change.
 */
device_lock(dev);
-   if (client-driver) {
-   if (client-driver-alert)
-   client-driver-alert(client, data-flag);
+   if (client-dev.driver) {
+   driver = to_i2c_driver(client-dev.driver);
+   if (driver-alert)
+   driver-alert(client, data-flag);
else
dev_warn(client-dev, no driver alert()!\n);
} else
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 2ab11dc..eff50e0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -205,7 +205,6 @@ struct i2c_driver {
  * @name: Indicates the type of the device, usually a chip name that's
  * generic enough to hide second-sourcing and compatible revisions.
  * @adapter: manages the bus segment hosting this I2C device
- * @driver: device's driver, hence pointer to access routines
  * @dev: Driver model device node for the slave.
  * @irq: indicates the IRQ generated by this device (if any)
  * @detected: member of an i2c_driver.clients list or i2c-core's
@@ -222,7 +221,6 @@ struct i2c_client {
/* _LOWER_ 7 bits   */
char name[I2C_NAME_SIZE];
struct i2c_adapter *adapter;/* the adapter we sit on*/
-   struct i2c_driver *driver;  /* and our access routines

Re: [RFC 01/10] drm: ADV7511 i2c HDMI encoder driver

[Lars-Peter Clausen]

[...]
 +
 +/**
 + * enum adv7511_input_color_depth - Selects the input format color depth
 + * @ADV7511_INPUT_COLOR_DEPTH_8BIT: Input format color depth is 8 bits per 
 channel
 + * @ADV7511_INPUT_COLOR_DEPTH_10BIT: Input format color dpeth is 10 bits 
 per channel
 + * @ADV7511_INPUT_COLOR_DEPTH_12BIT: Input format color depth is 12 bits 
 per channel
 + **/
 +enum adv7511_input_color_depth {
 +ADV7511_INPUT_COLOR_DEPTH_8BIT = 3,
 +ADV7511_INPUT_COLOR_DEPTH_10BIT = 1,
 +ADV7511_INPUT_COLOR_DEPTH_12BIT = 2,
 +};
 
 Those enums describe the video format at the chip input. This can be 
 configured statically from platform data or DT, but some platforms might need 
 to setup formats dynamically at runtime. For instance the ADV7511 could be 
 driven by a mux with two inputs using different formats.
 
 For these reasons I would combine all those enums in a single one that lists 
 all possible input formats. The formats should be standardized and moved to a 
 separate header file. Get and set format operations will be needed (this is 
 something CDF will address :-)).

Since most these settings are orthogonal to each other putting them in one
enum gives you 3 * 3 * 6 * 3 = 162 entries. These enums configure to which
pins of the ADV7511 what signal is connected. Standardizing this would
require that other chips use the same layouts for connecting the pins.

[...]
 +
 +/**
 + * enum adv7511_up_conversion - Selects the upscaling conversion method
 + * @ADV7511_UP_CONVERSION_ZERO_ORDER: Use zero order up conversion
 + * @ADV7511_UP_CONVERSION_FIRST_ORDER: Use first order up conversion
 + *
 + * This used when converting from a 4:2:2 format to a 4:4:4 format.
 + **/
 +enum adv7511_up_conversion {
 +ADV7511_UP_CONVERSION_ZERO_ORDER = 0,
 +ADV7511_UP_CONVERSION_FIRST_ORDER = 1,
 +};
 
 How is the upscaling conversion method supposed to be selected ? What does it 
 depend on ?
 

It's probably up to the system designer to say which method yields better
results for their system.

[...]
 +/**
 + * struct adv7511_video_config - Describes adv7511 hardware configuration
 + * @csc_enable: Whether to enable color space conversion
 + * @csc_scaling_factor: Color space conversion scaling factor
 + * @csc_coefficents:Color space conversion coefficents
 + * @hdmi_mode:  Whether to use HDMI or DVI output mode
 + * @avi_infoframe:  HDMI infoframe
 + **/
 +struct adv7511_video_config {
 +bool csc_enable;
 
 Shouldn't this be automatically computed based on the input and output 
 formats 
 ?

If the driver knew the input and output colorspaces and had coefficient
tables for all possible combinations built in that could be done. This is
maybe something that could be done in the framework.

 
 +enum adv7511_csc_scaling csc_scaling_factor;
 +const uint16_t *csc_coefficents;
 
 Do the coefficients need to be configured freely, or could presets do ?
 
 +bool hdmi_mode;
 +struct hdmi_avi_infoframe avi_infoframe;
 +};
[...]
 +static void adv7511_set_config(struct drm_encoder *encoder, void *c)
 
 Now we're getting to the controversial point.
 
 What bothers me about the DRM encoder slave API is that the display 
 controller 
 driver needs to be aware of the details of the slave encoder, as it needs to 
 pass an encoder-specific configuration structure to the .set_config() 
 operation. The question would thus be, what about using the Common Display 
 Framework ?

Well, as I said I'd prefer using the CDF for this driver. But even then the
display controller driver might need to know about the details of the
encoder. I think we talked about this during the FOSDEM meeting. The
graphics fabric on the board can easily get complex enough to require a
board custom driver, similar to what we have in ASoC. I think this
drm_bridge patchset[1] tries to address a similar issue.

[1] http://lists.freedesktop.org/archives/dri-devel/2013-August/043237.html
[...]
 +
 +for (i = 0; i  4; ++i) {
 +ret = i2c_transfer(adv7511-i2c_edid-adapter, xfer, 
 ARRAY_SIZE(xfer));
 +if (ret  0)
 +return ret;
 +else if (ret != 2)
 +return -EIO;
 +
 +xfer[1].buf += 64;
 +offset += 64;
 +}
 
 Shouldn't you read two times 64 bytes per block, not four times ?

The controller on the ADV7511 always reads two blocks at once, so it is 256
bytes.

 
 +
 +adv7511-current_edid_segment = block / 2;
 +}
 +
 +if (block % 2 == 0)
 +memcpy(buf, adv7511-edid_buf, len);
 +else
 +memcpy(buf, adv7511-edid_buf + 128, len);
 +
 +return 0;
 +}
 +
[...]
 +
 +struct edid *adv7511_get_edid(struct drm_encoder *encoder)
 +{
 +struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
 +
 +if (!adv7511-edid)
 +return NULL;
 +
 +return 

[PATCH] drm/exynos: exynos_hdmi: Pass correct pointer to free_irq()

[Lars-Peter Clausen]

free_irq() expects the same pointer that was passed to request_threaded_irq(),
otherwise the IRQ is not freed.

The issue was found using the following coccinelle script:


@r1@
type T;
T devid;
@@
request_threaded_irq(..., devid)

@r2@
type r1.T;
T devid;
position p;
@@
free_irq at p(..., devid)

@@
position p != r2.p;
@@
*free_irq at p(...)


Signed-off-by: Lars-Peter Clausen 
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c 
b/drivers/gpu/drm/exynos/exynos_hdmi.c
index bbfc384..7e99853 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -2082,7 +2082,7 @@ static int hdmi_remove(struct platform_device *pdev)

pm_runtime_disable(dev);

-   free_irq(hdata->irq, hdata);
+   free_irq(hdata->irq, ctx);


/* hdmiphy i2c driver */
-- 
1.8.0

[PATCH] drm/exynos: exynos_hdmi: Pass correct pointer to free_irq()

[Lars-Peter Clausen]

free_irq() expects the same pointer that was passed to request_threaded_irq(),
otherwise the IRQ is not freed.

The issue was found using the following coccinelle script:

smpl
@r1@
type T;
T devid;
@@
request_threaded_irq(..., devid)

@r2@
type r1.T;
T devid;
position p;
@@
free_irq@p(..., devid)

@@
position p != r2.p;
@@
*free_irq@p(...)
/smpl

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c 
b/drivers/gpu/drm/exynos/exynos_hdmi.c
index bbfc384..7e99853 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -2082,7 +2082,7 @@ static int hdmi_remove(struct platform_device *pdev)
 
pm_runtime_disable(dev);
 
-   free_irq(hdata-irq, hdata);
+   free_irq(hdata-irq, ctx);
 
 
/* hdmiphy i2c driver */
-- 
1.8.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

CDF discussions at FOSDEM

[Lars-Peter Clausen]

On 01/31/2013 11:53 AM, Laurent Pinchart wrote:
> On Friday 11 January 2013 21:27:03 Laurent Pinchart wrote:
>> Hi everybody,
>>
>> Would anyone be interested in meeting at the FOSDEM to discuss the Common
>> Display Framework ? There will be a CDF meeting at the ELC at the end of
>> February, the FOSDEM would be a good venue for European developers.
> 
> A quick follow-up on this.
> 
> Given the late notice getting a room from the FOSDEM staff wasn't possible. 
> There will be two meeting rooms available that can be reserved on-site only. 
> They can accomodate aroudn 30 people but there will deliberately be no 
> projector. They will be given on a first-come, first-serve basis for one hour 
> time slots at most (see https://fosdem.org/2013/news/2013-01-31-bof-
> announce/).
> 
> As room availability isn't guaranteed, and as one hour might be a bit short, 
> I've secured an off-site but very close 
> (http://www.openstreetmap.org/?lat=50.812924=4.384506=18=M - 
> UrLab) room that can accomodate 12 people around a meeting table (more is 
> possible, but it might get a bit tight then). I propose having the CDF 
> discussion there on Sunday morning from 9am to 11am (please let me know ASAP 
> if you can't make it at that time).
> 
> Daniel Vetter
> Jani Nikula
> Marcus Lorentzon
> Laurent Pinchart
> Michael (from Pengutronix, not sure about the last name, sorry)
> Philipp Zabel
> Rob Clark
> Robert Schwebel
> Sascha Hauer
> Ville Syrj?l?
> 
> That's already 10 people. If someone else would like to attend the meeting 
> please let me know.
> 

If there's a free seat I'd like to attend as well.

Thanks,
- Lars

Re: CDF discussions at FOSDEM

[Lars-Peter Clausen]

On 01/31/2013 11:53 AM, Laurent Pinchart wrote:
 On Friday 11 January 2013 21:27:03 Laurent Pinchart wrote:
 Hi everybody,

 Would anyone be interested in meeting at the FOSDEM to discuss the Common
 Display Framework ? There will be a CDF meeting at the ELC at the end of
 February, the FOSDEM would be a good venue for European developers.
 
 A quick follow-up on this.
 
 Given the late notice getting a room from the FOSDEM staff wasn't possible. 
 There will be two meeting rooms available that can be reserved on-site only. 
 They can accomodate aroudn 30 people but there will deliberately be no 
 projector. They will be given on a first-come, first-serve basis for one hour 
 time slots at most (see https://fosdem.org/2013/news/2013-01-31-bof-
 announce/).
 
 As room availability isn't guaranteed, and as one hour might be a bit short, 
 I've secured an off-site but very close 
 (http://www.openstreetmap.org/?lat=50.812924lon=4.384506zoom=18layers=M - 
 UrLab) room that can accomodate 12 people around a meeting table (more is 
 possible, but it might get a bit tight then). I propose having the CDF 
 discussion there on Sunday morning from 9am to 11am (please let me know ASAP 
 if you can't make it at that time).
 
 Daniel Vetter
 Jani Nikula
 Marcus Lorentzon
 Laurent Pinchart
 Michael (from Pengutronix, not sure about the last name, sorry)
 Philipp Zabel
 Rob Clark
 Robert Schwebel
 Sascha Hauer
 Ville Syrjälä
 
 That's already 10 people. If someone else would like to attend the meeting 
 please let me know.
 

If there's a free seat I'd like to attend as well.

Thanks,
- Lars
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC v2 3/5] drm: Add HDMI infoframe helpers

[Lars-Peter Clausen]

On 12/06/2012 08:28 AM, Thierry Reding wrote:
> On Wed, Dec 05, 2012 at 06:51:20PM +0100, Lars-Peter Clausen wrote:
>> On 12/05/2012 05:45 PM, Thierry Reding wrote:
>>> Add a generic helper to fill in an HDMI AVI infoframe with data
>>> extracted from a DRM display mode.
>>
>> That's a very nice patch series, comes in pretty handy. Thanks :)
>>
>> I've just one comment.
>>
>>> [...]
>>> +static inline enum hdmi_picture_aspect
>>> +drm_display_mode_get_aspect(const struct drm_display_mode *mode)
>>> +{
>>> +   enum hdmi_picture_aspect aspect = HDMI_PICTURE_ASPECT_NONE;
>>> +
>>> +   if ((mode->hdisplay * 9) / 16 == mode->vdisplay)
>>> +   aspect = HDMI_PICTURE_ASPECT_16_9;
>>> +   else if ((mode->hdisplay * 3) / 4 == mode->vdisplay)
>>> +   aspect = HDMI_PICTURE_ASPECT_4_3;
>>> +
>>> +   return aspect;
>>> +}
>>> +
>>> +/**
>>> + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe 
>>> with
>>> + *  data from a DRM display 
>>> mode
>>> + * @frame: HDMI AVI infoframe
>>> + * @mode: DRM display mode
>>> + *
>>> + * Returns 0 on success or a negative error code on failure.
>>> + */
>>> +int
>>> +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>>> +const struct drm_display_mode *mode)
>>> +{
>> [...]
>>> +
>>> +   frame->picture_aspect = drm_display_mode_get_aspect(mode);
>>
>> As far as I know the picture aspect ratio is supposed to be the aspect ratio
>> of the displayed content not the pixel aspect ratio. The receiver already
>> knows the pixel aspect ratio since it knows both the pixel width and the
>> height. The picture aspect ratio is something that could be set by a
>> userspace application running in fullscreen, e.g. a video player.
> 
> Isn't that what the active aspect ratio is supposed to do? Unfortunately
> neither the HDMI specification nor CEA-861-D seem to be very specific
> about this. What I did in the above was basically just refactoring from
> what the Tegra driver does. None of the other drivers are filling in any
> useful values here. Most equipment seems to work just fine if you pass a
> zeroed out AVI infoframe. =)
> 

The active aspect ratio is for when you have black borders on the sides of your
images. The picture aspect field is for when you for example have non square
pixels, which probably does not happen so often with PC like device.

Hm, btw. I think some HDMI VICs are defined to have non-square pixels like for
example 576p I wonder if we should skip these when we select the VIC. Otherwise
the video data may look stretched when displayed on the HDMI sink.

> Given that we don't have a way to pass any kind of information to the
> kernel about this, the only options we have are to either use this or go
> with HDMI_PICTURE_ASPECT_NONE. I suppose that the latter will work fine
> too.
> 

Agreed. HDMI_PICTURE_ASPECT_NONE should hopefully work.

Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers

[Lars-Peter Clausen]

On 12/06/2012 08:28 AM, Thierry Reding wrote:
 On Wed, Dec 05, 2012 at 06:51:20PM +0100, Lars-Peter Clausen wrote:
 On 12/05/2012 05:45 PM, Thierry Reding wrote:
 Add a generic helper to fill in an HDMI AVI infoframe with data
 extracted from a DRM display mode.

 That's a very nice patch series, comes in pretty handy. Thanks :)

 I've just one comment.

 [...]
 +static inline enum hdmi_picture_aspect
 +drm_display_mode_get_aspect(const struct drm_display_mode *mode)
 +{
 +   enum hdmi_picture_aspect aspect = HDMI_PICTURE_ASPECT_NONE;
 +
 +   if ((mode-hdisplay * 9) / 16 == mode-vdisplay)
 +   aspect = HDMI_PICTURE_ASPECT_16_9;
 +   else if ((mode-hdisplay * 3) / 4 == mode-vdisplay)
 +   aspect = HDMI_PICTURE_ASPECT_4_3;
 +
 +   return aspect;
 +}
 +
 +/**
 + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe 
 with
 + *  data from a DRM display 
 mode
 + * @frame: HDMI AVI infoframe
 + * @mode: DRM display mode
 + *
 + * Returns 0 on success or a negative error code on failure.
 + */
 +int
 +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 +const struct drm_display_mode *mode)
 +{
 [...]
 +
 +   frame-picture_aspect = drm_display_mode_get_aspect(mode);

 As far as I know the picture aspect ratio is supposed to be the aspect ratio
 of the displayed content not the pixel aspect ratio. The receiver already
 knows the pixel aspect ratio since it knows both the pixel width and the
 height. The picture aspect ratio is something that could be set by a
 userspace application running in fullscreen, e.g. a video player.
 
 Isn't that what the active aspect ratio is supposed to do? Unfortunately
 neither the HDMI specification nor CEA-861-D seem to be very specific
 about this. What I did in the above was basically just refactoring from
 what the Tegra driver does. None of the other drivers are filling in any
 useful values here. Most equipment seems to work just fine if you pass a
 zeroed out AVI infoframe. =)
 

The active aspect ratio is for when you have black borders on the sides of your
images. The picture aspect field is for when you for example have non square
pixels, which probably does not happen so often with PC like device.

Hm, btw. I think some HDMI VICs are defined to have non-square pixels like for
example 576p I wonder if we should skip these when we select the VIC. Otherwise
the video data may look stretched when displayed on the HDMI sink.

 Given that we don't have a way to pass any kind of information to the
 kernel about this, the only options we have are to either use this or go
 with HDMI_PICTURE_ASPECT_NONE. I suppose that the latter will work fine
 too.
 

Agreed. HDMI_PICTURE_ASPECT_NONE should hopefully work.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC v2 3/5] drm: Add HDMI infoframe helpers

[Lars-Peter Clausen]

On 12/05/2012 05:45 PM, Thierry Reding wrote:
> Add a generic helper to fill in an HDMI AVI infoframe with data
> extracted from a DRM display mode.

That's a very nice patch series, comes in pretty handy. Thanks :)

I've just one comment.

> [...]
> +static inline enum hdmi_picture_aspect
> +drm_display_mode_get_aspect(const struct drm_display_mode *mode)
> +{
> + enum hdmi_picture_aspect aspect = HDMI_PICTURE_ASPECT_NONE;
> +
> + if ((mode->hdisplay * 9) / 16 == mode->vdisplay)
> + aspect = HDMI_PICTURE_ASPECT_16_9;
> + else if ((mode->hdisplay * 3) / 4 == mode->vdisplay)
> + aspect = HDMI_PICTURE_ASPECT_4_3;
> +
> + return aspect;
> +}
> +
> +/**
> + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe 
> with
> + *  data from a DRM display mode
> + * @frame: HDMI AVI infoframe
> + * @mode: DRM display mode
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int
> +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> +  const struct drm_display_mode *mode)
> +{
[...]
> +
> + frame->picture_aspect = drm_display_mode_get_aspect(mode);

As far as I know the picture aspect ratio is supposed to be the aspect ratio
of the displayed content not the pixel aspect ratio. The receiver already
knows the pixel aspect ratio since it knows both the pixel width and the
height. The picture aspect ratio is something that could be set by a
userspace application running in fullscreen, e.g. a video player.

> + frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
> +
> + return 0;
> +}

Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers

[Lars-Peter Clausen]

On 12/05/2012 05:45 PM, Thierry Reding wrote:
 Add a generic helper to fill in an HDMI AVI infoframe with data
 extracted from a DRM display mode.

That's a very nice patch series, comes in pretty handy. Thanks :)

I've just one comment.

 [...]
 +static inline enum hdmi_picture_aspect
 +drm_display_mode_get_aspect(const struct drm_display_mode *mode)
 +{
 + enum hdmi_picture_aspect aspect = HDMI_PICTURE_ASPECT_NONE;
 +
 + if ((mode-hdisplay * 9) / 16 == mode-vdisplay)
 + aspect = HDMI_PICTURE_ASPECT_16_9;
 + else if ((mode-hdisplay * 3) / 4 == mode-vdisplay)
 + aspect = HDMI_PICTURE_ASPECT_4_3;
 +
 + return aspect;
 +}
 +
 +/**
 + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe 
 with
 + *  data from a DRM display mode
 + * @frame: HDMI AVI infoframe
 + * @mode: DRM display mode
 + *
 + * Returns 0 on success or a negative error code on failure.
 + */
 +int
 +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 +  const struct drm_display_mode *mode)
 +{
[...]
 +
 + frame-picture_aspect = drm_display_mode_get_aspect(mode);

As far as I know the picture aspect ratio is supposed to be the aspect ratio
of the displayed content not the pixel aspect ratio. The receiver already
knows the pixel aspect ratio since it knows both the pixel width and the
height. The picture aspect ratio is something that could be set by a
userspace application running in fullscreen, e.g. a video player.

 + frame-active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
 +
 + return 0;
 +}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PULL] SH Mobile DRM driver for v3.7

[Lars-Peter Clausen]

On 09/15/2012 01:28 AM, Dave Airlie wrote:
> On Fri, Sep 14, 2012 at 10:38 PM, Laurent Pinchart
>  wrote:
>> Hi Dave,
>>
>> The SH Mobile DRM driver is now (in my opinion) ready for mainline. It
>> requires GEM and KMS/FB helpers that have been reviewed on the list and
>> tested. Sascha is waiting for them to reach your tree to send a pull request
>> for another new driver.
> 
> Just a quick review before I pull,
> 
> Why does include/drm/shmob_drm.h exist? this file is meant to define
> the userspace API to the driver, if you don't have any userspace API
> or driver specific ioctls, this file shouldn't be required. You might
> want to create include/drm/shmob_internal.h maybe, if this is used as
> an interface to other places in the kernel. I probably need to check
> other have been doing the right thing here as well. (driver_drm.h
> should be user facing only)
> 
> Uggh drm_fb_cma_helper.c is pure midlayer mistake, are you 100% sure
> no driver is ever going to want to tweak the drm_fbdev_cma_ops?

Obviously we can't, for the same reasons we can't know whether there will ever
be a driver which needs to use custom fb_ops. It works fine as it is now for
three different drivers. And other drivers making use of the cma buffer helpers
are likely to have similar requirements, but if we ever get to a point where a
driver needs custom fb_ops it is fairly easy to change it then.

- Lars

Re: [PULL] SH Mobile DRM driver for v3.7

[Lars-Peter Clausen]

On 09/15/2012 01:28 AM, Dave Airlie wrote:
 On Fri, Sep 14, 2012 at 10:38 PM, Laurent Pinchart
 laurent.pinch...@ideasonboard.com wrote:
 Hi Dave,

 The SH Mobile DRM driver is now (in my opinion) ready for mainline. It
 requires GEM and KMS/FB helpers that have been reviewed on the list and
 tested. Sascha is waiting for them to reach your tree to send a pull request
 for another new driver.
 
 Just a quick review before I pull,
 
 Why does include/drm/shmob_drm.h exist? this file is meant to define
 the userspace API to the driver, if you don't have any userspace API
 or driver specific ioctls, this file shouldn't be required. You might
 want to create include/drm/shmob_internal.h maybe, if this is used as
 an interface to other places in the kernel. I probably need to check
 other have been doing the right thing here as well. (driver_drm.h
 should be user facing only)
 
 Uggh drm_fb_cma_helper.c is pure midlayer mistake, are you 100% sure
 no driver is ever going to want to tweak the drm_fbdev_cma_ops?

Obviously we can't, for the same reasons we can't know whether there will ever
be a driver which needs to use custom fb_ops. It works fine as it is now for
three different drivers. And other drivers making use of the cma buffer helpers
are likely to have similar requirements, but if we ever get to a point where a
driver needs custom fb_ops it is fairly easy to change it then.

- Lars
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4] DRM: add drm gem CMA helper

[Lars-Peter Clausen]

On 08/08/2012 04:44 PM, Laurent Pinchart wrote:
> Hi Sascha,
> 
> On Wednesday 27 June 2012 15:30:18 Sascha Hauer wrote:
>> Many embedded drm devices do not have a IOMMU and no dedicated
>> memory for graphics. These devices use CMA (Contiguous Memory
>> Allocator) backed graphics memory. This patch provides helper
>> functions to be able to share the code. The code technically does
>> not depend on CMA as the backend allocator, the name has been chosen
>> because cma makes for a nice, short but still descriptive function
>> prefix.
>>
>> Signed-off-by: Sascha Hauer 
>> ---
>>
>> changes since v3:
>> - *really* use DIV_ROUND_UP(args->width * args->bpp, 8) to calculate the
>> pitch changes since v2:
>> - make drm_gem_cma_create_with_handle static
>> - use DIV_ROUND_UP(args->width * args->bpp, 8) to calculate the pitch
>> - make drm_gem_cma_vm_ops const
>> - add missing #include 
>> changes since v1:
>> - map whole buffer at mmap time, not page by page at fault time
>> - several cleanups as suggested by Lars-Peter Clausen and Laurent Pinchart
>>
>>  drivers/gpu/drm/Kconfig  |6 +
>>  drivers/gpu/drm/Makefile |1 +
>>  drivers/gpu/drm/drm_gem_cma_helper.c |  251 +++
>>  include/drm/drm_gem_cma_helper.h |   44 ++
>>  4 files changed, 302 insertions(+)
>>  create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c
>>  create mode 100644 include/drm/drm_gem_cma_helper.h
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index dc5df12..8f362ec 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -53,6 +53,12 @@ config DRM_TTM
>>GPU memory types. Will be enabled automatically if a device driver
>>uses it.
>>
>> +config DRM_GEM_CMA_HELPER
>> +tristate
>> +depends on DRM
>> +help
>> +  Choose this if you need the GEM CMA helper functions
>> +
>>  config DRM_TDFX
>>  tristate "3dfx Banshee/Voodoo3+"
>>  depends on DRM && PCI
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 0487ff6..8759cda 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -15,6 +15,7 @@ drm-y   := drm_auth.o drm_buffer.o drm_bufs.o
>> drm_cache.o \ drm_trace_points.o drm_global.o drm_prime.o
>>
>>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>> +drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> 
> This results in a built failure for me with
> 
> CONFIG_DRM=y
> CONFIG_DRM_KMS_HELPER=m
> # CONFIG_DRM_LOAD_EDID_FIRMWARE is not set
> CONFIG_DRM_GEM_CMA_HELPER=m
> CONFIG_DRM_KMS_CMA_HELPER=m
> 
> drm_gem_cma_helper.o isn't compiled at all. Can you reproduce the problem ?

The Kconfig entry needs to be bool instead of tristate.

- Lars

Re: [PATCH v4] DRM: add drm gem CMA helper

[Lars-Peter Clausen]

On 08/08/2012 04:44 PM, Laurent Pinchart wrote:
 Hi Sascha,
 
 On Wednesday 27 June 2012 15:30:18 Sascha Hauer wrote:
 Many embedded drm devices do not have a IOMMU and no dedicated
 memory for graphics. These devices use CMA (Contiguous Memory
 Allocator) backed graphics memory. This patch provides helper
 functions to be able to share the code. The code technically does
 not depend on CMA as the backend allocator, the name has been chosen
 because cma makes for a nice, short but still descriptive function
 prefix.

 Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
 ---

 changes since v3:
 - *really* use DIV_ROUND_UP(args-width * args-bpp, 8) to calculate the
 pitch changes since v2:
 - make drm_gem_cma_create_with_handle static
 - use DIV_ROUND_UP(args-width * args-bpp, 8) to calculate the pitch
 - make drm_gem_cma_vm_ops const
 - add missing #include linux/export.h
 changes since v1:
 - map whole buffer at mmap time, not page by page at fault time
 - several cleanups as suggested by Lars-Peter Clausen and Laurent Pinchart

  drivers/gpu/drm/Kconfig  |6 +
  drivers/gpu/drm/Makefile |1 +
  drivers/gpu/drm/drm_gem_cma_helper.c |  251 +++
  include/drm/drm_gem_cma_helper.h |   44 ++
  4 files changed, 302 insertions(+)
  create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c
  create mode 100644 include/drm/drm_gem_cma_helper.h

 diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
 index dc5df12..8f362ec 100644
 --- a/drivers/gpu/drm/Kconfig
 +++ b/drivers/gpu/drm/Kconfig
 @@ -53,6 +53,12 @@ config DRM_TTM
GPU memory types. Will be enabled automatically if a device driver
uses it.

 +config DRM_GEM_CMA_HELPER
 +tristate
 +depends on DRM
 +help
 +  Choose this if you need the GEM CMA helper functions
 +
  config DRM_TDFX
  tristate 3dfx Banshee/Voodoo3+
  depends on DRM  PCI
 diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
 index 0487ff6..8759cda 100644
 --- a/drivers/gpu/drm/Makefile
 +++ b/drivers/gpu/drm/Makefile
 @@ -15,6 +15,7 @@ drm-y   := drm_auth.o drm_buffer.o drm_bufs.o
 drm_cache.o \ drm_trace_points.o drm_global.o drm_prime.o

  drm-$(CONFIG_COMPAT) += drm_ioc32.o
 +drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
 
 This results in a built failure for me with
 
 CONFIG_DRM=y
 CONFIG_DRM_KMS_HELPER=m
 # CONFIG_DRM_LOAD_EDID_FIRMWARE is not set
 CONFIG_DRM_GEM_CMA_HELPER=m
 CONFIG_DRM_KMS_CMA_HELPER=m
 
 drm_gem_cma_helper.o isn't compiled at all. Can you reproduce the problem ?

The Kconfig entry needs to be bool instead of tristate.

- Lars

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

exynos drm hdmi audio: how to recieve audio parameters (sf, bps, channel count)

[Lars-Peter Clausen]

On 07/20/2012 01:44 PM, RAHUL SHARMA wrote:
> 
> On Fri, Jul 20, 2012 at 11:40 AM, RAHUL SHARMA  
> wrote:
>>
>>
>> --- Original Message -------
>> Sender : Lars-Peter Clausen
>> Date : Jul 19, 2012 19:12 (GMT+05:30)
>> Title : Re: exynos drm hdmi audio: how to recieve audio parameters (sf, bps, 
>> channel count)
>>
>> On 07/19/2012 02:22 PM, Rob Clark wrote:
>>> On Tue, Jul 17, 2012 at 1:12 AM, RAHUL SHARMA wrote:
>>>> hi,
>>>>
>>>> I am adding support for hdmi audio, inside exynos drm hdmi driver. Hdmi 
>>>> audio is initialized with default parameters. I want to implement the 
>>>> mechanism to update hdmi registers, whenever audio properties got changed 
>>>> (i2s/spdif). raedon/r600 drm dirver is polling the audio ip every 100 ms 
>>>> and reconfigure the hdmi audio block. This is not possible with exynos as 
>>>> all information cannot be collected from i2s tx registers. It is directly 
>>>> set on wm8994 connected through i2c.
>>>> Possible solution:
>>>> 1) drm driver exposing ioctl for setting audio parameters.
>>>> 2) alsa driver notifying the change in audio parameters through kernel 
>>>> notifiers. drm hdmi driver subscribed for the same.
>>>
>>> I am certainly not an audio expert, but I am pretty sure something
>>> along the lines of solution #2 would be better.  I don't think
>>> userspace would want to know about some exynos drm specific ioctls in
>>> order to make audio work.
>>>
>>> BR,
>>> -R
>>
>>
>> I don't know how the audio setup for exynos HDMI hardware looks like, so
>> this might not be applicable in this case. But what I did for HDMI
>> transmitter I was working on, is to register a ASoC codec device as a
>> subdevice to the HDMI transmitter. All access to the sound registers is done
>> from that subdevice and you don't need any special notifier hooks, since you
>> can use the normal alsa driver callbacks.
>>
>> It might be a good idea if you could describe how your setup looks exactly.
>> E.g. which components does the audio stream pass through, where does it
>> originate and how the different components related to each other.
>>
>> - Lars
> 
> Thanks Lars, Rob, Kyungmin,
> 
> In exynos, i2s and spdif DAIs configure the i2s, spdif tx IP regs. In hw, I2s 
> and
> spdif tx o/p pins are also connected to the i2s-In and spdif-In blocks of 
> hdmi tx.
> Drm hdmi sub driver needs to configure the i2s-In and spdif-In blocks with the
> same setting as in i2s and spdif tx IPs. Hdmi is solely accessed by drm driver
> where hdmi is registered as a sub-driver along with lcd sub-driver. I hope, I 
> am
> able to explain the flow and connections, through above.

With this setup, I think, implementing the HDMI audio as a ASoC codec driver
is the right approach.

> 
> Lars, how can I refer your implementation where Adding 'hdmi audio' as a 
> seperate
> subdrv. If I understand correctly, it will be exposed as a seperate sound 
> card,
> parallel to i2s and spdif cards, which user app accesses and configure and 
> about
> scenarios with hpd.

With ASoC you can build complex sound card consisting out of multiple
components. E.g. SPDIF DAI + HDMI codec is a sound card and the settings
applied to that sound card will be send to both the SPDIF DAI and the HDMI
codec, so you can be sure that both have a matching config. You need to have
a ASoC board driver though which describes how DAI and codec are
interconnected, but you'll need that anyway to get a sound card for the
SPDIF controller instantiated, so that should not be a problem.

> 
> Kyungmin, about adding ioctl; it looks clean, but as Rob stated, is it a 
> right way as drm
> framework is primarily meant for gfx/video configurations? Please exaplin me 
> this point.

>From a technical point of view using a custom IOCTL for this is a bad idea,
since you'll require special tools to make sound over HDMI work, while by
using the method described above the standard tools will work out of the box.

- Lars

Re: exynos drm hdmi audio: how to recieve audio parameters (sf, bps, channel count)

[Lars-Peter Clausen]

On 07/20/2012 01:44 PM, RAHUL SHARMA wrote:
 
 On Fri, Jul 20, 2012 at 11:40 AM, RAHUL SHARMA rahul.sha...@samsung.com 
 wrote:


 --- Original Message ---
 Sender : Lars-Peter Clausenl...@metafoo.de
 Date : Jul 19, 2012 19:12 (GMT+05:30)
 Title : Re: exynos drm hdmi audio: how to recieve audio parameters (sf, bps, 
 channel count)

 On 07/19/2012 02:22 PM, Rob Clark wrote:
 On Tue, Jul 17, 2012 at 1:12 AM, RAHUL SHARMA wrote:
 hi,

 I am adding support for hdmi audio, inside exynos drm hdmi driver. Hdmi 
 audio is initialized with default parameters. I want to implement the 
 mechanism to update hdmi registers, whenever audio properties got changed 
 (i2s/spdif). raedon/r600 drm dirver is polling the audio ip every 100 ms 
 and reconfigure the hdmi audio block. This is not possible with exynos as 
 all information cannot be collected from i2s tx registers. It is directly 
 set on wm8994 connected through i2c.
 Possible solution:
 1) drm driver exposing ioctl for setting audio parameters.
 2) alsa driver notifying the change in audio parameters through kernel 
 notifiers. drm hdmi driver subscribed for the same.

 I am certainly not an audio expert, but I am pretty sure something
 along the lines of solution #2 would be better.  I don't think
 userspace would want to know about some exynos drm specific ioctls in
 order to make audio work.

 BR,
 -R


 I don't know how the audio setup for exynos HDMI hardware looks like, so
 this might not be applicable in this case. But what I did for HDMI
 transmitter I was working on, is to register a ASoC codec device as a
 subdevice to the HDMI transmitter. All access to the sound registers is done
 from that subdevice and you don't need any special notifier hooks, since you
 can use the normal alsa driver callbacks.

 It might be a good idea if you could describe how your setup looks exactly.
 E.g. which components does the audio stream pass through, where does it
 originate and how the different components related to each other.

 - Lars
 
 Thanks Lars, Rob, Kyungmin,
 
 In exynos, i2s and spdif DAIs configure the i2s, spdif tx IP regs. In hw, I2s 
 and
 spdif tx o/p pins are also connected to the i2s-In and spdif-In blocks of 
 hdmi tx.
 Drm hdmi sub driver needs to configure the i2s-In and spdif-In blocks with the
 same setting as in i2s and spdif tx IPs. Hdmi is solely accessed by drm driver
 where hdmi is registered as a sub-driver along with lcd sub-driver. I hope, I 
 am
 able to explain the flow and connections, through above.

With this setup, I think, implementing the HDMI audio as a ASoC codec driver
is the right approach.

 
 Lars, how can I refer your implementation where Adding 'hdmi audio' as a 
 seperate
 subdrv. If I understand correctly, it will be exposed as a seperate sound 
 card,
 parallel to i2s and spdif cards, which user app accesses and configure and 
 about
 scenarios with hpd.

With ASoC you can build complex sound card consisting out of multiple
components. E.g. SPDIF DAI + HDMI codec is a sound card and the settings
applied to that sound card will be send to both the SPDIF DAI and the HDMI
codec, so you can be sure that both have a matching config. You need to have
a ASoC board driver though which describes how DAI and codec are
interconnected, but you'll need that anyway to get a sound card for the
SPDIF controller instantiated, so that should not be a problem.

 
 Kyungmin, about adding ioctl; it looks clean, but as Rob stated, is it a 
 right way as drm
 framework is primarily meant for gfx/video configurations? Please exaplin me 
 this point.

From a technical point of view using a custom IOCTL for this is a bad idea,
since you'll require special tools to make sound over HDMI work, while by
using the method described above the standard tools will work out of the box.

- Lars
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 05:46 PM, Laurent Pinchart wrote:
> Hi Lars,
> 
> On Thursday 19 July 2012 17:12:28 Lars-Peter Clausen wrote:
>> On 07/19/2012 04:57 PM, Lars-Peter Clausen wrote:
>>> On 07/19/2012 04:41 PM, Laurent Pinchart wrote:
>>>> On Thursday 19 July 2012 16:02:41 Laurent Pinchart wrote:
>>>>> On Thursday 19 July 2012 15:55:56 Lars-Peter Clausen wrote:
>>>>>> On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
>>>>>>> On Thursday 19 July 2012 15:34:26 Lars-Peter Clausen wrote:
>>>>>>>> On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
>>>>>>>>> On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
>>>>>>>>>> This patchset introduces a set of helper function for implementing
>>>>>>>>>> the KMS framebuffer layer for drivers which use the drm gem CMA
>>>>>>>>>> helper function.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Lars-Peter Clausen 
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>> Note: This patch depends on Sascha's "DRM: add drm gem CMA helper"
>>>>>>>>>> patch
>>>>>>>>>>
>>>>>>>>>> Changes since v2:
>>>>>>>>>>  * Adapt to changes in the GEM CMA helper
>>>>>>>>>>  * Add basic buffer size checking in drm_fb_cma_create
>>>>>>>>>>
>>>>>>>>>> Changes since v1:
>>>>>>>>>>  * Some spelling fixes
>>>>>>>>>>  * Add missing kfree in drm_fb_cma_alloc error path
>>>>>>>>>>  * Add multi-plane support
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>>  drivers/gpu/drm/Kconfig |   10 +
>>>>>>>>>>  drivers/gpu/drm/Makefile|1 +
>>>>>>>>>>  drivers/gpu/drm/drm_fb_cma_helper.c |  393
>>>>>>>>>>  +++
>>>>>>>>>>  include/drm/drm_fb_cma_helper.h |   27 +++
>>>>>>>>>>  4 files changed, 431 insertions(+)
>>>>>>>>>>  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>>>>>  create mode 100644 include/drm/drm_fb_cma_helper.h
>>>>>>>>>
>>>>>>>>> [snip]
>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>>>>> b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
>>>>>>>>>> index 000..9042233
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>>>>
>>>>>>>>> [snip]
>>>>>>>>>
>>>>>>>>>> +/**
>>>>>>>>>> + * drm_fb_cma_create() - (struct drm_mode_config_funcs
>>>>>>>>>> *)->fb_create
>>>>>>>>>> callback function
>>>>>>>>>> + *
>>>>>>>>>> + * If your hardware has special alignment or pitch requirements
>>>>>>>>>> these
>>>>>>>>>> should be
>>>>>>>>>> + * checked before calling this function.
>>>>>>>>>> + */
>>>>>>>>>> +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>>>>>>>>>> +struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
>>>>>>>>>> +{
>>>>>>>>>> +struct drm_fb_cma *fb_cma;
>>>>>>>>>> +struct drm_gem_cma_object *objs[4];
>>>>>>>>>> +struct drm_gem_object *obj;
>>>>>>>>>> +int ret;
>>>>>>>>>> +int i;
>>>>>>>>>> +
>>>>>>>>>> +for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format);
>>>>
>>>> i++)
>>>>
>>>>>>>>>> {
>>>>>>>>>> +

[PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 05:01 PM, Laurent Pinchart wrote:
> Hi Lars,
> 
> On Thursday 19 July 2012 16:57:15 Lars-Peter Clausen wrote:
>> On 07/19/2012 04:41 PM, Laurent Pinchart wrote:
>>> On Thursday 19 July 2012 16:02:41 Laurent Pinchart wrote:
>>>> On Thursday 19 July 2012 15:55:56 Lars-Peter Clausen wrote:
>>>>> On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
>>>>>> On Thursday 19 July 2012 15:34:26 Lars-Peter Clausen wrote:
>>>>>>> On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
>>>>>>>> On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
>>>>>>>>> This patchset introduces a set of helper function for implementing
>>>>>>>>> the KMS framebuffer layer for drivers which use the drm gem CMA
>>>>>>>>> helper function.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Lars-Peter Clausen 
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> Note: This patch depends on Sascha's "DRM: add drm gem CMA helper"
>>>>>>>>> patch
>>>>>>>>>
>>>>>>>>> Changes since v2:
>>>>>>>>>   * Adapt to changes in the GEM CMA helper
>>>>>>>>>   * Add basic buffer size checking in drm_fb_cma_create
>>>>>>>>>
>>>>>>>>> Changes since v1:
>>>>>>>>>   * Some spelling fixes
>>>>>>>>>   * Add missing kfree in drm_fb_cma_alloc error path
>>>>>>>>>   * Add multi-plane support
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>  drivers/gpu/drm/Kconfig |   10 +
>>>>>>>>>  drivers/gpu/drm/Makefile|1 +
>>>>>>>>>  drivers/gpu/drm/drm_fb_cma_helper.c |  393
>>>>>>>>>  +++
>>>>>>>>>  include/drm/drm_fb_cma_helper.h |   27 +++
>>>>>>>>>  4 files changed, 431 insertions(+)
>>>>>>>>>  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>>>>  create mode 100644 include/drm/drm_fb_cma_helper.h
>>>>>>>>
>>>>>>>> [snip]
>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>>>> b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
>>>>>>>>> index 000..9042233
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>>>
>>>>>>>> [snip]
>>>>>>>>
>>>>>>>>> +/**
>>>>>>>>> + * drm_fb_cma_create() - (struct drm_mode_config_funcs
>>>>>>>>> *)->fb_create
>>>>>>>>> callback function
>>>>>>>>> + *
>>>>>>>>> + * If your hardware has special alignment or pitch requirements
>>>>>>>>> these
>>>>>>>>> should be
>>>>>>>>> + * checked before calling this function.
>>>>>>>>> + */
>>>>>>>>> +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>>>>>>>>> + struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
>>>>>>>>> +{
>>>>>>>>> + struct drm_fb_cma *fb_cma;
>>>>>>>>> + struct drm_gem_cma_object *objs[4];
>>>>>>>>> + struct drm_gem_object *obj;
>>>>>>>>> + int ret;
>>>>>>>>> + int i;
>>>>>>>>> +
>>>>>>>>> + for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format);
>>>
>>> i++)
>>>
>>>>>>>>> {
>>>>>>>>> + obj = drm_gem_object_lookup(dev, file_priv, mode_cmd-
>>>>>>>
>>>>>>> handles[i]);
>>>>>>>
>>>>>>>>> + if (!obj) {
>>>>>>>>> + dev_err(dev->dev, "Failed to lookup GEM 
&

[PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 04:57 PM, Lars-Peter Clausen wrote:
> On 07/19/2012 04:41 PM, Laurent Pinchart wrote:
>> On Thursday 19 July 2012 16:02:41 Laurent Pinchart wrote:
>>> On Thursday 19 July 2012 15:55:56 Lars-Peter Clausen wrote:
>>>> On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
>>>>> On Thursday 19 July 2012 15:34:26 Lars-Peter Clausen wrote:
>>>>>> On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
>>>>>>> On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
>>>>>>>> This patchset introduces a set of helper function for implementing
>>>>>>>> the KMS framebuffer layer for drivers which use the drm gem CMA
>>>>>>>> helper function.
>>>>>>>>
>>>>>>>> Signed-off-by: Lars-Peter Clausen 
>>>>>>>>
>>>>>>>> ---
>>>>>>>> Note: This patch depends on Sascha's "DRM: add drm gem CMA helper"
>>>>>>>> patch
>>>>>>>>
>>>>>>>> Changes since v2:
>>>>>>>>* Adapt to changes in the GEM CMA helper
>>>>>>>>* Add basic buffer size checking in drm_fb_cma_create
>>>>>>>>
>>>>>>>> Changes since v1:
>>>>>>>>* Some spelling fixes
>>>>>>>>* Add missing kfree in drm_fb_cma_alloc error path
>>>>>>>>* Add multi-plane support
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>  drivers/gpu/drm/Kconfig |   10 +
>>>>>>>>  drivers/gpu/drm/Makefile|1 +
>>>>>>>>  drivers/gpu/drm/drm_fb_cma_helper.c |  393
>>>>>>>>  +++
>>>>>>>>  include/drm/drm_fb_cma_helper.h |   27 +++
>>>>>>>>  4 files changed, 431 insertions(+)
>>>>>>>>  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>>>  create mode 100644 include/drm/drm_fb_cma_helper.h
>>>>>>>
>>>>>>> [snip]
>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>>> b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
>>>>>>>> index 000..9042233
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>>
>>>>>>> [snip]
>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create
>>>>>>>> callback function
>>>>>>>> + *
>>>>>>>> + * If your hardware has special alignment or pitch requirements
>>>>>>>> these
>>>>>>>> should be
>>>>>>>> + * checked before calling this function.
>>>>>>>> + */
>>>>>>>> +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>>>>>>>> +  struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
>>>>>>>> +{
>>>>>>>> +  struct drm_fb_cma *fb_cma;
>>>>>>>> +  struct drm_gem_cma_object *objs[4];
>>>>>>>> +  struct drm_gem_object *obj;
>>>>>>>> +  int ret;
>>>>>>>> +  int i;
>>>>>>>> +
>>>>>>>> +  for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); 
>> i++)
>>>>>>>> {
>>>>>>>> +  obj = drm_gem_object_lookup(dev, file_priv, mode_cmd-
>>>>>>
>>>>>> handles[i]);
>>>>>>
>>>>>>>> +  if (!obj) {
>>>>>>>> +  dev_err(dev->dev, "Failed to lookup GEM 
>>>>>>>> object\n");
>>>>>>>> +  ret = -ENXIO;
>>>>>>>> +  goto err_gem_object_unreference;
>>>>>>>> +  }
>>>>>>>> +
>>>>>>>> +  if (obj->size < mode_cmd->height * 
>>>>>>>> mode_cmd->

[PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 04:57 PM, Lars-Peter Clausen wrote:
> And well something similar for horizontal subsampling.

s/horizontal/vertical/

[PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 04:41 PM, Laurent Pinchart wrote:
> On Thursday 19 July 2012 16:02:41 Laurent Pinchart wrote:
>> On Thursday 19 July 2012 15:55:56 Lars-Peter Clausen wrote:
>>> On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
>>>> On Thursday 19 July 2012 15:34:26 Lars-Peter Clausen wrote:
>>>>> On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
>>>>>> On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
>>>>>>> This patchset introduces a set of helper function for implementing
>>>>>>> the KMS framebuffer layer for drivers which use the drm gem CMA
>>>>>>> helper function.
>>>>>>>
>>>>>>> Signed-off-by: Lars-Peter Clausen 
>>>>>>>
>>>>>>> ---
>>>>>>> Note: This patch depends on Sascha's "DRM: add drm gem CMA helper"
>>>>>>> patch
>>>>>>>
>>>>>>> Changes since v2:
>>>>>>> * Adapt to changes in the GEM CMA helper
>>>>>>> * Add basic buffer size checking in drm_fb_cma_create
>>>>>>>
>>>>>>> Changes since v1:
>>>>>>> * Some spelling fixes
>>>>>>> * Add missing kfree in drm_fb_cma_alloc error path
>>>>>>> * Add multi-plane support
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>>  drivers/gpu/drm/Kconfig |   10 +
>>>>>>>  drivers/gpu/drm/Makefile|1 +
>>>>>>>  drivers/gpu/drm/drm_fb_cma_helper.c |  393
>>>>>>>  +++
>>>>>>>  include/drm/drm_fb_cma_helper.h |   27 +++
>>>>>>>  4 files changed, 431 insertions(+)
>>>>>>>  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>>  create mode 100644 include/drm/drm_fb_cma_helper.h
>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>> b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
>>>>>>> index 000..9042233
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>> +/**
>>>>>>> + * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create
>>>>>>> callback function
>>>>>>> + *
>>>>>>> + * If your hardware has special alignment or pitch requirements
>>>>>>> these
>>>>>>> should be
>>>>>>> + * checked before calling this function.
>>>>>>> + */
>>>>>>> +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>>>>>>> +   struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
>>>>>>> +{
>>>>>>> +   struct drm_fb_cma *fb_cma;
>>>>>>> +   struct drm_gem_cma_object *objs[4];
>>>>>>> +   struct drm_gem_object *obj;
>>>>>>> +   int ret;
>>>>>>> +   int i;
>>>>>>> +
>>>>>>> +   for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); 
> i++)
>>>>>>> {
>>>>>>> +   obj = drm_gem_object_lookup(dev, file_priv, mode_cmd-
>>>>>
>>>>> handles[i]);
>>>>>
>>>>>>> +   if (!obj) {
>>>>>>> +   dev_err(dev->dev, "Failed to lookup GEM 
>>>>>>> object\n");
>>>>>>> +   ret = -ENXIO;
>>>>>>> +   goto err_gem_object_unreference;
>>>>>>> +   }
>>>>>>> +
>>>>>>> +   if (obj->size < mode_cmd->height * 
>>>>>>> mode_cmd->pitches[i]) {
>>>>>>
>>>>>> Shouldn't this be
>>>>>>
>>>>>> if (obj->size < mode_cmd->height * mode_cmd->pitches[i]
>>>>>>
>>>>>>   + mode_cmd->offsets[i])
>>>>>>
>>>>>> ?
>

[PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
> Hi Lars,
> 
> On Thursday 19 July 2012 15:34:26 Lars-Peter Clausen wrote:
>> On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
>>> On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
>>>> This patchset introduces a set of helper function for implementing the
>>>> KMS
>>>> framebuffer layer for drivers which use the drm gem CMA helper function.
>>>>
>>>> Signed-off-by: Lars-Peter Clausen 
>>>>
>>>> ---
>>>> Note: This patch depends on Sascha's "DRM: add drm gem CMA helper" patch
>>>>
>>>> Changes since v2:
>>>>* Adapt to changes in the GEM CMA helper
>>>>* Add basic buffer size checking in drm_fb_cma_create
>>>>
>>>> Changes since v1:
>>>>* Some spelling fixes
>>>>* Add missing kfree in drm_fb_cma_alloc error path
>>>>* Add multi-plane support
>>>>
>>>> ---
>>>>
>>>>  drivers/gpu/drm/Kconfig |   10 +
>>>>  drivers/gpu/drm/Makefile|1 +
>>>>  drivers/gpu/drm/drm_fb_cma_helper.c |  393 +
>>>>  include/drm/drm_fb_cma_helper.h |   27 +++
>>>>  4 files changed, 431 insertions(+)
>>>>  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
>>>>  create mode 100644 include/drm/drm_fb_cma_helper.h
>>>
>>> [snip]
>>>
>>>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>>>> b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
>>>> index 000..9042233
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>>>
>>> [snip]
>>>
>>>> +/**
>>>> + * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create
>>>> callback function
>>>> + *
>>>> + * If your hardware has special alignment or pitch requirements these
>>>> should be
>>>> + * checked before calling this function.
>>>> + */
>>>> +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>>>> +  struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
>>>> +{
>>>> +  struct drm_fb_cma *fb_cma;
>>>> +  struct drm_gem_cma_object *objs[4];
>>>> +  struct drm_gem_object *obj;
>>>> +  int ret;
>>>> +  int i;
>>>> +
>>>> +  for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
>>>> +  obj = drm_gem_object_lookup(dev, file_priv, mode_cmd-
>> handles[i]);
>>>> +  if (!obj) {
>>>> +  dev_err(dev->dev, "Failed to lookup GEM object\n");
>>>> +  ret = -ENXIO;
>>>> +  goto err_gem_object_unreference;
>>>> +  }
>>>> +
>>>> +  if (obj->size < mode_cmd->height * mode_cmd->pitches[i]) {
>>>
>>> Shouldn't this be
>>>
>>> if (obj->size < mode_cmd->height * mode_cmd->pitches[i]
>>>
>>>   + mode_cmd->offsets[i])
>>>
>>> ?
>>
>> That's actually a good question. I'd expect the offset to be included in the
>> pitch.
>>
>> If you access pixels like mem[offset + x * bpp + y * pitch] then pitch has
>> to be greater equal to offset + max_x * bpp, otherwise you'd have
>> overlapping lines.
> 
> My understanding is that the offset is a linear offset from the start of the 
> buffer to allow X/Y panning. In that case the pitch is a frame buffer 
> property 
> that is not be influenced by the offset at all.
> 

Hi,

I think panning is normally done by setting the x and y offset for the crtc.

But yes, you are right the offset might just be a linear offset to the start
of the actual data. I was just thinking about the case where the different
planes are interleaved. But for panning or non-interleaved planes
it obviously is different.

Though this leaves us with a problem. If the planes are interleaved and the
offset is included in the pitch your check may fail, even though the buffer
is large enough.

Maybe we need to handle both cases differently. If offset < pitch check for
obj->size < mode_cmd->height * mode_cmd->pitches[i] otherwise check for
obj->size < mode_cmd->height * mode_cmd->pitches[i] + mode_cmd->offsets[i]

But that doesn't quite work either if you have both interleaved planes and a
linear offset...

- Lars

exynos drm hdmi audio: how to recieve audio parameters (sf, bps, channel count)

[Lars-Peter Clausen]

On 07/19/2012 02:22 PM, Rob Clark wrote:
> On Tue, Jul 17, 2012 at 1:12 AM, RAHUL SHARMA  
> wrote:
>> hi,
>>
>> I am adding support for hdmi audio, inside exynos drm hdmi driver. Hdmi 
>> audio is initialized with default parameters. I want to implement the 
>> mechanism to update hdmi registers, whenever audio properties got changed 
>> (i2s/spdif). raedon/r600 drm dirver is polling the audio ip every 100 ms and 
>> reconfigure the hdmi audio block. This is not possible with exynos as all 
>> information cannot be collected from i2s tx registers. It is directly set on 
>> wm8994 connected through i2c.
>> Possible solution:
>> 1) drm driver exposing ioctl for setting audio parameters.
>> 2) alsa driver notifying the change in audio parameters through kernel 
>> notifiers. drm hdmi driver subscribed for the same.
> 
> I am certainly not an audio expert, but I am pretty sure something
> along the lines of solution #2 would be better.  I don't think
> userspace would want to know about some exynos drm specific ioctls in
> order to make audio work.
> 
> BR,
> -R


I don't know how the audio setup for exynos HDMI hardware looks like, so
this might not be applicable in this case. But what I did for HDMI
transmitter I was working on, is to register a ASoC codec device as a
subdevice to the HDMI transmitter. All access to the sound registers is done
from that subdevice and you don't need any special notifier hooks, since you
can use the normal alsa driver callbacks.

It might be a good idea if you could describe how your setup looks exactly.
E.g. which components does the audio stream pass through, where does it
originate and how the different components related to each other.

- Lars

[PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
> Hi Lars,
> 
> Thank you for the patch.
> 
> On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
>> This patchset introduces a set of helper function for implementing the KMS
>> framebuffer layer for drivers which use the drm gem CMA helper function.
>>
>> Signed-off-by: Lars-Peter Clausen 
>>
>> ---
>> Note: This patch depends on Sascha's "DRM: add drm gem CMA helper" patch
>>
>> Changes since v2:
>>  * Adapt to changes in the GEM CMA helper
>>  * Add basic buffer size checking in drm_fb_cma_create
>> Changes since v1:
>>  * Some spelling fixes
>>  * Add missing kfree in drm_fb_cma_alloc error path
>>  * Add multi-plane support
>> ---
>>  drivers/gpu/drm/Kconfig |   10 +
>>  drivers/gpu/drm/Makefile|1 +
>>  drivers/gpu/drm/drm_fb_cma_helper.c |  393 
>>  include/drm/drm_fb_cma_helper.h |   27 +++
>>  4 files changed, 431 insertions(+)
>>  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
>>  create mode 100644 include/drm/drm_fb_cma_helper.h
> 
> [snip]
> 
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>> b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
>> index 000..9042233
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> 
> [snip]
> 
>> +/**
>> + * drm_fb_cma_create() - (struct drm_mode_config_funcs *)->fb_create
>> callback function
>> + *
>> + * If your hardware has special alignment or pitch requirements these
>> should be
>> + * checked before calling this function.
>> + */
>> +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
>> +struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
>> +{
>> +struct drm_fb_cma *fb_cma;
>> +struct drm_gem_cma_object *objs[4];
>> +struct drm_gem_object *obj;
>> +int ret;
>> +int i;
>> +
>> +for (i = 0; i < drm_format_num_planes(mode_cmd->pixel_format); i++) {
>> +obj = drm_gem_object_lookup(dev, file_priv, 
>> mode_cmd->handles[i]);
>> +if (!obj) {
>> +dev_err(dev->dev, "Failed to lookup GEM object\n");
>> +ret = -ENXIO;
>> +goto err_gem_object_unreference;
>> +}
>> +
>> +if (obj->size < mode_cmd->height * mode_cmd->pitches[i]) {
> 
> Shouldn't this be
> 
> if (obj->size < mode_cmd->height * mode_cmd->pitches[i]
>   + mode_cmd->offsets[i])
> 
> ?

That's actually a good question. I'd expect the offset to be included in the
pitch.

If you access pixels like mem[offset + x * bpp + y * pitch] then pitch has
to be greater equal to offset + max_x * bpp, otherwise you'd have
overlapping lines.

Thanks,
- Lars

Re: [PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
 Hi Lars,
 
 Thank you for the patch.
 
 On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
 This patchset introduces a set of helper function for implementing the KMS
 framebuffer layer for drivers which use the drm gem CMA helper function.

 Signed-off-by: Lars-Peter Clausen l...@metafoo.de

 ---
 Note: This patch depends on Sascha's DRM: add drm gem CMA helper patch

 Changes since v2:
  * Adapt to changes in the GEM CMA helper
  * Add basic buffer size checking in drm_fb_cma_create
 Changes since v1:
  * Some spelling fixes
  * Add missing kfree in drm_fb_cma_alloc error path
  * Add multi-plane support
 ---
  drivers/gpu/drm/Kconfig |   10 +
  drivers/gpu/drm/Makefile|1 +
  drivers/gpu/drm/drm_fb_cma_helper.c |  393 
  include/drm/drm_fb_cma_helper.h |   27 +++
  4 files changed, 431 insertions(+)
  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
  create mode 100644 include/drm/drm_fb_cma_helper.h
 
 [snip]
 
 diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
 b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
 index 000..9042233
 --- /dev/null
 +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
 
 [snip]
 
 +/**
 + * drm_fb_cma_create() - (struct drm_mode_config_funcs *)-fb_create
 callback function
 + *
 + * If your hardware has special alignment or pitch requirements these
 should be
 + * checked before calling this function.
 + */
 +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
 +struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
 +{
 +struct drm_fb_cma *fb_cma;
 +struct drm_gem_cma_object *objs[4];
 +struct drm_gem_object *obj;
 +int ret;
 +int i;
 +
 +for (i = 0; i  drm_format_num_planes(mode_cmd-pixel_format); i++) {
 +obj = drm_gem_object_lookup(dev, file_priv, 
 mode_cmd-handles[i]);
 +if (!obj) {
 +dev_err(dev-dev, Failed to lookup GEM object\n);
 +ret = -ENXIO;
 +goto err_gem_object_unreference;
 +}
 +
 +if (obj-size  mode_cmd-height * mode_cmd-pitches[i]) {
 
 Shouldn't this be
 
 if (obj-size  mode_cmd-height * mode_cmd-pitches[i]
   + mode_cmd-offsets[i])
 
 ?

That's actually a good question. I'd expect the offset to be included in the
pitch.

If you access pixels like mem[offset + x * bpp + y * pitch] then pitch has
to be greater equal to offset + max_x * bpp, otherwise you'd have
overlapping lines.

Thanks,
- Lars

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: exynos drm hdmi audio: how to recieve audio parameters (sf, bps, channel count)

[Lars-Peter Clausen]

On 07/19/2012 02:22 PM, Rob Clark wrote:
 On Tue, Jul 17, 2012 at 1:12 AM, RAHUL SHARMA rahul.sha...@samsung.com 
 wrote:
 hi,

 I am adding support for hdmi audio, inside exynos drm hdmi driver. Hdmi 
 audio is initialized with default parameters. I want to implement the 
 mechanism to update hdmi registers, whenever audio properties got changed 
 (i2s/spdif). raedon/r600 drm dirver is polling the audio ip every 100 ms and 
 reconfigure the hdmi audio block. This is not possible with exynos as all 
 information cannot be collected from i2s tx registers. It is directly set on 
 wm8994 connected through i2c.
 Possible solution:
 1) drm driver exposing ioctl for setting audio parameters.
 2) alsa driver notifying the change in audio parameters through kernel 
 notifiers. drm hdmi driver subscribed for the same.
 
 I am certainly not an audio expert, but I am pretty sure something
 along the lines of solution #2 would be better.  I don't think
 userspace would want to know about some exynos drm specific ioctls in
 order to make audio work.
 
 BR,
 -R


I don't know how the audio setup for exynos HDMI hardware looks like, so
this might not be applicable in this case. But what I did for HDMI
transmitter I was working on, is to register a ASoC codec device as a
subdevice to the HDMI transmitter. All access to the sound registers is done
from that subdevice and you don't need any special notifier hooks, since you
can use the normal alsa driver callbacks.

It might be a good idea if you could describe how your setup looks exactly.
E.g. which components does the audio stream pass through, where does it
originate and how the different components related to each other.

- Lars
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
 Hi Lars,
 
 On Thursday 19 July 2012 15:34:26 Lars-Peter Clausen wrote:
 On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
 On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
 This patchset introduces a set of helper function for implementing the
 KMS
 framebuffer layer for drivers which use the drm gem CMA helper function.

 Signed-off-by: Lars-Peter Clausen l...@metafoo.de

 ---
 Note: This patch depends on Sascha's DRM: add drm gem CMA helper patch

 Changes since v2:
* Adapt to changes in the GEM CMA helper
* Add basic buffer size checking in drm_fb_cma_create

 Changes since v1:
* Some spelling fixes
* Add missing kfree in drm_fb_cma_alloc error path
* Add multi-plane support

 ---

  drivers/gpu/drm/Kconfig |   10 +
  drivers/gpu/drm/Makefile|1 +
  drivers/gpu/drm/drm_fb_cma_helper.c |  393 +
  include/drm/drm_fb_cma_helper.h |   27 +++
  4 files changed, 431 insertions(+)
  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
  create mode 100644 include/drm/drm_fb_cma_helper.h

 [snip]

 diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
 b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
 index 000..9042233
 --- /dev/null
 +++ b/drivers/gpu/drm/drm_fb_cma_helper.c

 [snip]

 +/**
 + * drm_fb_cma_create() - (struct drm_mode_config_funcs *)-fb_create
 callback function
 + *
 + * If your hardware has special alignment or pitch requirements these
 should be
 + * checked before calling this function.
 + */
 +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
 +  struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
 +{
 +  struct drm_fb_cma *fb_cma;
 +  struct drm_gem_cma_object *objs[4];
 +  struct drm_gem_object *obj;
 +  int ret;
 +  int i;
 +
 +  for (i = 0; i  drm_format_num_planes(mode_cmd-pixel_format); i++) {
 +  obj = drm_gem_object_lookup(dev, file_priv, mode_cmd-
 handles[i]);
 +  if (!obj) {
 +  dev_err(dev-dev, Failed to lookup GEM object\n);
 +  ret = -ENXIO;
 +  goto err_gem_object_unreference;
 +  }
 +
 +  if (obj-size  mode_cmd-height * mode_cmd-pitches[i]) {

 Shouldn't this be

 if (obj-size  mode_cmd-height * mode_cmd-pitches[i]

   + mode_cmd-offsets[i])

 ?

 That's actually a good question. I'd expect the offset to be included in the
 pitch.

 If you access pixels like mem[offset + x * bpp + y * pitch] then pitch has
 to be greater equal to offset + max_x * bpp, otherwise you'd have
 overlapping lines.
 
 My understanding is that the offset is a linear offset from the start of the 
 buffer to allow X/Y panning. In that case the pitch is a frame buffer 
 property 
 that is not be influenced by the offset at all.
 

Hi,

I think panning is normally done by setting the x and y offset for the crtc.

But yes, you are right the offset might just be a linear offset to the start
of the actual data. I was just thinking about the case where the different
planes are interleaved. But for panning or non-interleaved planes
it obviously is different.

Though this leaves us with a problem. If the planes are interleaved and the
offset is included in the pitch your check may fail, even though the buffer
is large enough.

Maybe we need to handle both cases differently. If offset  pitch check for
obj-size  mode_cmd-height * mode_cmd-pitches[i] otherwise check for
obj-size  mode_cmd-height * mode_cmd-pitches[i] + mode_cmd-offsets[i]

But that doesn't quite work either if you have both interleaved planes and a
linear offset...

- Lars
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 04:41 PM, Laurent Pinchart wrote:
 On Thursday 19 July 2012 16:02:41 Laurent Pinchart wrote:
 On Thursday 19 July 2012 15:55:56 Lars-Peter Clausen wrote:
 On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
 On Thursday 19 July 2012 15:34:26 Lars-Peter Clausen wrote:
 On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
 On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
 This patchset introduces a set of helper function for implementing
 the KMS framebuffer layer for drivers which use the drm gem CMA
 helper function.

 Signed-off-by: Lars-Peter Clausen l...@metafoo.de

 ---
 Note: This patch depends on Sascha's DRM: add drm gem CMA helper
 patch

 Changes since v2:
 * Adapt to changes in the GEM CMA helper
 * Add basic buffer size checking in drm_fb_cma_create

 Changes since v1:
 * Some spelling fixes
 * Add missing kfree in drm_fb_cma_alloc error path
 * Add multi-plane support

 ---

  drivers/gpu/drm/Kconfig |   10 +
  drivers/gpu/drm/Makefile|1 +
  drivers/gpu/drm/drm_fb_cma_helper.c |  393
  +++
  include/drm/drm_fb_cma_helper.h |   27 +++
  4 files changed, 431 insertions(+)
  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
  create mode 100644 include/drm/drm_fb_cma_helper.h

 [snip]

 diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
 b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
 index 000..9042233
 --- /dev/null
 +++ b/drivers/gpu/drm/drm_fb_cma_helper.c

 [snip]

 +/**
 + * drm_fb_cma_create() - (struct drm_mode_config_funcs *)-fb_create
 callback function
 + *
 + * If your hardware has special alignment or pitch requirements
 these
 should be
 + * checked before calling this function.
 + */
 +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
 +   struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
 +{
 +   struct drm_fb_cma *fb_cma;
 +   struct drm_gem_cma_object *objs[4];
 +   struct drm_gem_object *obj;
 +   int ret;
 +   int i;
 +
 +   for (i = 0; i  drm_format_num_planes(mode_cmd-pixel_format); 
 i++)
 {
 +   obj = drm_gem_object_lookup(dev, file_priv, mode_cmd-

 handles[i]);

 +   if (!obj) {
 +   dev_err(dev-dev, Failed to lookup GEM 
 object\n);
 +   ret = -ENXIO;
 +   goto err_gem_object_unreference;
 +   }
 +
 +   if (obj-size  mode_cmd-height * 
 mode_cmd-pitches[i]) {

 Shouldn't this be

 if (obj-size  mode_cmd-height * mode_cmd-pitches[i]

   + mode_cmd-offsets[i])

 ?

 That's actually a good question. I'd expect the offset to be included
 in the pitch.

 If you access pixels like mem[offset + x * bpp + y * pitch] then pitch
 has to be greater equal to offset + max_x * bpp, otherwise you'd have
 overlapping lines.

 My understanding is that the offset is a linear offset from the start of
 the buffer to allow X/Y panning. In that case the pitch is a frame
 buffer property that is not be influenced by the offset at all.

 Hi,

 I think panning is normally done by setting the x and y offset for the
 crtc.

 But yes, you are right the offset might just be a linear offset to the
 start of the actual data. I was just thinking about the case where the
 different planes are interleaved. But for panning or non-interleaved
 planes it obviously is different.

 Though this leaves us with a problem. If the planes are interleaved and
 the offset is included in the pitch your check may fail, even though the
 buffer is large enough.

 Maybe we need to handle both cases differently. If offset  pitch check
 for
 obj-size  mode_cmd-height * mode_cmd-pitches[i] otherwise check for
 obj-size  mode_cmd-height * mode_cmd-pitches[i] + mode_cmd-offsets[i]

 But that doesn't quite work either if you have both interleaved planes and
 a linear offset...

 What about first finding out what those offsets are supposed to be used for
 ?

 Ville, git blame points to you as the author of the offsets field :-) Could
 you please comment on this ?
 
 My bad, I was looking at drm_framebuffer and not drm_mode_fb_cmd2.
 
 Offset really looks like it is a linear offset from the beginning of the 
 buffer. This allows placing several planes at different locations in a single 
 buffer. I think we need to add mode_cmd-offsets[i] unconditionally when 
 checking the size.
 

There are two cases to consider interleaved and non-interleaved planes in a
single buffer.

a) Separate planes
[Y Y Y Y ]
[Y Y Y Y ]
[ .. ]
[Y Y Y Y ]
[CBCR CBCR CBCR CBCR  ]
[CBCR CBCR CBCR CBCR  ]
[ ... ]
[CBCR CBCR CBCR CBCR  ]

Plane 0:
bpp = 1
offset = 0

Plane 1:
bpp = 1
offset = plane0_size


b) Interleaved planes
[Y CBCR Y CBCR Y CBCR ]
[Y CBCR Y CBCR Y CBCR ]
[ ... ]
[Y CBCR Y CBCR Y CBCR ]

Plane 0:
bbp = 2
offset = 0

Plane 1:
bbp = 2
offset = 1


For case b

Re: [PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 04:57 PM, Lars-Peter Clausen wrote:
 And well something similar for horizontal subsampling.

s/horizontal/vertical/

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 04:57 PM, Lars-Peter Clausen wrote:
 On 07/19/2012 04:41 PM, Laurent Pinchart wrote:
 On Thursday 19 July 2012 16:02:41 Laurent Pinchart wrote:
 On Thursday 19 July 2012 15:55:56 Lars-Peter Clausen wrote:
 On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
 On Thursday 19 July 2012 15:34:26 Lars-Peter Clausen wrote:
 On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
 On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
 This patchset introduces a set of helper function for implementing
 the KMS framebuffer layer for drivers which use the drm gem CMA
 helper function.

 Signed-off-by: Lars-Peter Clausen l...@metafoo.de

 ---
 Note: This patch depends on Sascha's DRM: add drm gem CMA helper
 patch

 Changes since v2:
* Adapt to changes in the GEM CMA helper
* Add basic buffer size checking in drm_fb_cma_create

 Changes since v1:
* Some spelling fixes
* Add missing kfree in drm_fb_cma_alloc error path
* Add multi-plane support

 ---

  drivers/gpu/drm/Kconfig |   10 +
  drivers/gpu/drm/Makefile|1 +
  drivers/gpu/drm/drm_fb_cma_helper.c |  393
  +++
  include/drm/drm_fb_cma_helper.h |   27 +++
  4 files changed, 431 insertions(+)
  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
  create mode 100644 include/drm/drm_fb_cma_helper.h

 [snip]

 diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
 b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
 index 000..9042233
 --- /dev/null
 +++ b/drivers/gpu/drm/drm_fb_cma_helper.c

 [snip]

 +/**
 + * drm_fb_cma_create() - (struct drm_mode_config_funcs *)-fb_create
 callback function
 + *
 + * If your hardware has special alignment or pitch requirements
 these
 should be
 + * checked before calling this function.
 + */
 +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
 +  struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
 +{
 +  struct drm_fb_cma *fb_cma;
 +  struct drm_gem_cma_object *objs[4];
 +  struct drm_gem_object *obj;
 +  int ret;
 +  int i;
 +
 +  for (i = 0; i  drm_format_num_planes(mode_cmd-pixel_format); 
 i++)
 {
 +  obj = drm_gem_object_lookup(dev, file_priv, mode_cmd-

 handles[i]);

 +  if (!obj) {
 +  dev_err(dev-dev, Failed to lookup GEM 
 object\n);
 +  ret = -ENXIO;
 +  goto err_gem_object_unreference;
 +  }
 +
 +  if (obj-size  mode_cmd-height * 
 mode_cmd-pitches[i]) {

 Shouldn't this be

 if (obj-size  mode_cmd-height * mode_cmd-pitches[i]

   + mode_cmd-offsets[i])

 ?

 That's actually a good question. I'd expect the offset to be included
 in the pitch.

 If you access pixels like mem[offset + x * bpp + y * pitch] then pitch
 has to be greater equal to offset + max_x * bpp, otherwise you'd have
 overlapping lines.

 My understanding is that the offset is a linear offset from the start of
 the buffer to allow X/Y panning. In that case the pitch is a frame
 buffer property that is not be influenced by the offset at all.

 Hi,

 I think panning is normally done by setting the x and y offset for the
 crtc.

 But yes, you are right the offset might just be a linear offset to the
 start of the actual data. I was just thinking about the case where the
 different planes are interleaved. But for panning or non-interleaved
 planes it obviously is different.

 Though this leaves us with a problem. If the planes are interleaved and
 the offset is included in the pitch your check may fail, even though the
 buffer is large enough.

 Maybe we need to handle both cases differently. If offset  pitch check
 for
 obj-size  mode_cmd-height * mode_cmd-pitches[i] otherwise check for
 obj-size  mode_cmd-height * mode_cmd-pitches[i] + mode_cmd-offsets[i]

 But that doesn't quite work either if you have both interleaved planes and
 a linear offset...

 What about first finding out what those offsets are supposed to be used for
 ?

 Ville, git blame points to you as the author of the offsets field :-) Could
 you please comment on this ?

 My bad, I was looking at drm_framebuffer and not drm_mode_fb_cmd2.

 Offset really looks like it is a linear offset from the beginning of the 
 buffer. This allows placing several planes at different locations in a 
 single 
 buffer. I think we need to add mode_cmd-offsets[i] unconditionally when 
 checking the size.

 
 There are two cases to consider interleaved and non-interleaved planes in a
 single buffer.
 
 a) Separate planes
 [Y Y Y Y ]
 [Y Y Y Y ]
 [ .. ]
 [Y Y Y Y ]
 [CBCR CBCR CBCR CBCR  ]
 [CBCR CBCR CBCR CBCR  ]
 [ ... ]
 [CBCR CBCR CBCR CBCR  ]
 
 Plane 0:
 bpp = 1
 offset = 0
 
 Plane 1:
 bpp = 1
 offset = plane0_size
 
 
 b) Interleaved planes
 [Y CBCR Y CBCR Y CBCR ]
 [Y CBCR Y CBCR Y CBCR ]
 [ ... ]
 [Y CBCR Y CBCR Y CBCR ]
 
 Plane 0

Re: [PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 05:01 PM, Laurent Pinchart wrote:
 Hi Lars,
 
 On Thursday 19 July 2012 16:57:15 Lars-Peter Clausen wrote:
 On 07/19/2012 04:41 PM, Laurent Pinchart wrote:
 On Thursday 19 July 2012 16:02:41 Laurent Pinchart wrote:
 On Thursday 19 July 2012 15:55:56 Lars-Peter Clausen wrote:
 On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
 On Thursday 19 July 2012 15:34:26 Lars-Peter Clausen wrote:
 On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
 On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
 This patchset introduces a set of helper function for implementing
 the KMS framebuffer layer for drivers which use the drm gem CMA
 helper function.

 Signed-off-by: Lars-Peter Clausen l...@metafoo.de

 ---
 Note: This patch depends on Sascha's DRM: add drm gem CMA helper
 patch

 Changes since v2:
   * Adapt to changes in the GEM CMA helper
   * Add basic buffer size checking in drm_fb_cma_create

 Changes since v1:
   * Some spelling fixes
   * Add missing kfree in drm_fb_cma_alloc error path
   * Add multi-plane support

 ---

  drivers/gpu/drm/Kconfig |   10 +
  drivers/gpu/drm/Makefile|1 +
  drivers/gpu/drm/drm_fb_cma_helper.c |  393
  +++
  include/drm/drm_fb_cma_helper.h |   27 +++
  4 files changed, 431 insertions(+)
  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
  create mode 100644 include/drm/drm_fb_cma_helper.h

 [snip]

 diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
 b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
 index 000..9042233
 --- /dev/null
 +++ b/drivers/gpu/drm/drm_fb_cma_helper.c

 [snip]

 +/**
 + * drm_fb_cma_create() - (struct drm_mode_config_funcs
 *)-fb_create
 callback function
 + *
 + * If your hardware has special alignment or pitch requirements
 these
 should be
 + * checked before calling this function.
 + */
 +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
 + struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
 +{
 + struct drm_fb_cma *fb_cma;
 + struct drm_gem_cma_object *objs[4];
 + struct drm_gem_object *obj;
 + int ret;
 + int i;
 +
 + for (i = 0; i  drm_format_num_planes(mode_cmd-pixel_format);

 i++)

 {
 + obj = drm_gem_object_lookup(dev, file_priv, mode_cmd-

 handles[i]);

 + if (!obj) {
 + dev_err(dev-dev, Failed to lookup GEM 
 object\n);
 + ret = -ENXIO;
 + goto err_gem_object_unreference;
 + }
 +
 + if (obj-size  mode_cmd-height * 
 mode_cmd-pitches[i]) {

 Shouldn't this be

 if (obj-size  mode_cmd-height * mode_cmd-pitches[i]

   + mode_cmd-offsets[i])

 ?

 That's actually a good question. I'd expect the offset to be included
 in the pitch.

 If you access pixels like mem[offset + x * bpp + y * pitch] then pitch
 has to be greater equal to offset + max_x * bpp, otherwise you'd have
 overlapping lines.

 My understanding is that the offset is a linear offset from the start
 of
 the buffer to allow X/Y panning. In that case the pitch is a frame
 buffer property that is not be influenced by the offset at all.

 Hi,

 I think panning is normally done by setting the x and y offset for the
 crtc.

 But yes, you are right the offset might just be a linear offset to the
 start of the actual data. I was just thinking about the case where the
 different planes are interleaved. But for panning or non-interleaved
 planes it obviously is different.

 Though this leaves us with a problem. If the planes are interleaved and
 the offset is included in the pitch your check may fail, even though the
 buffer is large enough.

 Maybe we need to handle both cases differently. If offset  pitch check
 for
 obj-size  mode_cmd-height * mode_cmd-pitches[i] otherwise check for
 obj-size  mode_cmd-height * mode_cmd-pitches[i] +
 mode_cmd-offsets[i]

 But that doesn't quite work either if you have both interleaved planes
 and
 a linear offset...

 What about first finding out what those offsets are supposed to be used
 for
 ?

 Ville, git blame points to you as the author of the offsets field :-)
 Could
 you please comment on this ?

 My bad, I was looking at drm_framebuffer and not drm_mode_fb_cmd2.

 Offset really looks like it is a linear offset from the beginning of the
 buffer. This allows placing several planes at different locations in a
 single buffer. I think we need to add mode_cmd-offsets[i]
 unconditionally when checking the size.

 There are two cases to consider interleaved and non-interleaved planes in a
 single buffer.

 a) Separate planes
 [Y Y Y Y ]
 [Y Y Y Y ]
 [ .. ]
 [Y Y Y Y ]
 [CBCR CBCR CBCR CBCR  ]
 [CBCR CBCR CBCR CBCR  ]
 [ ... ]
 [CBCR CBCR CBCR CBCR  ]

 Plane 0:
 bpp = 1
 offset = 0

 Plane 1:
 bpp = 1
 offset = plane0_size


 b) Interleaved planes
 [Y CBCR Y CBCR Y CBCR ]
 [Y CBCR Y CBCR Y CBCR

Re: [PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 07/19/2012 05:46 PM, Laurent Pinchart wrote:
 Hi Lars,
 
 On Thursday 19 July 2012 17:12:28 Lars-Peter Clausen wrote:
 On 07/19/2012 04:57 PM, Lars-Peter Clausen wrote:
 On 07/19/2012 04:41 PM, Laurent Pinchart wrote:
 On Thursday 19 July 2012 16:02:41 Laurent Pinchart wrote:
 On Thursday 19 July 2012 15:55:56 Lars-Peter Clausen wrote:
 On 07/19/2012 03:33 PM, Laurent Pinchart wrote:
 On Thursday 19 July 2012 15:34:26 Lars-Peter Clausen wrote:
 On 07/19/2012 02:46 PM, Laurent Pinchart wrote:
 On Monday 02 July 2012 16:37:47 Lars-Peter Clausen wrote:
 This patchset introduces a set of helper function for implementing
 the KMS framebuffer layer for drivers which use the drm gem CMA
 helper function.

 Signed-off-by: Lars-Peter Clausen l...@metafoo.de

 ---
 Note: This patch depends on Sascha's DRM: add drm gem CMA helper
 patch

 Changes since v2:
  * Adapt to changes in the GEM CMA helper
  * Add basic buffer size checking in drm_fb_cma_create

 Changes since v1:
  * Some spelling fixes
  * Add missing kfree in drm_fb_cma_alloc error path
  * Add multi-plane support

 ---

  drivers/gpu/drm/Kconfig |   10 +
  drivers/gpu/drm/Makefile|1 +
  drivers/gpu/drm/drm_fb_cma_helper.c |  393
  +++
  include/drm/drm_fb_cma_helper.h |   27 +++
  4 files changed, 431 insertions(+)
  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
  create mode 100644 include/drm/drm_fb_cma_helper.h

 [snip]

 diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
 b/drivers/gpu/drm/drm_fb_cma_helper.c new file mode 100644
 index 000..9042233
 --- /dev/null
 +++ b/drivers/gpu/drm/drm_fb_cma_helper.c

 [snip]

 +/**
 + * drm_fb_cma_create() - (struct drm_mode_config_funcs
 *)-fb_create
 callback function
 + *
 + * If your hardware has special alignment or pitch requirements
 these
 should be
 + * checked before calling this function.
 + */
 +struct drm_framebuffer *drm_fb_cma_create(struct drm_device *dev,
 +struct drm_file *file_priv, struct drm_mode_fb_cmd2 *mode_cmd)
 +{
 +struct drm_fb_cma *fb_cma;
 +struct drm_gem_cma_object *objs[4];
 +struct drm_gem_object *obj;
 +int ret;
 +int i;
 +
 +for (i = 0; i  drm_format_num_planes(mode_cmd-pixel_format);

 i++)

 {
 +obj = drm_gem_object_lookup(dev, file_priv, mode_cmd-

 handles[i]);

 +if (!obj) {
 +dev_err(dev-dev, Failed to lookup GEM 
 object\n);
 +ret = -ENXIO;
 +goto err_gem_object_unreference;
 +}
 +
 +if (obj-size  mode_cmd-height * 
 mode_cmd-pitches[i]) {

 Shouldn't this be

 if (obj-size  mode_cmd-height * mode_cmd-pitches[i]

   + mode_cmd-offsets[i])

 ?

 That's actually a good question. I'd expect the offset to be included
 in the pitch.

 If you access pixels like mem[offset + x * bpp + y * pitch] then
 pitch has to be greater equal to offset + max_x * bpp, otherwise
 you'd have overlapping lines.

 My understanding is that the offset is a linear offset from the start
 of the buffer to allow X/Y panning. In that case the pitch is a frame
 buffer property that is not be influenced by the offset at all.

 Hi,

 I think panning is normally done by setting the x and y offset for the
 crtc.

 But yes, you are right the offset might just be a linear offset to the
 start of the actual data. I was just thinking about the case where the
 different planes are interleaved. But for panning or non-interleaved
 planes it obviously is different.

 Though this leaves us with a problem. If the planes are interleaved and
 the offset is included in the pitch your check may fail, even though
 the buffer is large enough.

 Maybe we need to handle both cases differently. If offset  pitch check
 for
 obj-size  mode_cmd-height * mode_cmd-pitches[i] otherwise check for
 obj-size  mode_cmd-height * mode_cmd-pitches[i] +
 mode_cmd-offsets[i]

 But that doesn't quite work either if you have both interleaved planes
 and a linear offset...

 What about first finding out what those offsets are supposed to be used
 for
 ?

 Ville, git blame points to you as the author of the offsets field :-)
 Could you please comment on this ?

 My bad, I was looking at drm_framebuffer and not drm_mode_fb_cmd2.

 Offset really looks like it is a linear offset from the beginning of the
 buffer. This allows placing several planes at different locations in a
 single buffer. I think we need to add mode_cmd-offsets[i]
 unconditionally when checking the size.

 There are two cases to consider interleaved and non-interleaved planes in
 a single buffer.

 a) Separate planes
 [Y Y Y Y ]
 [Y Y Y Y ]
 [ .. ]
 [Y Y Y Y ]
 [CBCR CBCR CBCR CBCR  ]
 [CBCR CBCR CBCR CBCR  ]
 [ ... ]
 [CBCR CBCR CBCR CBCR  ]

 Plane 0:
 bpp = 1
 offset = 0

 Plane 1:
 bpp = 1
 offset = plane0_size


 b) Interleaved planes
 [Y CBCR Y CBCR Y CBCR ]
 [Y CBCR Y CBCR Y CBCR

[PATCH v4] DRM: add drm gem CMA helper

[Lars-Peter Clausen]

On 06/27/2012 03:30 PM, Sascha Hauer wrote:
> Many embedded drm devices do not have a IOMMU and no dedicated
> memory for graphics. These devices use CMA (Contiguous Memory
> Allocator) backed graphics memory. This patch provides helper
> functions to be able to share the code. The code technically does
> not depend on CMA as the backend allocator, the name has been chosen
> because cma makes for a nice, short but still descriptive function
> prefix.
> 
> Signed-off-by: Sascha Hauer 

Tested-by: Lars-Peter Clausen 

... for quite some time now.

There are now three drivers, each from different people, which depend on
this part of infrastructure code. These drivers also depends on the "DRM:
Add DRM kms/fb cma helper" patch, which builds on top of this patch. Is
there anything that needs to be done or anything we can do to speed up
inclusion of these two patches into the DRM tree?

Thanks,
- Lars


> ---
> 
> changes since v3:
> - *really* use DIV_ROUND_UP(args->width * args->bpp, 8) to calculate the pitch
> changes since v2:
> - make drm_gem_cma_create_with_handle static
> - use DIV_ROUND_UP(args->width * args->bpp, 8) to calculate the pitch
> - make drm_gem_cma_vm_ops const
> - add missing #include 
> changes since v1:
> - map whole buffer at mmap time, not page by page at fault time
> - several cleanups as suggested by Lars-Peter Clausen and Laurent Pinchart
> 
>  drivers/gpu/drm/Kconfig  |6 +
>  drivers/gpu/drm/Makefile |1 +
>  drivers/gpu/drm/drm_gem_cma_helper.c |  251 
> ++
>  include/drm/drm_gem_cma_helper.h |   44 ++
>  4 files changed, 302 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c
>  create mode 100644 include/drm/drm_gem_cma_helper.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index dc5df12..8f362ec 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -53,6 +53,12 @@ config DRM_TTM
> GPU memory types. Will be enabled automatically if a device driver
> uses it.
>  
> +config DRM_GEM_CMA_HELPER
> + tristate
> + depends on DRM
> + help
> +   Choose this if you need the GEM CMA helper functions
> +
>  config DRM_TDFX
>   tristate "3dfx Banshee/Voodoo3+"
>   depends on DRM && PCI
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 0487ff6..8759cda 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -15,6 +15,7 @@ drm-y   :=  drm_auth.o drm_buffer.o drm_bufs.o 
> drm_cache.o \
>   drm_trace_points.o drm_global.o drm_prime.o
>  
>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
> +drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>  
>  drm-usb-y   := drm_usb.o
>  
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c 
> b/drivers/gpu/drm/drm_gem_cma_helper.c
> new file mode 100644
> index 000..1aa8fee
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -0,0 +1,251 @@
> +/*
> + * drm gem CMA (contiguous memory allocator) helper functions
> + *
> + * Copyright (C) 2012 Sascha Hauer, Pengutronix
> + *
> + * Based on Samsung Exynos code
> + *
> + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +static unsigned int get_gem_mmap_offset(struct drm_gem_object *obj)
> +{
> + return (unsigned int)obj->map_list.hash.key << PAGE_SHIFT;
> +}
> +
> +static void drm_gem_cma_buf_destroy(struct drm_device *drm,
> + struct drm_gem_cma_object *cma_obj)
> +{
> + dma_free_writecombine(drm->dev, cma_obj->base.size, cma_obj->vaddr,
> + cma_obj->paddr);
> +}
> +
> +/*
> + * drm_gem_cma_create - allocate an object with the given size
> + *
> + * returns a struct drm_gem_cma_object* on success or ERR_PTR values
> + * on failure.
> + */
> +struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> 

Re: [PATCH v4] DRM: add drm gem CMA helper

[Lars-Peter Clausen]

On 06/27/2012 03:30 PM, Sascha Hauer wrote:
 Many embedded drm devices do not have a IOMMU and no dedicated
 memory for graphics. These devices use CMA (Contiguous Memory
 Allocator) backed graphics memory. This patch provides helper
 functions to be able to share the code. The code technically does
 not depend on CMA as the backend allocator, the name has been chosen
 because cma makes for a nice, short but still descriptive function
 prefix.
 
 Signed-off-by: Sascha Hauer s.ha...@pengutronix.de

Tested-by: Lars-Peter Clausen l...@metafoo.de

... for quite some time now.

There are now three drivers, each from different people, which depend on
this part of infrastructure code. These drivers also depends on the DRM:
Add DRM kms/fb cma helper patch, which builds on top of this patch. Is
there anything that needs to be done or anything we can do to speed up
inclusion of these two patches into the DRM tree?

Thanks,
- Lars


 ---
 
 changes since v3:
 - *really* use DIV_ROUND_UP(args-width * args-bpp, 8) to calculate the pitch
 changes since v2:
 - make drm_gem_cma_create_with_handle static
 - use DIV_ROUND_UP(args-width * args-bpp, 8) to calculate the pitch
 - make drm_gem_cma_vm_ops const
 - add missing #include linux/export.h
 changes since v1:
 - map whole buffer at mmap time, not page by page at fault time
 - several cleanups as suggested by Lars-Peter Clausen and Laurent Pinchart
 
  drivers/gpu/drm/Kconfig  |6 +
  drivers/gpu/drm/Makefile |1 +
  drivers/gpu/drm/drm_gem_cma_helper.c |  251 
 ++
  include/drm/drm_gem_cma_helper.h |   44 ++
  4 files changed, 302 insertions(+)
  create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c
  create mode 100644 include/drm/drm_gem_cma_helper.h
 
 diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
 index dc5df12..8f362ec 100644
 --- a/drivers/gpu/drm/Kconfig
 +++ b/drivers/gpu/drm/Kconfig
 @@ -53,6 +53,12 @@ config DRM_TTM
 GPU memory types. Will be enabled automatically if a device driver
 uses it.
  
 +config DRM_GEM_CMA_HELPER
 + tristate
 + depends on DRM
 + help
 +   Choose this if you need the GEM CMA helper functions
 +
  config DRM_TDFX
   tristate 3dfx Banshee/Voodoo3+
   depends on DRM  PCI
 diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
 index 0487ff6..8759cda 100644
 --- a/drivers/gpu/drm/Makefile
 +++ b/drivers/gpu/drm/Makefile
 @@ -15,6 +15,7 @@ drm-y   :=  drm_auth.o drm_buffer.o drm_bufs.o 
 drm_cache.o \
   drm_trace_points.o drm_global.o drm_prime.o
  
  drm-$(CONFIG_COMPAT) += drm_ioc32.o
 +drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
  
  drm-usb-y   := drm_usb.o
  
 diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c 
 b/drivers/gpu/drm/drm_gem_cma_helper.c
 new file mode 100644
 index 000..1aa8fee
 --- /dev/null
 +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
 @@ -0,0 +1,251 @@
 +/*
 + * drm gem CMA (contiguous memory allocator) helper functions
 + *
 + * Copyright (C) 2012 Sascha Hauer, Pengutronix
 + *
 + * Based on Samsung Exynos code
 + *
 + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License
 + * as published by the Free Software Foundation; either version 2
 + * of the License, or (at your option) any later version.
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + */
 +
 +#include linux/mm.h
 +#include linux/slab.h
 +#include linux/mutex.h
 +#include linux/export.h
 +#include linux/dma-mapping.h
 +
 +#include drm/drmP.h
 +#include drm/drm.h
 +#include drm/drm_gem_cma_helper.h
 +
 +static unsigned int get_gem_mmap_offset(struct drm_gem_object *obj)
 +{
 + return (unsigned int)obj-map_list.hash.key  PAGE_SHIFT;
 +}
 +
 +static void drm_gem_cma_buf_destroy(struct drm_device *drm,
 + struct drm_gem_cma_object *cma_obj)
 +{
 + dma_free_writecombine(drm-dev, cma_obj-base.size, cma_obj-vaddr,
 + cma_obj-paddr);
 +}
 +
 +/*
 + * drm_gem_cma_create - allocate an object with the given size
 + *
 + * returns a struct drm_gem_cma_object* on success or ERR_PTR values
 + * on failure.
 + */
 +struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
 + unsigned int size)
 +{
 + struct drm_gem_cma_object *cma_obj;
 + struct drm_gem_object *gem_obj;
 + int ret;
 +
 + size = round_up(size, PAGE_SIZE);
 +
 + cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);
 + if (!cma_obj)
 + return ERR_PTR(-ENOMEM);
 +
 + cma_obj-vaddr = dma_alloc_writecombine(drm-dev, size

[PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

This patchset introduces a set of helper function for implementing the KMS
framebuffer layer for drivers which use the drm gem CMA helper function.

Signed-off-by: Lars-Peter Clausen 

---
Note: This patch depends on Sascha's "DRM: add drm gem CMA helper" patch

Changes since v2:
* Adapt to changes in the GEM CMA helper
* Add basic buffer size checking in drm_fb_cma_create
Changes since v1:
* Some spelling fixes
* Add missing kfree in drm_fb_cma_alloc error path
* Add multi-plane support
---
 drivers/gpu/drm/Kconfig |   10 +
 drivers/gpu/drm/Makefile|1 +
 drivers/gpu/drm/drm_fb_cma_helper.c |  393 +++
 include/drm/drm_fb_cma_helper.h |   27 +++
 4 files changed, 431 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
 create mode 100644 include/drm/drm_fb_cma_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 41bbd95..e511c9a 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -41,6 +41,16 @@ config DRM_GEM_CMA_HELPER
help
  Choose this if you need the GEM CMA helper functions

+config DRM_KMS_CMA_HELPER
+   tristate
+   select DRM_GEM_CMA_HELPER
+   select DRM_KMS_HELPER
+   select FB_SYS_FILLRECT
+   select FB_SYS_COPYAREA
+   select FB_SYS_IMAGEBLIT
+   help
+ Choose this if you need the KMS cma helper functions
+
 config DRM_TDFX
tristate "3dfx Banshee/Voodoo3+"
depends on DRM && PCI
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 6e9e948..5dcb1a5 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o

 drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o
+drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o

 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c 
b/drivers/gpu/drm/drm_fb_cma_helper.c
new file mode 100644
index 000..9042233
--- /dev/null
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -0,0 +1,393 @@
+/*
+ * drm kms/fb cma (contiguous memory allocator) helper functions
+ *
+ * Copyright (C) 2012 Analog Device Inc.
+ *   Author: Lars-Peter Clausen 
+ *
+ * Based on udl_fbdev.c
+ *  Copyright (C) 2012 Red Hat
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct drm_fb_cma {
+   struct drm_framebuffer  fb;
+   struct drm_gem_cma_object   *obj[4];
+};
+
+struct drm_fbdev_cma {
+   struct drm_fb_helperfb_helper;
+   struct drm_fb_cma   *fb;
+};
+
+static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper *helper)
+{
+   return container_of(helper, struct drm_fbdev_cma, fb_helper);
+}
+
+static inline struct drm_fb_cma *to_fb_cma(struct drm_framebuffer *fb)
+{
+   return container_of(fb, struct drm_fb_cma, fb);
+}
+
+static void drm_fb_cma_destroy(struct drm_framebuffer *fb)
+{
+   struct drm_fb_cma *fb_cma = to_fb_cma(fb);
+   int i;
+
+   for (i = 0; i < 4; i++) {
+   if (fb_cma->obj[i])
+   
drm_gem_object_unreference_unlocked(_cma->obj[i]->base);
+   }
+
+   drm_framebuffer_cleanup(fb);
+   kfree(fb_cma);
+}
+
+static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
+   struct drm_file *file_priv, unsigned int *handle)
+{
+   struct drm_fb_cma *fb_cma = to_fb_cma(fb);
+
+   return drm_gem_handle_create(file_priv,
+   _cma->obj[0]->base, handle);
+}
+
+static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
+   .destroy= drm_fb_cma_destroy,
+   .create_handle  = drm_fb_cma_create_handle,
+};
+
+static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
+   struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_cma_object **obj,
+   unsigned int num_planes)
+{
+   struct drm_fb_cma *fb_cma;
+   int ret;
+   int i;
+
+   fb_cma = kzalloc(sizeof(*fb_cma), GFP_KERNEL);
+   if (!fb_cma)
+   return ERR_PTR(-ENOMEM);
+
+   ret = drm_framebuffer_init(dev, _cma->fb, _fb_cma_funcs);
+   if (ret) {
+   dev_err(dev->dev, "Failed to initalize framebuffer: %d\n", ret);
+  

[PATCH v3] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

This patchset introduces a set of helper function for implementing the KMS
framebuffer layer for drivers which use the drm gem CMA helper function.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de

---
Note: This patch depends on Sascha's DRM: add drm gem CMA helper patch

Changes since v2:
* Adapt to changes in the GEM CMA helper
* Add basic buffer size checking in drm_fb_cma_create
Changes since v1:
* Some spelling fixes
* Add missing kfree in drm_fb_cma_alloc error path
* Add multi-plane support
---
 drivers/gpu/drm/Kconfig |   10 +
 drivers/gpu/drm/Makefile|1 +
 drivers/gpu/drm/drm_fb_cma_helper.c |  393 +++
 include/drm/drm_fb_cma_helper.h |   27 +++
 4 files changed, 431 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
 create mode 100644 include/drm/drm_fb_cma_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 41bbd95..e511c9a 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -41,6 +41,16 @@ config DRM_GEM_CMA_HELPER
help
  Choose this if you need the GEM CMA helper functions
 
+config DRM_KMS_CMA_HELPER
+   tristate
+   select DRM_GEM_CMA_HELPER
+   select DRM_KMS_HELPER
+   select FB_SYS_FILLRECT
+   select FB_SYS_COPYAREA
+   select FB_SYS_IMAGEBLIT
+   help
+ Choose this if you need the KMS cma helper functions
+
 config DRM_TDFX
tristate 3dfx Banshee/Voodoo3+
depends on DRM  PCI
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 6e9e948..5dcb1a5 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
 
 drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o
+drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c 
b/drivers/gpu/drm/drm_fb_cma_helper.c
new file mode 100644
index 000..9042233
--- /dev/null
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -0,0 +1,393 @@
+/*
+ * drm kms/fb cma (contiguous memory allocator) helper functions
+ *
+ * Copyright (C) 2012 Analog Device Inc.
+ *   Author: Lars-Peter Clausen l...@metafoo.de
+ *
+ * Based on udl_fbdev.c
+ *  Copyright (C) 2012 Red Hat
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include drm/drmP.h
+#include drm/drm_crtc.h
+#include drm/drm_fb_helper.h
+#include drm/drm_crtc_helper.h
+#include drm/drm_gem_cma_helper.h
+#include drm/drm_fb_cma_helper.h
+#include linux/module.h
+
+struct drm_fb_cma {
+   struct drm_framebuffer  fb;
+   struct drm_gem_cma_object   *obj[4];
+};
+
+struct drm_fbdev_cma {
+   struct drm_fb_helperfb_helper;
+   struct drm_fb_cma   *fb;
+};
+
+static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper *helper)
+{
+   return container_of(helper, struct drm_fbdev_cma, fb_helper);
+}
+
+static inline struct drm_fb_cma *to_fb_cma(struct drm_framebuffer *fb)
+{
+   return container_of(fb, struct drm_fb_cma, fb);
+}
+
+static void drm_fb_cma_destroy(struct drm_framebuffer *fb)
+{
+   struct drm_fb_cma *fb_cma = to_fb_cma(fb);
+   int i;
+
+   for (i = 0; i  4; i++) {
+   if (fb_cma-obj[i])
+   
drm_gem_object_unreference_unlocked(fb_cma-obj[i]-base);
+   }
+
+   drm_framebuffer_cleanup(fb);
+   kfree(fb_cma);
+}
+
+static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
+   struct drm_file *file_priv, unsigned int *handle)
+{
+   struct drm_fb_cma *fb_cma = to_fb_cma(fb);
+
+   return drm_gem_handle_create(file_priv,
+   fb_cma-obj[0]-base, handle);
+}
+
+static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
+   .destroy= drm_fb_cma_destroy,
+   .create_handle  = drm_fb_cma_create_handle,
+};
+
+static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
+   struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_cma_object **obj,
+   unsigned int num_planes)
+{
+   struct drm_fb_cma *fb_cma;
+   int ret;
+   int i;
+
+   fb_cma = kzalloc(sizeof(*fb_cma), GFP_KERNEL);
+   if (!fb_cma)
+   return ERR_PTR(-ENOMEM);
+
+   ret = drm_framebuffer_init(dev, fb_cma-fb, drm_fb_cma_funcs);
+   if (ret) {
+   dev_err(dev-dev

[PATCH 4/4] drm: Renesas SH Mobile DRM driver

[Lars-Peter Clausen]

On 06/27/2012 10:27 PM, Laurent Pinchart wrote:
> Hi Lars-Peter,
> 
> On Wednesday 27 June 2012 22:06:54 Lars-Peter Clausen wrote:
>> On 06/27/2012 02:40 PM, Laurent Pinchart wrote:
>>> Hi Lars-Peter,
>>>
>>> [...]
>>>
>>> Do you plan to post a v3 of your "DRM: Add DRM kms/fb cma helper" patch in
>>> the near future ?
>>
>> Hi Laurent,
>>
>> I don't think there have been any changes since v2 except for adjusting to
>> the renamed functions in Sascha's patch. I'll send a v3 tomorrow.
> 
> OK, thanks.
> 
> As Ville pointed out, we need to validate the offsets and make sure the frame 
> buffer fits in the GEM object(s). I don't think there's code in your patch to 
> do so, have you thought about where/how to implement that ?

I think part of it needs to be done in the driver code, since the the generic
code does not the specific buffer layout requirements imposed by the hardware.
We can probably add some basic sanity checks for the buffer size though.

- Lars

[PATCH 4/4] drm: Renesas SH Mobile DRM driver

[Lars-Peter Clausen]

On 06/27/2012 02:40 PM, Laurent Pinchart wrote:
> Hi Lars-Peter,
> 
> [...]
> 
> Do you plan to post a v3 of your "DRM: Add DRM kms/fb cma helper" patch in 
> the 
> near future ?

Hi Laurent,

I don't think there have been any changes since v2 except for adjusting to the
renamed functions in Sascha's patch. I'll send a v3 tomorrow.

I think it makes most sense if you carry these two patches (Sascha's and mine)
as part of your "Renesas SH Mobile DRM driver" series, so reviewers can see it
in context.

- Lars

Re: [PATCH 4/4] drm: Renesas SH Mobile DRM driver

[Lars-Peter Clausen]

On 06/27/2012 02:40 PM, Laurent Pinchart wrote:
 Hi Lars-Peter,
 
 [...]
 
 Do you plan to post a v3 of your DRM: Add DRM kms/fb cma helper patch in 
 the 
 near future ?

Hi Laurent,

I don't think there have been any changes since v2 except for adjusting to the
renamed functions in Sascha's patch. I'll send a v3 tomorrow.

I think it makes most sense if you carry these two patches (Sascha's and mine)
as part of your Renesas SH Mobile DRM driver series, so reviewers can see it
in context.

- Lars

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm: Renesas SH Mobile DRM driver

[Lars-Peter Clausen]

On 06/27/2012 10:27 PM, Laurent Pinchart wrote:
 Hi Lars-Peter,
 
 On Wednesday 27 June 2012 22:06:54 Lars-Peter Clausen wrote:
 On 06/27/2012 02:40 PM, Laurent Pinchart wrote:
 Hi Lars-Peter,

 [...]

 Do you plan to post a v3 of your DRM: Add DRM kms/fb cma helper patch in
 the near future ?

 Hi Laurent,

 I don't think there have been any changes since v2 except for adjusting to
 the renamed functions in Sascha's patch. I'll send a v3 tomorrow.
 
 OK, thanks.
 
 As Ville pointed out, we need to validate the offsets and make sure the frame 
 buffer fits in the GEM object(s). I don't think there's code in your patch to 
 do so, have you thought about where/how to implement that ?

I think part of it needs to be done in the driver code, since the the generic
code does not the specific buffer layout requirements imposed by the hardware.
We can probably add some basic sanity checks for the buffer size though.

- Lars
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] DRM: add drm gem cma helper

[Lars-Peter Clausen]

On 05/31/2012 10:08 AM, Sascha Hauer wrote:
> [...]
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c
> new file mode 100644
> index 000..d8c0dc7
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -0,0 +1,243 @@
> +/*
> + * drm gem cma (contiguous memory allocator) helper functions
> + *
> + * Copyright (C) 2012 Sascha Hauer, Pengutronix
> + *
> + * Based on Samsung Exynos code
> + *
> + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include 
> +#include 
> +#include 

Sorry, forgot to mention this during the v1 review. This needs
'#include ' for EXPORT_SYMBOL_GPL. I get compile errors
otherwise. Maybe it also makes sense to include linux/slab.h,
linux/dma-mapping.h, linux/mm.h and linux/mutex.h since those are only
implicitly included right now.

- Lars

[PATCH v2] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 05/31/2012 08:11 PM, Sascha Hauer wrote:
> On Thu, May 31, 2012 at 11:34:37AM +0200, Lars-Peter Clausen wrote:
>>>> +  drm_helper_mode_fill_fb_struct(_cma->fb, mode_cmd);
>>>> +
>>>> +  for (i = 0; i < num_planes; i++)
>>>> +  fb_cma->obj[i] = obj[i];
>>>
>>> Check for valid num_planes before this loop?
>>>
>>
>> Hm, I think the callers already take care of this. drm_format_num_planes will
>> always return a valid number and the other caller passes 1 unconditionally.
> 
> As long as the array always is big enough to hold the maximum number of
> planes I think it's fine. However, if there is some format with 5 planes
> (don't know if there is, I only know yuv with multiple planes) it's not
> obvious that this code does not support this.

The DRM ABI limits the number of planes to 4. But adding a global
DRM_MAX_PLANES is probably a good idea to avoid surprises if this ever changes.

Re: [PATCH v2] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 05/31/2012 08:11 PM, Sascha Hauer wrote:
 On Thu, May 31, 2012 at 11:34:37AM +0200, Lars-Peter Clausen wrote:
 +  drm_helper_mode_fill_fb_struct(fb_cma-fb, mode_cmd);
 +
 +  for (i = 0; i  num_planes; i++)
 +  fb_cma-obj[i] = obj[i];

 Check for valid num_planes before this loop?


 Hm, I think the callers already take care of this. drm_format_num_planes will
 always return a valid number and the other caller passes 1 unconditionally.
 
 As long as the array always is big enough to hold the maximum number of
 planes I think it's fine. However, if there is some format with 5 planes
 (don't know if there is, I only know yuv with multiple planes) it's not
 obvious that this code does not support this.

The DRM ABI limits the number of planes to 4. But adding a global
DRM_MAX_PLANES is probably a good idea to avoid surprises if this ever changes.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] DRM: add drm gem cma helper

[Lars-Peter Clausen]

On 05/31/2012 10:08 AM, Sascha Hauer wrote:
 [...]
 diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c
 new file mode 100644
 index 000..d8c0dc7
 --- /dev/null
 +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
 @@ -0,0 +1,243 @@
 +/*
 + * drm gem cma (contiguous memory allocator) helper functions
 + *
 + * Copyright (C) 2012 Sascha Hauer, Pengutronix
 + *
 + * Based on Samsung Exynos code
 + *
 + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License
 + * as published by the Free Software Foundation; either version 2
 + * of the License, or (at your option) any later version.
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + */
 +#include drm/drmP.h
 +#include drm/drm.h
 +#include drm/drm_gem_cma_helper.h

Sorry, forgot to mention this during the v1 review. This needs
'#include linux/export.h' for EXPORT_SYMBOL_GPL. I get compile errors
otherwise. Maybe it also makes sense to include linux/slab.h,
linux/dma-mapping.h, linux/mm.h and linux/mutex.h since those are only
implicitly included right now.

- Lars
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 05/31/2012 10:13 AM, Sascha Hauer wrote:
> On Wed, May 30, 2012 at 05:30:15PM +0200, Lars-Peter Clausen wrote:
>> This patch introduces a set of helper function for implementing the KMS
>> framebuffer layer for drivers which use the drm gem CMA helper function.
>>
>> Signed-off-by: Lars-Peter Clausen 
>>
>> ---
>> Changes since v1:
>>  * Some spelling fixes
>>  * Add missing kfree in drm_fb_cma_alloc error path
>>  * Add multi-plane support. drm_fb_cma_get_gem_obj now takes a second
>>parameter to specify the plane.
>> ---
>>  drivers/gpu/drm/Kconfig |   11 +
>>  drivers/gpu/drm/Makefile|1 +
>>  drivers/gpu/drm/drm_fb_cma_helper.c |  384 
>> +++
>>  include/drm/drm_fb_cma_helper.h |   27 +++
>>  4 files changed, 423 insertions(+)
>>  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
>>  create mode 100644 include/drm/drm_fb_cma_helper.h
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index a78dfbe..e7c0a3d 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -41,6 +41,17 @@ config DRM_GEM_CMA_HELPER
>>  help
>>Choose this if you need the GEM cma helper functions
>>  
>> +config DRM_KMS_CMA_HELPER
>> +tristate
>> +select DRM_GEM_CMA_HELPER
>> +select DRM_KMS_HELPER
>> +select FB_SYS_FILLRECT
>> +select FB_SYS_COPYAREA
>> +select FB_SYS_IMAGEBLIT
>> +help
>> +  Choose this if you need the KMS cma helper functions
>> +
>> +
>>  config DRM_TDFX
>>  tristate "3dfx Banshee/Voodoo3+"
>>  depends on DRM && PCI
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 6e9e948..5dcb1a5 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
>>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>>  
>>  drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o
>> +drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>>  
>>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>>  
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c 
>> b/drivers/gpu/drm/drm_fb_cma_helper.c
>> new file mode 100644
>> index 000..8821a98
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -0,0 +1,384 @@
>> +/*
>> + * drm kms/fb cma (contiguous memory allocator) helper functions
>> + *
>> + * Copyright (C) 2012 Analog Device Inc.
>> + *   Author: Lars-Peter Clausen 
>> + *
>> + * Based on udl_fbdev.c
>> + *  Copyright (C) 2012 Red Hat
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +struct drm_fb_cma {
>> +struct drm_framebuffer  fb;
>> +struct drm_gem_cma_obj  *obj[4];
> 
> Can we have a define for this magic '4'? It is used several times in
> this patch.

Yes, had the same though. The magic 4 is actually used all through all of DRM.
Something like '#define DRM_MAX_PLANES 4' probably makes sense.

> 
>> +static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
>> +struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_cma_obj **obj,
>> +unsigned int num_planes)
>> +{
>> +struct drm_fb_cma *fb_cma;
>> +int ret;
>> +int i;
>> +
>> +fb_cma = kzalloc(sizeof(*fb_cma), GFP_KERNEL);
>> +if (!fb_cma)
>> +return ERR_PTR(-ENOMEM);
>> +
>> +ret = drm_framebuffer_init(dev, _cma->fb, _fb_cma_funcs);
>> +if (ret) {
>> +dev_err(dev->dev, "Failed to initalize framebuffer: %d\n", ret);
>> +kfree(fb_cma);
>> +return ERR_PTR(ret);
>> +}
>> +
>> +drm_helper_mode_fill_fb_struct(_cma->fb, mode_cmd);
>> +
>> +for (i = 0; i < num_planes; i++)
>> +fb_cma->obj[i] = obj[i];
> 
> Check for valid num_planes before this loop?
> 

Hm, I think the callers already take care of this. drm_format_num_planes will
always return a valid number and the other caller passes 1 unconditionally.

> 
> Note I have renamed struct drm_gem_cma_obj to struct drm_gem_cma_object
> in the patch I just sent.
> 

Do you think it makes sense for you to carry this patch as part of your iMX DRM
series?

Thanks,
- Lars

Re: [PATCH v2] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

On 05/31/2012 10:13 AM, Sascha Hauer wrote:
 On Wed, May 30, 2012 at 05:30:15PM +0200, Lars-Peter Clausen wrote:
 This patch introduces a set of helper function for implementing the KMS
 framebuffer layer for drivers which use the drm gem CMA helper function.

 Signed-off-by: Lars-Peter Clausen l...@metafoo.de

 ---
 Changes since v1:
  * Some spelling fixes
  * Add missing kfree in drm_fb_cma_alloc error path
  * Add multi-plane support. drm_fb_cma_get_gem_obj now takes a second
parameter to specify the plane.
 ---
  drivers/gpu/drm/Kconfig |   11 +
  drivers/gpu/drm/Makefile|1 +
  drivers/gpu/drm/drm_fb_cma_helper.c |  384 
 +++
  include/drm/drm_fb_cma_helper.h |   27 +++
  4 files changed, 423 insertions(+)
  create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
  create mode 100644 include/drm/drm_fb_cma_helper.h

 diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
 index a78dfbe..e7c0a3d 100644
 --- a/drivers/gpu/drm/Kconfig
 +++ b/drivers/gpu/drm/Kconfig
 @@ -41,6 +41,17 @@ config DRM_GEM_CMA_HELPER
  help
Choose this if you need the GEM cma helper functions
  
 +config DRM_KMS_CMA_HELPER
 +tristate
 +select DRM_GEM_CMA_HELPER
 +select DRM_KMS_HELPER
 +select FB_SYS_FILLRECT
 +select FB_SYS_COPYAREA
 +select FB_SYS_IMAGEBLIT
 +help
 +  Choose this if you need the KMS cma helper functions
 +
 +
  config DRM_TDFX
  tristate 3dfx Banshee/Voodoo3+
  depends on DRM  PCI
 diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
 index 6e9e948..5dcb1a5 100644
 --- a/drivers/gpu/drm/Makefile
 +++ b/drivers/gpu/drm/Makefile
 @@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
  
  drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o
 +drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
  
  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
  
 diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c 
 b/drivers/gpu/drm/drm_fb_cma_helper.c
 new file mode 100644
 index 000..8821a98
 --- /dev/null
 +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
 @@ -0,0 +1,384 @@
 +/*
 + * drm kms/fb cma (contiguous memory allocator) helper functions
 + *
 + * Copyright (C) 2012 Analog Device Inc.
 + *   Author: Lars-Peter Clausen l...@metafoo.de
 + *
 + * Based on udl_fbdev.c
 + *  Copyright (C) 2012 Red Hat
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License
 + * as published by the Free Software Foundation; either version 2
 + * of the License, or (at your option) any later version.
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + */
 +
 +#include drm/drmP.h
 +#include drm/drm_crtc.h
 +#include drm/drm_fb_helper.h
 +#include drm/drm_crtc_helper.h
 +#include drm/drm_gem_cma_helper.h
 +#include drm/drm_fb_cma_helper.h
 +#include linux/module.h
 +
 +struct drm_fb_cma {
 +struct drm_framebuffer  fb;
 +struct drm_gem_cma_obj  *obj[4];
 
 Can we have a define for this magic '4'? It is used several times in
 this patch.

Yes, had the same though. The magic 4 is actually used all through all of DRM.
Something like '#define DRM_MAX_PLANES 4' probably makes sense.

 
 +static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
 +struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_cma_obj **obj,
 +unsigned int num_planes)
 +{
 +struct drm_fb_cma *fb_cma;
 +int ret;
 +int i;
 +
 +fb_cma = kzalloc(sizeof(*fb_cma), GFP_KERNEL);
 +if (!fb_cma)
 +return ERR_PTR(-ENOMEM);
 +
 +ret = drm_framebuffer_init(dev, fb_cma-fb, drm_fb_cma_funcs);
 +if (ret) {
 +dev_err(dev-dev, Failed to initalize framebuffer: %d\n, ret);
 +kfree(fb_cma);
 +return ERR_PTR(ret);
 +}
 +
 +drm_helper_mode_fill_fb_struct(fb_cma-fb, mode_cmd);
 +
 +for (i = 0; i  num_planes; i++)
 +fb_cma-obj[i] = obj[i];
 
 Check for valid num_planes before this loop?
 

Hm, I think the callers already take care of this. drm_format_num_planes will
always return a valid number and the other caller passes 1 unconditionally.

 
 Note I have renamed struct drm_gem_cma_obj to struct drm_gem_cma_object
 in the patch I just sent.
 

Do you think it makes sense for you to carry this patch as part of your iMX DRM
series?

Thanks,
- Lars
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] DRM: Add DRM kms/fb cma helper

[Lars-Peter Clausen]

This patch introduces a set of helper function for implementing the KMS
framebuffer layer for drivers which use the drm gem CMA helper function.

Signed-off-by: Lars-Peter Clausen 

---
Changes since v1:
* Some spelling fixes
* Add missing kfree in drm_fb_cma_alloc error path
* Add multi-plane support. drm_fb_cma_get_gem_obj now takes a second
  parameter to specify the plane.
---
 drivers/gpu/drm/Kconfig |   11 +
 drivers/gpu/drm/Makefile|1 +
 drivers/gpu/drm/drm_fb_cma_helper.c |  384 +++
 include/drm/drm_fb_cma_helper.h |   27 +++
 4 files changed, 423 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_fb_cma_helper.c
 create mode 100644 include/drm/drm_fb_cma_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index a78dfbe..e7c0a3d 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -41,6 +41,17 @@ config DRM_GEM_CMA_HELPER
help
  Choose this if you need the GEM cma helper functions

+config DRM_KMS_CMA_HELPER
+   tristate
+   select DRM_GEM_CMA_HELPER
+   select DRM_KMS_HELPER
+   select FB_SYS_FILLRECT
+   select FB_SYS_COPYAREA
+   select FB_SYS_IMAGEBLIT
+   help
+ Choose this if you need the KMS cma helper functions
+
+
 config DRM_TDFX
tristate "3dfx Banshee/Voodoo3+"
depends on DRM && PCI
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 6e9e948..5dcb1a5 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -18,6 +18,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o

 drm_kms_helper-y := drm_fb_helper.o drm_crtc_helper.o drm_dp_i2c_helper.o
+drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o

 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c 
b/drivers/gpu/drm/drm_fb_cma_helper.c
new file mode 100644
index 000..8821a98
--- /dev/null
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -0,0 +1,384 @@
+/*
+ * drm kms/fb cma (contiguous memory allocator) helper functions
+ *
+ * Copyright (C) 2012 Analog Device Inc.
+ *   Author: Lars-Peter Clausen 
+ *
+ * Based on udl_fbdev.c
+ *  Copyright (C) 2012 Red Hat
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct drm_fb_cma {
+   struct drm_framebuffer  fb;
+   struct drm_gem_cma_obj  *obj[4];
+};
+
+struct drm_fbdev_cma {
+   struct drm_fb_helperfb_helper;
+   struct drm_fb_cma   *fb;
+};
+
+static inline struct drm_fbdev_cma *to_fbdev_cma(struct drm_fb_helper *helper)
+{
+   return container_of(helper, struct drm_fbdev_cma, fb_helper);
+}
+
+static inline struct drm_fb_cma *to_fb_cma(struct drm_framebuffer *fb)
+{
+   return container_of(fb, struct drm_fb_cma, fb);
+}
+
+static void drm_fb_cma_destroy(struct drm_framebuffer *fb)
+{
+   struct drm_fb_cma *fb_cma = to_fb_cma(fb);
+   int i;
+
+   for (i = 0; i < 4; i++) {
+   if (fb_cma->obj[i])
+   
drm_gem_object_unreference_unlocked(_cma->obj[i]->base);
+   }
+
+   drm_framebuffer_cleanup(fb);
+   kfree(fb_cma);
+}
+
+static int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
+   struct drm_file *file_priv, unsigned int *handle)
+{
+   struct drm_fb_cma *fb_cma = to_fb_cma(fb);
+
+   return drm_gem_handle_create(file_priv,
+   _cma->obj[0]->base, handle);
+}
+
+static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
+   .destroy= drm_fb_cma_destroy,
+   .create_handle  = drm_fb_cma_create_handle,
+};
+
+static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
+   struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_cma_obj **obj,
+   unsigned int num_planes)
+{
+   struct drm_fb_cma *fb_cma;
+   int ret;
+   int i;
+
+   fb_cma = kzalloc(sizeof(*fb_cma), GFP_KERNEL);
+   if (!fb_cma)
+   return ERR_PTR(-ENOMEM);
+
+   ret = drm_framebuffer_init(dev, _cma->fb, _fb_cma_funcs);
+   if (ret) {
+   dev_err(dev->dev, "Failed to initalize framebuffer: %d\n", ret);
+   kfree(fb_cma);
+   return ERR_PTR(ret);
+   }
+
+   drm_helper_mode_fill_fb_struct(_cma->fb, mode_cmd);
+
+   for (i = 0; i < num_planes; i++)
+ 

[PATCH 4/4] drm: Renesas SH Mobile DRM driver

[Lars-Peter Clausen]

On 05/30/2012 04:45 PM, Lars-Peter Clausen wrote:
> On 05/30/2012 02:32 PM, Laurent Pinchart wrote:
>> [...]
>> +for (i = 0; i < (format->yuv ? 2 : 1); ++i) {
>> +obj = drm_gem_object_lookup(dev, file_priv,
>> +mode_cmd->handles[i]);
>> +if (obj == NULL) {
>> +dev_dbg(dev->dev, "GEM object %u not found\n",
>> +mode_cmd->handles[i]);
>> +ret = -ENOENT;
>> +goto error;
>> +}
>> +sfb->sobj[i] = to_shmob_gem_object(obj);
>> +}
>> +
> 
> I added multi-plane support the cma fb helper functions and it seems to
> work. But all other DRM drivers seem to assume that multi-plane formats
> still only have a single buffer, while yours seems to assume that there is a
> plane for each buffer. The exception is the Exynos driver, but it added a
> new set of formats which are identical to the other formats, but use one
> buffer per plane.
> 
> So I'm not sure how to implement this correctly in a generic fashion.
> 

This post by Ville Syrj?l? seems to explain it quite well:
http://lists.freedesktop.org/archives/dri-devel/2012-April/021044.html

[PATCH 4/4] drm: Renesas SH Mobile DRM driver

[Lars-Peter Clausen]

On 05/30/2012 02:32 PM, Laurent Pinchart wrote:
> [...]
> + for (i = 0; i < (format->yuv ? 2 : 1); ++i) {
> + obj = drm_gem_object_lookup(dev, file_priv,
> + mode_cmd->handles[i]);
> + if (obj == NULL) {
> + dev_dbg(dev->dev, "GEM object %u not found\n",
> + mode_cmd->handles[i]);
> + ret = -ENOENT;
> + goto error;
> + }
> + sfb->sobj[i] = to_shmob_gem_object(obj);
> + }
> +

I added multi-plane support the cma fb helper functions and it seems to
work. But all other DRM drivers seem to assume that multi-plane formats
still only have a single buffer, while yours seems to assume that there is a
plane for each buffer. The exception is the Exynos driver, but it added a
new set of formats which are identical to the other formats, but use one
buffer per plane.

So I'm not sure how to implement this correctly in a generic fashion.

- Lars

[PATCH 4/4] drm: Renesas SH Mobile DRM driver

[Lars-Peter Clausen]

On 05/30/2012 03:43 PM, Sascha Hauer wrote:
> Hi Laurent,
> 
> On Wed, May 30, 2012 at 02:32:59PM +0200, Laurent Pinchart wrote:
>> The SH Mobile LCD controller (LCDC) DRM driver supports the main
>> graphics plane in RGB and YUV formats, as well as the overlay planes (in
>> alpha-blending mode only).
>>
>> Only flat panel outputs using the parallel interface are supported.
>> Support for SYS panels, HDMI and DSI is currently not implemented.
> 
> Have you seen
> 
> http://lists.freedesktop.org/archives/dri-devel/2012-May/023536.html
> 
> and
> 
> http://lists.freedesktop.org/archives/dri-devel/2012-May/023544.html
> 
> The first patch is a generic gem helper for dma_alloc_* backed memory
> which could serve as a drop in replacement for your gem code. The
> latter is a generic helper for the framebuffer handling on these
> devices. I saw that your driver supports planes which Lars' patch
> currently doesn't, but it should be easy to add support for this.
> 

Yup, just added plane support, will repost the patch in a moment.

- Lars