Hi Laurent,
On 17/07/18 13:52, Laurent Pinchart wrote:
> 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@xxxxxxxxxxxxxxxx>
>>>>
>>>> ---
>>>>
>>>> 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.
I'm not sure what there is to match on currently.
I thought that we had restrictions on which display pipelines supported
interlaced. (i.e. D3/E3 might not) - but they seem to support extended
display lists ...
So isn't it the case that any pipeline which we connect to DRM supports
interlaced? (currently) - we can't / don't physically connect other VSP
entities to the DRM pipes...
>
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + rpf->interlaced = cfg->interlaced;
>>>> +
>>>> rpf->fmtinfo = fmtinfo;
>>>> rpf->format.num_planes = fmtinfo->planes;
>>>> rpf->format.plane_fmt[0].bytesperline = cfg->pitch;
>
> [snip]
