[PATCH v1 0/4] xlnx-zcu102: fix the display port.

[PATCH v1 0/4] xlnx-zcu102: fix the display port.

[frederic.konrad]

From: Frederic Konrad <fkonrad@amd.com>

Hi,

This patch set fixes some issues with the DisplayPort for the ZCU102:

The first patch fixes the wrong register size and thus the risk of register
overflow.

The three other one add a vblank interrupt required by the linux driver:
  - When using the VNC graphic backend and leaving it unconnected, in the best
    case the gfx_update callback is called once every 3000ms which is
    insufficient for the driver.  This is fixed by providing a VBLANK interrupt
    from a ptimer.
  - This requirement revealed two issues with the IRQ numbers and the
    interrupt disable logic fixed by the two last patches.

Tested by booting Petalinux with the framebuffer enabled.

Best Regards,
Fred

Frederic Konrad (2):
  xlnx_dp: fix the wrong register size
  xlnx-zynqmp: fix the irq mapping for the display port and its dma

Sai Pavan Boddu (2):
  xlnx_dp: Introduce a vblank signal
  xlnx_dp: Fix the interrupt disable logic

 hw/arm/xlnx-zynqmp.c         |  4 ++--
 hw/display/xlnx_dp.c         | 43 +++++++++++++++++++++++++++---------
 include/hw/display/xlnx_dp.h | 12 ++++++++--
 3 files changed, 44 insertions(+), 15 deletions(-)

-- 
2.25.1

[PATCH v1 1/4] xlnx_dp: fix the wrong register size

[frederic.konrad]

From: Frederic Konrad <fkonrad@amd.com>

The core and the vblend registers size are wrong, they should respectively be
0x3B0 and 0x1E0 according to:
  https://www.xilinx.com/htmldocs/registers/ug1087/ug1087-zynq-ultrascale-registers.html.

Let's fix that and use macros when creating the mmio region.

Fixes: 58ac482a66d ("introduce xlnx-dp")
Signed-off-by: Frederic Konrad <fkonrad@amd.com>
---
 hw/display/xlnx_dp.c         | 17 ++++++++++-------
 include/hw/display/xlnx_dp.h |  9 +++++++--
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 9bb781e312..0378570459 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1219,19 +1219,22 @@ static void xlnx_dp_init(Object *obj)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     XlnxDPState *s = XLNX_DP(obj);
 
-    memory_region_init(&s->container, obj, TYPE_XLNX_DP, 0xC050);
+    memory_region_init(&s->container, obj, TYPE_XLNX_DP, DP_CONTAINER_SIZE);
 
     memory_region_init_io(&s->core_iomem, obj, &dp_ops, s, TYPE_XLNX_DP
-                          ".core", 0x3AF);
-    memory_region_add_subregion(&s->container, 0x0000, &s->core_iomem);
+                          ".core", sizeof(s->core_registers));
+    memory_region_add_subregion(&s->container, DP_CORE_REG_OFFSET,
+                                &s->core_iomem);
 
     memory_region_init_io(&s->vblend_iomem, obj, &vblend_ops, s, TYPE_XLNX_DP
-                          ".v_blend", 0x1DF);
-    memory_region_add_subregion(&s->container, 0xA000, &s->vblend_iomem);
+                          ".v_blend", sizeof(s->vblend_registers));
+    memory_region_add_subregion(&s->container, DP_VBLEND_REG_OFFSET,
+                                &s->vblend_iomem);
 
     memory_region_init_io(&s->avbufm_iomem, obj, &avbufm_ops, s, TYPE_XLNX_DP
-                          ".av_buffer_manager", 0x238);
-    memory_region_add_subregion(&s->container, 0xB000, &s->avbufm_iomem);
+                          ".av_buffer_manager", sizeof(s->avbufm_registers));
+    memory_region_add_subregion(&s->container, DP_AVBUF_REG_OFFSET,
+                                &s->avbufm_iomem);
 
     memory_region_init_io(&s->audio_iomem, obj, &audio_ops, s, TYPE_XLNX_DP
                           ".audio", sizeof(s->audio_registers));
diff --git a/include/hw/display/xlnx_dp.h b/include/hw/display/xlnx_dp.h
index 8ab4733bb8..1ef5a89ee7 100644
--- a/include/hw/display/xlnx_dp.h
+++ b/include/hw/display/xlnx_dp.h
@@ -39,10 +39,15 @@
 #define AUD_CHBUF_MAX_DEPTH                 (32 * KiB)
 #define MAX_QEMU_BUFFER_SIZE                (4 * KiB)
 
-#define DP_CORE_REG_ARRAY_SIZE              (0x3AF >> 2)
+#define DP_CORE_REG_OFFSET                  (0x0000)
+#define DP_CORE_REG_ARRAY_SIZE              (0x3B0 >> 2)
+#define DP_AVBUF_REG_OFFSET                 (0xB000)
 #define DP_AVBUF_REG_ARRAY_SIZE             (0x238 >> 2)
-#define DP_VBLEND_REG_ARRAY_SIZE            (0x1DF >> 2)
+#define DP_VBLEND_REG_OFFSET                (0xA000)
+#define DP_VBLEND_REG_ARRAY_SIZE            (0x1E0 >> 2)
+#define DP_AUDIO_REG_OFFSET                 (0xC000)
 #define DP_AUDIO_REG_ARRAY_SIZE             (0x50 >> 2)
+#define DP_CONTAINER_SIZE                   (0xC050)
 
 struct PixmanPlane {
     pixman_format_code_t format;
-- 
2.25.1

[PATCH v1 2/4] xlnx_dp: Introduce a vblank signal

[frederic.konrad]

From: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>

Add a periodic timer which raises vblank at a frequency of 30Hz.

Signed-off-by: Sai Pavan Boddu <saipava@xilinx.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Changes by fkonrad:
  - Switched to transaction-based ptimer API.
  - Added the DP_INT_VBLNK_START macro.
Signed-off-by: Frederic Konrad <fkonrad@amd.com>
---
 hw/display/xlnx_dp.c         | 24 +++++++++++++++++++++---
 include/hw/display/xlnx_dp.h |  3 +++
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 0378570459..984b0a6bb9 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -114,6 +114,7 @@
 #define DP_TX_N_AUD                         (0x032C >> 2)
 #define DP_TX_AUDIO_EXT_DATA(n)             ((0x0330 + 4 * n) >> 2)
 #define DP_INT_STATUS                       (0x03A0 >> 2)
+#define DP_INT_VBLNK_START                  (1 << 13)
 #define DP_INT_MASK                         (0x03A4 >> 2)
 #define DP_INT_EN                           (0x03A8 >> 2)
 #define DP_INT_DS                           (0x03AC >> 2)
@@ -773,6 +774,14 @@ static void xlnx_dp_write(void *opaque, hwaddr offset, uint64_t value,
         break;
     case DP_TRANSMITTER_ENABLE:
         s->core_registers[offset] = value & 0x01;
+        ptimer_transaction_begin(s->vblank);
+        if (value & 0x1) {
+            ptimer_set_limit(s->vblank, 1, 1);
+            ptimer_run(s->vblank, 0);
+        } else {
+            ptimer_stop(s->vblank);
+        }
+        ptimer_transaction_commit(s->vblank);
         break;
     case DP_FORCE_SCRAMBLER_RESET:
         /*
@@ -1177,9 +1186,6 @@ static void xlnx_dp_update_display(void *opaque)
         return;
     }
 
-    s->core_registers[DP_INT_STATUS] |= (1 << 13);
-    xlnx_dp_update_irq(s);
-
     xlnx_dpdma_trigger_vsync_irq(s->dpdma);
 
     /*
@@ -1275,6 +1281,14 @@ static void xlnx_dp_finalize(Object *obj)
     fifo8_destroy(&s->rx_fifo);
 }
 
+static void vblank_hit(void *opaque)
+{
+    XlnxDPState *s = XLNX_DP(opaque);
+
+    s->core_registers[DP_INT_STATUS] |= DP_INT_VBLNK_START;
+    xlnx_dp_update_irq(s);
+}
+
 static void xlnx_dp_realize(DeviceState *dev, Error **errp)
 {
     XlnxDPState *s = XLNX_DP(dev);
@@ -1309,6 +1323,10 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
                                            &as);
     AUD_set_volume_out(s->amixer_output_stream, 0, 255, 255);
     xlnx_dp_audio_activate(s);
+    s->vblank = ptimer_init(vblank_hit, s, PTIMER_POLICY_DEFAULT);
+    ptimer_transaction_begin(s->vblank);
+    ptimer_set_freq(s->vblank, 30);
+    ptimer_transaction_commit(s->vblank);
 }
 
 static void xlnx_dp_reset(DeviceState *dev)
diff --git a/include/hw/display/xlnx_dp.h b/include/hw/display/xlnx_dp.h
index 1ef5a89ee7..e86a87f235 100644
--- a/include/hw/display/xlnx_dp.h
+++ b/include/hw/display/xlnx_dp.h
@@ -35,6 +35,7 @@
 #include "hw/dma/xlnx_dpdma.h"
 #include "audio/audio.h"
 #include "qom/object.h"
+#include "hw/ptimer.h"
 
 #define AUD_CHBUF_MAX_DEPTH                 (32 * KiB)
 #define MAX_QEMU_BUFFER_SIZE                (4 * KiB)
@@ -107,6 +108,8 @@ struct XlnxDPState {
      */
     DPCDState *dpcd;
     I2CDDCState *edid;
+
+    ptimer_state *vblank;
 };
 
 #define TYPE_XLNX_DP "xlnx.v-dp"
-- 
2.25.1

[PATCH v1 3/4] xlnx_dp: Fix the interrupt disable logic

[frederic.konrad]

From: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>

Fix interrupt disable logic. Mask value 1 indicates that interrupts are
disabled.

Signed-off-by: Sai Pavan Boddu <saipava@xilinx.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Frederic Konrad <fkonrad@amd.com>
---
 hw/display/xlnx_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 984b0a6bb9..c60e8d0386 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -885,7 +885,7 @@ static void xlnx_dp_write(void *opaque, hwaddr offset, uint64_t value,
         xlnx_dp_update_irq(s);
         break;
     case DP_INT_DS:
-        s->core_registers[DP_INT_MASK] |= ~value;
+        s->core_registers[DP_INT_MASK] |= value;
         xlnx_dp_update_irq(s);
         break;
     default:
-- 
2.25.1

[PATCH v1 4/4] xlnx-zynqmp: fix the irq mapping for the display port and its dma

[frederic.konrad]

From: Frederic Konrad <fkonrad@amd.com>

When the display port has been initially implemented the device driver wasn't
using interrupts.  Now that the display port driver waits for vblank interrupt
it has been noticed that the irq mapping is wrong.  So use the value from the
linux device tree and the ultrascale+ reference manual.

Signed-off-by: Frederic Konrad <fkonrad@amd.com>
---
 hw/arm/xlnx-zynqmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 375309e68e..383e177a00 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -60,10 +60,10 @@
 #define SERDES_SIZE         0x20000
 
 #define DP_ADDR             0xfd4a0000
-#define DP_IRQ              113
+#define DP_IRQ              0x77
 
 #define DPDMA_ADDR          0xfd4c0000
-#define DPDMA_IRQ           116
+#define DPDMA_IRQ           0x7a
 
 #define APU_ADDR            0xfd5c0000
 #define APU_IRQ             153
-- 
2.25.1

Re: [PATCH v1 1/4] xlnx_dp: fix the wrong register size

[Edgar E. Iglesias]

On Tue, May 03, 2022 at 04:25:42PM +0100, frederic.konrad@xilinx.com wrote:
> From: Frederic Konrad <fkonrad@amd.com>
> 
> The core and the vblend registers size are wrong, they should respectively be
> 0x3B0 and 0x1E0 according to:
>   https://www.xilinx.com/htmldocs/registers/ug1087/ug1087-zynq-ultrascale-registers.html.
> 
> Let's fix that and use macros when creating the mmio region.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>


> 
> Fixes: 58ac482a66d ("introduce xlnx-dp")
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>
> ---
>  hw/display/xlnx_dp.c         | 17 ++++++++++-------
>  include/hw/display/xlnx_dp.h |  9 +++++++--
>  2 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 9bb781e312..0378570459 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -1219,19 +1219,22 @@ static void xlnx_dp_init(Object *obj)
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      XlnxDPState *s = XLNX_DP(obj);
>  
> -    memory_region_init(&s->container, obj, TYPE_XLNX_DP, 0xC050);
> +    memory_region_init(&s->container, obj, TYPE_XLNX_DP, DP_CONTAINER_SIZE);
>  
>      memory_region_init_io(&s->core_iomem, obj, &dp_ops, s, TYPE_XLNX_DP
> -                          ".core", 0x3AF);
> -    memory_region_add_subregion(&s->container, 0x0000, &s->core_iomem);
> +                          ".core", sizeof(s->core_registers));
> +    memory_region_add_subregion(&s->container, DP_CORE_REG_OFFSET,
> +                                &s->core_iomem);
>  
>      memory_region_init_io(&s->vblend_iomem, obj, &vblend_ops, s, TYPE_XLNX_DP
> -                          ".v_blend", 0x1DF);
> -    memory_region_add_subregion(&s->container, 0xA000, &s->vblend_iomem);
> +                          ".v_blend", sizeof(s->vblend_registers));
> +    memory_region_add_subregion(&s->container, DP_VBLEND_REG_OFFSET,
> +                                &s->vblend_iomem);
>  
>      memory_region_init_io(&s->avbufm_iomem, obj, &avbufm_ops, s, TYPE_XLNX_DP
> -                          ".av_buffer_manager", 0x238);
> -    memory_region_add_subregion(&s->container, 0xB000, &s->avbufm_iomem);
> +                          ".av_buffer_manager", sizeof(s->avbufm_registers));
> +    memory_region_add_subregion(&s->container, DP_AVBUF_REG_OFFSET,
> +                                &s->avbufm_iomem);
>  
>      memory_region_init_io(&s->audio_iomem, obj, &audio_ops, s, TYPE_XLNX_DP
>                            ".audio", sizeof(s->audio_registers));
> diff --git a/include/hw/display/xlnx_dp.h b/include/hw/display/xlnx_dp.h
> index 8ab4733bb8..1ef5a89ee7 100644
> --- a/include/hw/display/xlnx_dp.h
> +++ b/include/hw/display/xlnx_dp.h
> @@ -39,10 +39,15 @@
>  #define AUD_CHBUF_MAX_DEPTH                 (32 * KiB)
>  #define MAX_QEMU_BUFFER_SIZE                (4 * KiB)
>  
> -#define DP_CORE_REG_ARRAY_SIZE              (0x3AF >> 2)
> +#define DP_CORE_REG_OFFSET                  (0x0000)
> +#define DP_CORE_REG_ARRAY_SIZE              (0x3B0 >> 2)
> +#define DP_AVBUF_REG_OFFSET                 (0xB000)
>  #define DP_AVBUF_REG_ARRAY_SIZE             (0x238 >> 2)
> -#define DP_VBLEND_REG_ARRAY_SIZE            (0x1DF >> 2)
> +#define DP_VBLEND_REG_OFFSET                (0xA000)
> +#define DP_VBLEND_REG_ARRAY_SIZE            (0x1E0 >> 2)
> +#define DP_AUDIO_REG_OFFSET                 (0xC000)
>  #define DP_AUDIO_REG_ARRAY_SIZE             (0x50 >> 2)
> +#define DP_CONTAINER_SIZE                   (0xC050)
>  
>  struct PixmanPlane {
>      pixman_format_code_t format;
> -- 
> 2.25.1
> 

Re: [PATCH v1 2/4] xlnx_dp: Introduce a vblank signal

[Edgar E. Iglesias]

On Tue, May 03, 2022 at 04:25:43PM +0100, frederic.konrad@xilinx.com wrote:
> From: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> 
> Add a periodic timer which raises vblank at a frequency of 30Hz.
>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

 
> Signed-off-by: Sai Pavan Boddu <saipava@xilinx.com>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Changes by fkonrad:
>   - Switched to transaction-based ptimer API.
>   - Added the DP_INT_VBLNK_START macro.
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>
> ---
>  hw/display/xlnx_dp.c         | 24 +++++++++++++++++++++---
>  include/hw/display/xlnx_dp.h |  3 +++
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 0378570459..984b0a6bb9 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -114,6 +114,7 @@
>  #define DP_TX_N_AUD                         (0x032C >> 2)
>  #define DP_TX_AUDIO_EXT_DATA(n)             ((0x0330 + 4 * n) >> 2)
>  #define DP_INT_STATUS                       (0x03A0 >> 2)
> +#define DP_INT_VBLNK_START                  (1 << 13)
>  #define DP_INT_MASK                         (0x03A4 >> 2)
>  #define DP_INT_EN                           (0x03A8 >> 2)
>  #define DP_INT_DS                           (0x03AC >> 2)
> @@ -773,6 +774,14 @@ static void xlnx_dp_write(void *opaque, hwaddr offset, uint64_t value,
>          break;
>      case DP_TRANSMITTER_ENABLE:
>          s->core_registers[offset] = value & 0x01;
> +        ptimer_transaction_begin(s->vblank);
> +        if (value & 0x1) {
> +            ptimer_set_limit(s->vblank, 1, 1);
> +            ptimer_run(s->vblank, 0);
> +        } else {
> +            ptimer_stop(s->vblank);
> +        }
> +        ptimer_transaction_commit(s->vblank);
>          break;
>      case DP_FORCE_SCRAMBLER_RESET:
>          /*
> @@ -1177,9 +1186,6 @@ static void xlnx_dp_update_display(void *opaque)
>          return;
>      }
>  
> -    s->core_registers[DP_INT_STATUS] |= (1 << 13);
> -    xlnx_dp_update_irq(s);
> -
>      xlnx_dpdma_trigger_vsync_irq(s->dpdma);
>  
>      /*
> @@ -1275,6 +1281,14 @@ static void xlnx_dp_finalize(Object *obj)
>      fifo8_destroy(&s->rx_fifo);
>  }
>  
> +static void vblank_hit(void *opaque)
> +{
> +    XlnxDPState *s = XLNX_DP(opaque);
> +
> +    s->core_registers[DP_INT_STATUS] |= DP_INT_VBLNK_START;
> +    xlnx_dp_update_irq(s);
> +}
> +
>  static void xlnx_dp_realize(DeviceState *dev, Error **errp)
>  {
>      XlnxDPState *s = XLNX_DP(dev);
> @@ -1309,6 +1323,10 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
>                                             &as);
>      AUD_set_volume_out(s->amixer_output_stream, 0, 255, 255);
>      xlnx_dp_audio_activate(s);
> +    s->vblank = ptimer_init(vblank_hit, s, PTIMER_POLICY_DEFAULT);
> +    ptimer_transaction_begin(s->vblank);
> +    ptimer_set_freq(s->vblank, 30);
> +    ptimer_transaction_commit(s->vblank);
>  }
>  
>  static void xlnx_dp_reset(DeviceState *dev)
> diff --git a/include/hw/display/xlnx_dp.h b/include/hw/display/xlnx_dp.h
> index 1ef5a89ee7..e86a87f235 100644
> --- a/include/hw/display/xlnx_dp.h
> +++ b/include/hw/display/xlnx_dp.h
> @@ -35,6 +35,7 @@
>  #include "hw/dma/xlnx_dpdma.h"
>  #include "audio/audio.h"
>  #include "qom/object.h"
> +#include "hw/ptimer.h"
>  
>  #define AUD_CHBUF_MAX_DEPTH                 (32 * KiB)
>  #define MAX_QEMU_BUFFER_SIZE                (4 * KiB)
> @@ -107,6 +108,8 @@ struct XlnxDPState {
>       */
>      DPCDState *dpcd;
>      I2CDDCState *edid;
> +
> +    ptimer_state *vblank;
>  };
>  
>  #define TYPE_XLNX_DP "xlnx.v-dp"
> -- 
> 2.25.1
> 

Re: [PATCH v1 4/4] xlnx-zynqmp: fix the irq mapping for the display port and its dma

[Edgar E. Iglesias]

On Tue, May 03, 2022 at 04:25:45PM +0100, frederic.konrad@xilinx.com wrote:
> From: Frederic Konrad <fkonrad@amd.com>
> 
> When the display port has been initially implemented the device driver wasn't
> using interrupts.  Now that the display port driver waits for vblank interrupt
> it has been noticed that the irq mapping is wrong.  So use the value from the
> linux device tree and the ultrascale+ reference manual.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>


> 
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>
> ---
>  hw/arm/xlnx-zynqmp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 375309e68e..383e177a00 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -60,10 +60,10 @@
>  #define SERDES_SIZE         0x20000
>  
>  #define DP_ADDR             0xfd4a0000
> -#define DP_IRQ              113
> +#define DP_IRQ              0x77
>  
>  #define DPDMA_ADDR          0xfd4c0000
> -#define DPDMA_IRQ           116
> +#define DPDMA_IRQ           0x7a
>  
>  #define APU_ADDR            0xfd5c0000
>  #define APU_IRQ             153
> -- 
> 2.25.1
> 

RE: [PATCH v1 0/4] xlnx-zcu102: fix the display port.

[Frederic Konrad]

Ping

Can this be applied?  Here is the patchew link:
https://patchew.org/QEMU/20220503152545.1100386-1-fkonrad@xilinx.com/

Best Regards,
Fred

> -----Original Message-----
> From: frederic.konrad@xilinx.com <frederic.konrad@xilinx.com>
> Sent: 03 May 2022 16:26
> To: qemu-devel@nongnu.org
> Cc: alistair@alistair23.me; edgar.iglesias@gmail.com;
> peter.maydell@linaro.org; qemu-arm@nongnu.org; Sai Pavan Boddu
> <saipava@xilinx.com>; Edgar Iglesias <edgari@xilinx.com>;
> fkonrad@amd.com
> Subject: [PATCH v1 0/4] xlnx-zcu102: fix the display port.
> 
> From: Frederic Konrad <fkonrad@amd.com>
> 
> Hi,
> 
> This patch set fixes some issues with the DisplayPort for the ZCU102:
> 
> The first patch fixes the wrong register size and thus the risk of register
> overflow.
> 
> The three other one add a vblank interrupt required by the linux driver:
>   - When using the VNC graphic backend and leaving it unconnected, in the
> best
>     case the gfx_update callback is called once every 3000ms which is
>     insufficient for the driver.  This is fixed by providing a VBLANK interrupt
>     from a ptimer.
>   - This requirement revealed two issues with the IRQ numbers and the
>     interrupt disable logic fixed by the two last patches.
> 
> Tested by booting Petalinux with the framebuffer enabled.
> 
> Best Regards,
> Fred
> 
> Frederic Konrad (2):
>   xlnx_dp: fix the wrong register size
>   xlnx-zynqmp: fix the irq mapping for the display port and its dma
> 
> Sai Pavan Boddu (2):
>   xlnx_dp: Introduce a vblank signal
>   xlnx_dp: Fix the interrupt disable logic
> 
>  hw/arm/xlnx-zynqmp.c         |  4 ++--
>  hw/display/xlnx_dp.c         | 43 +++++++++++++++++++++++++++---------
>  include/hw/display/xlnx_dp.h | 12 ++++++++--
>  3 files changed, 44 insertions(+), 15 deletions(-)
> 
> --
> 2.25.1

Re: [PATCH v1 2/4] xlnx_dp: Introduce a vblank signal

[Peter Maydell]

On Tue, 3 May 2022 at 16:27, <frederic.konrad@xilinx.com> wrote:
>
> From: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
>
> Add a periodic timer which raises vblank at a frequency of 30Hz.
>
> Signed-off-by: Sai Pavan Boddu <saipava@xilinx.com>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Changes by fkonrad:
>   - Switched to transaction-based ptimer API.
>   - Added the DP_INT_VBLNK_START macro.
> Signed-off-by: Frederic Konrad <fkonrad@amd.com>
> ---


> @@ -1309,6 +1323,10 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp)
>                                             &as);
>      AUD_set_volume_out(s->amixer_output_stream, 0, 255, 255);
>      xlnx_dp_audio_activate(s);
> +    s->vblank = ptimer_init(vblank_hit, s, PTIMER_POLICY_DEFAULT);
> +    ptimer_transaction_begin(s->vblank);
> +    ptimer_set_freq(s->vblank, 30);
> +    ptimer_transaction_commit(s->vblank);

The ptimer documentation (in include/hw/ptimer.h) says
 * The default ptimer policy retains backward compatibility with the legacy
 * timers. Custom policies are adjusting the default one. Consider providing
 * a correct policy for your timer.

and goes on to describe various weird behaviours of the default
policy. You almost certainly don't want to use PTIMER_POLICY_DEFAULT
for a new timer -- instead figure out the behaviour you actually
want and specify the appropriate flags.

thanks
-- PMM

RE: [PATCH v1 2/4] xlnx_dp: Introduce a vblank signal

[Frederic Konrad]

> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: 16 May 2022 10:57
> To: Frederic Konrad <fkonrad@xilinx.com>
> Cc: qemu-devel@nongnu.org; alistair@alistair23.me;
> edgar.iglesias@gmail.com; qemu-arm@nongnu.org; Sai Pavan Boddu
> <saipava@xilinx.com>; Edgar Iglesias <edgari@xilinx.com>;
> fkonrad@amd.com; Sai Pavan Boddu <saipava@xilinx.com>; Edgar Iglesias
> <edgari@xilinx.com>
> Subject: Re: [PATCH v1 2/4] xlnx_dp: Introduce a vblank signal
> 
> On Tue, 3 May 2022 at 16:27, <frederic.konrad@xilinx.com> wrote:
> >
> > From: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> >
> > Add a periodic timer which raises vblank at a frequency of 30Hz.
> >
> > Signed-off-by: Sai Pavan Boddu <saipava@xilinx.com>
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > Changes by fkonrad:
> >   - Switched to transaction-based ptimer API.
> >   - Added the DP_INT_VBLNK_START macro.
> > Signed-off-by: Frederic Konrad <fkonrad@amd.com>
> > ---
> 
> 
> > @@ -1309,6 +1323,10 @@ static void xlnx_dp_realize(DeviceState *dev,
> Error **errp)
> >                                             &as);
> >      AUD_set_volume_out(s->amixer_output_stream, 0, 255, 255);
> >      xlnx_dp_audio_activate(s);
> > +    s->vblank = ptimer_init(vblank_hit, s, PTIMER_POLICY_DEFAULT);
> > +    ptimer_transaction_begin(s->vblank);
> > +    ptimer_set_freq(s->vblank, 30);
> > +    ptimer_transaction_commit(s->vblank);
> 
> The ptimer documentation (in include/hw/ptimer.h) says
>  * The default ptimer policy retains backward compatibility with the legacy
>  * timers. Custom policies are adjusting the default one. Consider providing
>  * a correct policy for your timer.
> 
> and goes on to describe various weird behaviours of the default
> policy. You almost certainly don't want to use PTIMER_POLICY_DEFAULT
> for a new timer -- instead figure out the behaviour you actually
> want and specify the appropriate flags.

Hi Peter,

Thanks for your feedback.

Yes, I think, I can just use CONTINUOUS_TRIGGER and NO_IMMEDIATE_TRIGGER
instead of forcing the decrementer / reload value to 1.  Would that be cleaner?

Thanks,
Fred

> 
> thanks
> -- PMM

Re: [PATCH v1 2/4] xlnx_dp: Introduce a vblank signal

[Peter Maydell]

On Mon, 16 May 2022 at 11:36, Frederic Konrad <fkonrad@xilinx.com> wrote:
> > From: Peter Maydell <peter.maydell@linaro.org>
> >
> > On Tue, 3 May 2022 at 16:27, <frederic.konrad@xilinx.com> wrote:
> > The ptimer documentation (in include/hw/ptimer.h) says
> >  * The default ptimer policy retains backward compatibility with the legacy
> >  * timers. Custom policies are adjusting the default one. Consider providing
> >  * a correct policy for your timer.
> >
> > and goes on to describe various weird behaviours of the default
> > policy. You almost certainly don't want to use PTIMER_POLICY_DEFAULT
> > for a new timer -- instead figure out the behaviour you actually
> > want and specify the appropriate flags.
>
> Hi Peter,
>
> Thanks for your feedback.
>
> Yes, I think, I can just use CONTINUOUS_TRIGGER and NO_IMMEDIATE_TRIGGER
> instead of forcing the decrementer / reload value to 1.  Would that be cleaner?

I don't know exactly what behaviour you want, but you should probably
also consider PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD.

-- PMM