Re: [RFC PATCH 2/3] v4l: vsp1: extend VSP1 module API to allow DRM callback registration

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

 



Hi Laurent,

On 03/03/17 02:11, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday 01 Mar 2017 13:12:55 Kieran Bingham wrote:
>> To be able to perform page flips in DRM without flicker we need to be
>> able to notify the rcar-du module when the VSP has completed its
>> processing.
>>
>> To synchronise the page flip events for userspace, we move the required
>> event through the VSP to track the data flow. When the frame is
>> completed, the event can be returned back to the originator through the
>> registered callback.
>>
>> We must not have bidirectional dependencies on the two components to
>> maintain support for loadable modules, thus we extend the API to allow
>> a callback to be registered within the VSP DRM interface.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  |  2 +-
>>  drivers/media/platform/vsp1/vsp1_drm.c | 42 +++++++++++++++++++++++++--
>>  drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++++++-
>>  include/media/vsp1.h                   |  6 +++-
>>  4 files changed, 58 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b5bfbe50bd87..71e70e1e0881
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> @@ -81,7 +81,7 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
>>
>>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
>>  {
>> -	vsp1_du_atomic_flush(crtc->vsp->vsp);
>> +	vsp1_du_atomic_flush(crtc->vsp->vsp, NULL);
>>  }
>>
>>  /* Keep the two tables in sync. */
>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
>> b/drivers/media/platform/vsp1/vsp1_drm.c index 8e2aa3f8e52f..743cbce48d0c
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
 >> @@ -52,6 +52,40 @@ int vsp1_du_init(struct device *dev)
>>  EXPORT_SYMBOL_GPL(vsp1_du_init);
>>
>>  /**
>> + * vsp1_du_register_callback - Register VSP completion notifier callback
>> + *
>> + * Allow the DRM framework to register a callback with us to notify the end
>> of + * processing each frame. This allows synchronisation for page
>> flipping. + *
>> + * @dev: the VSP device
>> + * @callback: the callback function to notify the DU module
>> + * @private: private structure data to pass with the callback
>> + *
>> + */
>> +void vsp1_du_register_callback(struct device *dev,
>> +			       void (*callback)(void *, void *),
>> +			       void *private)
>> +{
>> +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>> +
>> +	vsp1->drm->du_complete = callback;
>> +	vsp1->drm->du_private = private;
>> +}
>> +EXPORT_SYMBOL_GPL(vsp1_du_register_callback);
> 
> As they're not supposed to change at runtime while the display is running, how 
> about passing the callback and private data pointer to the vsp1_du_setup_lif() 
> function ? Feel free to create a structure for all the parameters passed to 
> the function if you think we'll have too much (which would, as a side effect, 
> made updates to the API easier in the future as changes to the two subsystems 
> will be easier to decouple).

Sure, that's fine. I think a config structure makes sense here.

>> +static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
>> +{
>> +	struct vsp1_drm *drm = to_vsp1_drm(pipe);
>> +
>> +	if (drm->du_complete && drm->active_data)
>> +		drm->du_complete(drm->du_private, drm->active_data);
>> +
>> +	/* The pending frame is now active */
>> +	drm->active_data = drm->pending_data;
>> +	drm->pending_data = NULL;
>> +}
> 
> I would move this function to the "Interrupt Handling" section.

Ack.

>> +/**
>>   * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
>>   * @dev: the VSP device
>>   * @width: output frame width in pixels
>> @@ -99,7 +133,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
>> width, }
>>
>>  		pipe->num_inputs = 0;
>> -
>> +		pipe->frame_end = NULL;
> 
> You can drop this if ...
> 
>> +		vsp1->drm->du_complete = NULL;
>>  		vsp1_dlm_reset(pipe->output->dlm);
>>  		vsp1_device_put(vsp1);
>>
>> @@ -196,6 +231,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
>> width, if (ret < 0)
>>  		return ret;
>>
>> +	pipe->frame_end = vsp1_du_pipeline_frame_end;
>> +

> 
> ... you move this to vsp1_drm_init().

Done.


> 
>>  	ret = media_entity_pipeline_start(&pipe->output->entity.subdev.entity,
>>  					  &pipe->pipe);
>>  	if (ret < 0) {
>> @@ -420,7 +457,7 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1,
>> struct vsp1_rwpf *rpf) * vsp1_du_atomic_flush - Commit an atomic update
>>   * @dev: the VSP device
>>   */
>> -void vsp1_du_atomic_flush(struct device *dev)
>> +void vsp1_du_atomic_flush(struct device *dev, void *data)
>>  {
>>  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>>  	struct vsp1_pipeline *pipe = &vsp1->drm->pipe;
>> @@ -504,6 +541,7 @@ void vsp1_du_atomic_flush(struct device *dev)
>>
>>  	vsp1_dl_list_commit(pipe->dl);
>>  	pipe->dl = NULL;
>> +	vsp1->drm->pending_data = data;
>>
>>  	/* Start or stop the pipeline if needed. */
>>  	if (!vsp1->drm->num_inputs && pipe->num_inputs) {
>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
>> b/drivers/media/platform/vsp1/vsp1_drm.h index 9e28ab9254ba..fde19e5948a0
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drm.h
>> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
>> @@ -33,8 +33,20 @@ struct vsp1_drm {
>>  		struct v4l2_rect compose;
>>  		unsigned int zpos;
>>  	} inputs[VSP1_MAX_RPF];
>> +
>> +	/* Frame syncronisation */
>> +	void (*du_complete)(void *, void *);
>> +	void *du_private;
>> +	void *pending_data;
>> +	void *active_data;
>>  };
>>
>> +static inline struct vsp1_drm *
>> +to_vsp1_drm(struct vsp1_pipeline *pipe)
> 
> No need for a line split.

Fixed.

>> +{
>> +	return container_of(pipe, struct vsp1_drm, pipe);
>> +}
>> +
>>  int vsp1_drm_init(struct vsp1_device *vsp1);
>>  void vsp1_drm_cleanup(struct vsp1_device *vsp1);
>>  int vsp1_drm_create_links(struct vsp1_device *vsp1);
>> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
>> index 458b400373d4..f82fbab01f21 100644
>> --- a/include/media/vsp1.h
>> +++ b/include/media/vsp1.h
>> @@ -20,6 +20,10 @@ struct device;
>>
>>  int vsp1_du_init(struct device *dev);
>>
>> +void vsp1_du_register_callback(struct device *dev,
>> +			       void (*callback)(void *, void *),
>> +			       void *private);
>> +
>>  int vsp1_du_setup_lif(struct device *dev, unsigned int width,
>>  		      unsigned int height);
>>
>> @@ -36,6 +40,6 @@ struct vsp1_du_atomic_config {
>>  void vsp1_du_atomic_begin(struct device *dev);
>>  int vsp1_du_atomic_update(struct device *dev, unsigned int rpf,
>>  			  const struct vsp1_du_atomic_config *cfg);
>> -void vsp1_du_atomic_flush(struct device *dev);
>> +void vsp1_du_atomic_flush(struct device *dev, void *data);
>>
>>  #endif /* __MEDIA_VSP1_H__ */
> 




[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