Re: [PATCH v4 10/11] media: vsp1: Support Interlaced display pipelines

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thursday, 3 May 2018 16:36:21 EEST Kieran Bingham wrote:
> Calculate the top and bottom fields for the interlaced frames and
> utilise the extended display list command feature to implement the
> auto-field operations. This allows the DU to update the VSP2 registers
> dynamically based upon the currently processing field.
> 
> Signed-off-by: Kieran Bingham 
> 
> ---
> v3:
>  - Pass DL through partition calls to allow autocmd's to be retrieved
>  - Document interlaced field in struct vsp1_du_atomic_config
> 
> v2:
>  - fix erroneous BIT value which enabled interlaced
>  - fix field handling at frame_end interrupt
> ---
>  drivers/media/platform/vsp1/vsp1_dl.c   | 10 -
>  drivers/media/platform/vsp1/vsp1_drm.c  | 11 -
>  drivers/media/platform/vsp1/vsp1_regs.h |  1 +-
>  drivers/media/platform/vsp1/vsp1_rpf.c  | 71 --
>  drivers/media/platform/vsp1/vsp1_rwpf.h |  1 +-
>  include/media/vsp1.h|  2 +-
>  6 files changed, 93 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index d33ae5f125bd..bbe9f3006f71
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -906,6 +906,8 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool
> internal) */
>  unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  {
> + struct vsp1_device *vsp1 = dlm->vsp1;
> + u32 status = vsp1_read(vsp1, VI6_STATUS);
>   unsigned int flags = 0;
> 
>   spin_lock(>lock);
> @@ -931,6 +933,14 @@ unsigned int vsp1_dlm_irq_frame_end(struct
> vsp1_dl_manager *dlm) goto done;
> 
>   /*
> +  * Progressive streams report only TOP fields. If we have a BOTTOM
> +  * field, we are interlaced, and expect the frame to complete on the
> +  * next frame end interrupt.
> +  */
> + if (status & VI6_STATUS_FLD_STD(dlm->index))
> + goto done;
> +
> + /*
>* The device starts processing the queued display list right after the
>* frame end interrupt. The display list thus becomes active.
>*/
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index 2c3db8b8adce..cc29c9d96bb7
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -811,6 +811,17 @@ int vsp1_du_atomic_update(struct device *dev, unsigned
> int pipe_index, return -EINVAL;
>   }
> 
> + if (!(vsp1_feature(vsp1, VSP1_HAS_EXT_DL)) && cfg->interlaced) {

Nitpicking, writing the condition as

if (cfg->interlaced && !(vsp1_feature(vsp1, VSP1_HAS_EXT_DL)))

would match the comment better. You can also drop the parentheses around the 
vsp1_feature() call.

> + /*
> +  * Interlaced support requires extended display lists to
> +  * provide the auto-fld feature with the DU.
> +  */
> + dev_dbg(vsp1->dev, "Interlaced unsupported on this output\n");

Could we catch this in the DU driver to fail atomic test ?

> + return -EINVAL;
> + }
> +
> + rpf->interlaced = cfg->interlaced;
> +
>   rpf->fmtinfo = fmtinfo;
>   rpf->format.num_planes = fmtinfo->planes;
>   rpf->format.plane_fmt[0].bytesperline = cfg->pitch;
> diff --git a/drivers/media/platform/vsp1/vsp1_regs.h
> b/drivers/media/platform/vsp1/vsp1_regs.h index d054767570c1..a2ac65cc5155
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> @@ -28,6 +28,7 @@
>  #define VI6_SRESET_SRTS(n)   (1 << (n))
> 
>  #define VI6_STATUS   0x0038
> +#define VI6_STATUS_FLD_STD(n)(1 << ((n) + 28))
>  #define VI6_STATUS_SYS_ACT(n)(1 << ((n) + 8))
> 
>  #define VI6_WPF_IRQ_ENB(n)   (0x0048 + (n) * 12)
> diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c
> b/drivers/media/platform/vsp1/vsp1_rpf.c index 8fae7c485642..511b2e127820
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rpf.c
> @@ -20,6 +20,20 @@
>  #define RPF_MAX_WIDTH8190
>  #define RPF_MAX_HEIGHT   8190
> 
> +/* Pre extended display list command data structure */
> +struct vsp1_extcmd_auto_fld_body {
> + u32 top_y0;
> + u32 bottom_y0;
> + u32 top_c0;
> + u32 bottom_c0;
> + u32 top_c1;
> + u32 bottom_c1;
> + u32 reserved0;
> + u32 reserved1;
> +} __packed;
> +
> +#define VSP1_DL_EXT_AUTOFLD_INT  BIT(0)

Should the bit definition be moved to vsp1_regs.h ?

>  /* 
>   * Device Access
>   */
> @@ -64,6 +78,14 @@ static void rpf_configure_stream(struct vsp1_entity
> *entity, pstride |= format->plane_fmt[1].bytesperline
>   

[PATCH v4 10/11] media: vsp1: Support Interlaced display pipelines

[Kieran Bingham]

Calculate the top and bottom fields for the interlaced frames and
utilise the extended display list command feature to implement the
auto-field operations. This allows the DU to update the VSP2 registers
dynamically based upon the currently processing field.

Signed-off-by: Kieran Bingham 

---
v3:
 - Pass DL through partition calls to allow autocmd's to be retrieved
 - Document interlaced field in struct vsp1_du_atomic_config

v2:
 - fix erroneous BIT value which enabled interlaced
 - fix field handling at frame_end interrupt
---
 drivers/media/platform/vsp1/vsp1_dl.c   | 10 -
 drivers/media/platform/vsp1/vsp1_drm.c  | 11 -
 drivers/media/platform/vsp1/vsp1_regs.h |  1 +-
 drivers/media/platform/vsp1/vsp1_rpf.c  | 71 --
 drivers/media/platform/vsp1/vsp1_rwpf.h |  1 +-
 include/media/vsp1.h|  2 +-
 6 files changed, 93 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
b/drivers/media/platform/vsp1/vsp1_dl.c
index d33ae5f125bd..bbe9f3006f71 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -906,6 +906,8 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool 
internal)
  */
 unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 {
+   struct vsp1_device *vsp1 = dlm->vsp1;
+   u32 status = vsp1_read(vsp1, VI6_STATUS);
unsigned int flags = 0;
 
spin_lock(>lock);
@@ -931,6 +933,14 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager 
*dlm)
goto done;
 
/*
+* Progressive streams report only TOP fields. If we have a BOTTOM
+* field, we are interlaced, and expect the frame to complete on the
+* next frame end interrupt.
+*/
+   if (status & VI6_STATUS_FLD_STD(dlm->index))
+   goto done;
+
+   /*
 * The device starts processing the queued display list right after the
 * frame end interrupt. The display list thus becomes active.
 */
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index 2c3db8b8adce..cc29c9d96bb7 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -811,6 +811,17 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int 
pipe_index,
return -EINVAL;
}
 
+   if (!(vsp1_feature(vsp1, VSP1_HAS_EXT_DL)) && cfg->interlaced) {
+   /*
+* Interlaced support requires extended display lists to
+* provide the auto-fld feature with the DU.
+*/
+   dev_dbg(vsp1->dev, "Interlaced unsupported on this output\n");
+   return -EINVAL;
+   }
+
+   rpf->interlaced = cfg->interlaced;
+
rpf->fmtinfo = fmtinfo;
rpf->format.num_planes = fmtinfo->planes;
rpf->format.plane_fmt[0].bytesperline = cfg->pitch;
diff --git a/drivers/media/platform/vsp1/vsp1_regs.h 
b/drivers/media/platform/vsp1/vsp1_regs.h
index d054767570c1..a2ac65cc5155 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -28,6 +28,7 @@
 #define VI6_SRESET_SRTS(n) (1 << (n))
 
 #define VI6_STATUS 0x0038
+#define VI6_STATUS_FLD_STD(n)  (1 << ((n) + 28))
 #define VI6_STATUS_SYS_ACT(n)  (1 << ((n) + 8))
 
 #define VI6_WPF_IRQ_ENB(n) (0x0048 + (n) * 12)
diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c 
b/drivers/media/platform/vsp1/vsp1_rpf.c
index 8fae7c485642..511b2e127820 100644
--- a/drivers/media/platform/vsp1/vsp1_rpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rpf.c
@@ -20,6 +20,20 @@
 #define RPF_MAX_WIDTH  8190
 #define RPF_MAX_HEIGHT 8190
 
+/* Pre extended display list command data structure */
+struct vsp1_extcmd_auto_fld_body {
+   u32 top_y0;
+   u32 bottom_y0;
+   u32 top_c0;
+   u32 bottom_c0;
+   u32 top_c1;
+   u32 bottom_c1;
+   u32 reserved0;
+   u32 reserved1;
+} __packed;
+
+#define VSP1_DL_EXT_AUTOFLD_INTBIT(0)
+
 /* 
-
  * Device Access
  */
@@ -64,6 +78,14 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
pstride |= format->plane_fmt[1].bytesperline
<< VI6_RPF_SRCM_PSTRIDE_C_SHIFT;
 
+   /*
+* pstride has both STRIDE_Y and STRIDE_C, but multiplying the whole
+* of pstride by 2 is conveniently OK here as we are multiplying both
+* values.
+*/
+   if (rpf->interlaced)
+   pstride *= 2;
+
vsp1_rpf_write(rpf, dlb, VI6_RPF_SRCM_PSTRIDE, pstride);
 
/* Format */
@@ -100,6 +122,9 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
top = compose->top;
}
 
+   if