Re: [PATCH v6 11/18] media: vsp1: drm: Implement writeback support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

On 13/03/2019 15:56, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Wed, Mar 13, 2019 at 11:42:34AM +0000, Kieran Bingham wrote:
>> On 13/03/2019 00:05, Laurent Pinchart wrote:
>>> Extend the vsp1_du_atomic_flush() API with writeback support by adding
>>> format, pitch and memory addresses of the writeback framebuffer.
>>> Writeback completion is reported through the existing frame completion
>>> callback with a new VSP1_DU_STATUS_WRITEBACK status flag.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>

My concerns have been addressed here:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>


>>> ---
>>>  drivers/media/platform/vsp1/vsp1_dl.c  | 14 ++++++++++++++
>>>  drivers/media/platform/vsp1/vsp1_dl.h  |  3 ++-
>>>  drivers/media/platform/vsp1/vsp1_drm.c | 25 ++++++++++++++++++++++++-
>>>  include/media/vsp1.h                   | 15 +++++++++++++++
>>>  4 files changed, 55 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
>>> index ed7cda4130f2..104b6f514536 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_dl.c
>>> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
>>> @@ -958,6 +958,9 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
>>>   *
>>>   * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
>>>   * became active had been queued with the internal notification flag.
>>> + *
>>> + * The VSP1_DL_FRAME_END_WRITEBACK flag indicates that the previously active
>>> + * display list had been queued with the writeback flag.
>>
>> How does this interact with the possibility of the writeback being
>> disabled by the WPF in the event of it failing to get a DL.
>>
>> It's only a small corner case, but will the 'writeback' report back as
>> though it succeeded? (without writing to memory, and thus giving an
>> unmodified buffer back?)
> 
> Wrteback completion will never be reported in that case. This shouldn't
> happen as we should never fail to get a display list. Do you think it
> would be better to fake completion ?

Would this lack of completion cause a hang while DRM waits for the
completion to occur? I guess this would timeout after some period.

I'm not sure what's worse. To hang / block for a timeout - or just
return a 'bad buffer'. We would know in the VSP that the completion has
failed, so we could signal a failure, but I think as the rest of the DRM
model goes with a timeout if the flip_done fails to complete for
example, we should follow that.

So leave this as is, and perhaps lets make sure the core writeback
framework will report a warning if it hits a time out.

If we ever fail to get a display list - we will have bigger issues which
will propogate elsewhere :)


>>>   */
>>>  unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>>>  {
>>> @@ -995,6 +998,17 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>>>  	if (status & VI6_STATUS_FLD_STD(dlm->index))
>>>  		goto done;
>>>  
>>> +	/*
>>> +	 * If the active display list has the writeback flag set, the frame
>>> +	 * completion marks the end of the writeback capture. Return the
>>> +	 * VSP1_DL_FRAME_END_WRITEBACK flag and reset the display list's
>>> +	 * writeback flag.
>>> +	 */
>>> +	if (dlm->active && (dlm->active->flags & VSP1_DL_FRAME_END_WRITEBACK)) {
>>> +		flags |= VSP1_DL_FRAME_END_WRITEBACK;
>>> +		dlm->active->flags &= ~VSP1_DL_FRAME_END_WRITEBACK;
>>> +	}
>>> +
>>>  	/*
>>>  	 * 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_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
>>> index e0fdb145e6ed..4d7bcfdc9bd9 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_dl.h
>>> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
>>> @@ -18,7 +18,8 @@ struct vsp1_dl_list;
>>>  struct vsp1_dl_manager;
>>>  
>>>  #define VSP1_DL_FRAME_END_COMPLETED		BIT(0)
>>> -#define VSP1_DL_FRAME_END_INTERNAL		BIT(1)
>>> +#define VSP1_DL_FRAME_END_WRITEBACK		BIT(1)
>>
>> So below BIT(2) (code above) the flags match the externally exposed
>> bitfield for the VSP1_DU_STATUS_
>>
>> While above (code below), are 'private' bitfields.
>>
>> Should this requirement be documented here somehow? especially the
>> mapping of FRAME_END_{COMPLETED,WRITEBACK} to
>> DU_STATUS_{COMPLETED,WRITEBACK}.
> 
> I've added a comment here, as explained in my reply to your review of
> 10/18, to document this.

Great.


>>> +#define VSP1_DL_FRAME_END_INTERNAL		BIT(2)
>>>  
>>>  /**
>>>   * struct vsp1_dl_ext_cmd - Extended Display command
>>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
>>> index 0367f88135bf..16826bf184c7 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
>>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
>>> @@ -37,7 +37,9 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
>>>  
>>>  	if (drm_pipe->du_complete) {
>>>  		struct vsp1_entity *uif = drm_pipe->uif;
>>> -		unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
>>> +		unsigned int status = completion
>>> +				    & (VSP1_DU_STATUS_COMPLETE |
>>> +				       VSP1_DU_STATUS_WRITEBACK);
>>>  		u32 crc;
>>>  
>>>  		crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
>>> @@ -541,6 +543,8 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
>>>  
>>>  	if (drm_pipe->force_brx_release)
>>>  		dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
>>> +	if (pipe->output->writeback)
>>> +		dl_flags |= VSP1_DL_FRAME_END_WRITEBACK;
>>>  
>>>  	dl = vsp1_dl_list_get(pipe->output->dlm);
>>>  	dlb = vsp1_dl_list_get_body0(dl);
>>> @@ -870,12 +874,31 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
>>>  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>>>  	struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
>>>  	struct vsp1_pipeline *pipe = &drm_pipe->pipe;
>>> +	int ret;
>>>  
>>>  	drm_pipe->crc = cfg->crc;
>>>  
>>>  	mutex_lock(&vsp1->drm->lock);
>>> +
>>> +	if (pipe->output->has_writeback && cfg->writeback.pixelformat) {
>>
>> Is pipe->output->has_writeback necessary here? Can
>> cfg->writeback.pixelformat be set if pipe->output->has_writeback is false?
>>
>> Hrm ... actually - Perhaps it is useful. It validates both sides of the
>> system.
>>
>> pipe->output->has_writeback is a capability of the VSP, where as
>> cfg->writeback.pixelformat is a 'request' from the DU.
> 
> Correct, I think it's best to check both, to ensure we don't try to
> queue a writeback request on a system that doesn't support writeback. On
> the other hand this shouldn't happen as the DU driver shouldn't expose
> writeback to userspace in that case, so if you don't think the check is
> worth it I can remove the has_writeback field completely.

It's a cheap check, I don't think it is too much of an issue - but I
agree (if we don't already) then we should make sure userspace does not
see a writeback functionality if it is not supported through the whole
pipeline (i.e. including the capability in the VSP1).

That would make me lean towards removing this check here - *iff* we
guarantee that the VSP will only be asked to do write back when it's
possible.



>>> +		const struct vsp1_du_writeback_config *wb_cfg = &cfg->writeback;
>>> +
>>> +		ret = vsp1_du_pipeline_set_rwpf_format(vsp1, pipe->output,
>>> +						       wb_cfg->pixelformat,
>>> +						       wb_cfg->pitch);
>>> +		if (WARN_ON(ret < 0))
>>> +			goto done;
>>> +
>>> +		pipe->output->mem.addr[0] = wb_cfg->mem[0];
>>> +		pipe->output->mem.addr[1] = wb_cfg->mem[1];
>>> +		pipe->output->mem.addr[2] = wb_cfg->mem[2];
>>> +		pipe->output->writeback = true;
>>> +	}
>>> +
>>>  	vsp1_du_pipeline_setup_inputs(vsp1, pipe);
>>>  	vsp1_du_pipeline_configure(pipe);
>>> +
>>> +done:
>>>  	mutex_unlock(&vsp1->drm->lock);
>>>  }
>>>  EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
>>> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
>>> index 877496936487..cc1b0d42ce95 100644
>>> --- a/include/media/vsp1.h
>>> +++ b/include/media/vsp1.h
>>> @@ -18,6 +18,7 @@ struct device;
>>>  int vsp1_du_init(struct device *dev);
>>>  
>>>  #define VSP1_DU_STATUS_COMPLETE		BIT(0)
>>> +#define VSP1_DU_STATUS_WRITEBACK	BIT(1)
>>>  
>>>  /**
>>>   * struct vsp1_du_lif_config - VSP LIF configuration
>>> @@ -83,12 +84,26 @@ struct vsp1_du_crc_config {
>>>  	unsigned int index;
>>>  };
>>>  
>>> +/**
>>> + * struct vsp1_du_writeback_config - VSP writeback configuration parameters
>>> + * @pixelformat: plane pixel format (V4L2 4CC)
>>> + * @pitch: line pitch in bytes for the first plane
>>> + * @mem: DMA memory address for each plane of the frame buffer
>>> + */
>>> +struct vsp1_du_writeback_config {
>>> +	u32 pixelformat;
>>> +	unsigned int pitch;
>>> +	dma_addr_t mem[3];
>>> +};
>>> +
>>>  /**
>>>   * struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration parameters
>>>   * @crc: CRC computation configuration
>>> + * @writeback: writeback configuration
>>>   */
>>>  struct vsp1_du_atomic_pipe_config {
>>>  	struct vsp1_du_crc_config crc;
>>> +	struct vsp1_du_writeback_config writeback;
>>>  };
>>>  
>>>  void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);
> 

-- 
Regards
--
Kieran



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux