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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

On Monday, 16 July 2018 21:21:00 EEST Kieran Bingham wrote:
> On 24/05/18 13:51, Laurent Pinchart wrote:
> > 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 <kieran.bingham+renesas@ideasonboard.com>
> >> 
> >> ---
> >> 
> >> 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(-)

[snip]

> >> 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)))
> 
> Done.
> 
> > 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 ?
> 
> Ugh - I thought moving the configuration to vsp1_du_setup_lif() would
> give us this, but that return value is not checked in the DU.
> 
> How can we interogate the VSP1 to ask it if it supports interlaced from
> rcar_du_vsp_plane_atomic_check()?
> 
> 
> Some dummy call to vsp1_du_setup_lif() to check the return value ? Or
> should we implement a hook to call through to perform checks in the VSP1
> DRM API?

Would it be possible to just infer that from the DU compatible string, without 
querying the VSP driver ? Of course that's a bit of a layering violation, but 
as we know what type of VSP instance is present in each SoC, such a small hack 
wouldn't hurt in my opinion. If the need arises later we can introduce an API 
to query the information from the VSP driver.

> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	rpf->interlaced = cfg->interlaced;
> >> +
> >>  	rpf->fmtinfo = fmtinfo;
> >>  	rpf->format.num_planes = fmtinfo->planes;
> >>  	rpf->format.plane_fmt[0].bytesperline = cfg->pitch;

[snip]

-- 
Regards,

Laurent Pinchart