Re: [PATCH v4 10/11] media: vsp1: Support Interlaced display pipelines
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
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