Re: [PATCH v2 5/8] v4l: vsp1: Refactor display list configure operations

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

 



Hi Laurent,

Just a query on your bikeshedding here.

Choose your colours wisely :)

--
Kieran

On 12/09/17 20:19, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Tuesday, 12 September 2017 00:16:50 EEST Kieran Bingham wrote:
>> On 17/08/17 19:13, Laurent Pinchart wrote:
>>> On Monday 14 Aug 2017 16:13:28 Kieran Bingham wrote:
>>>> The entities provide a single .configure operation which configures the
>>>> object into the target display list, based on the vsp1_entity_params
>>>> selection.
>>>>
>>>> This restricts us to a single function prototype for both static
>>>> configuration (the pre-stream INIT stage) and the dynamic runtime stages
>>>> for both each frame - and each partition therein.
>>>>
>>>> Split the configure function into two parts, '.prepare()' and
>>>> '.configure()', merging both the VSP1_ENTITY_PARAMS_RUNTIME and
>>>> VSP1_ENTITY_PARAMS_PARTITION stages into a single call through the
>>>> .configure(). The configuration for individual partitions is handled by
>>>> passing the partition number to the configure call, and processing any
>>>> runtime stage actions on the first partition only.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>>>> ---
>>>>
>>>>  drivers/media/platform/vsp1/vsp1_bru.c    |  12 +-
>>>>  drivers/media/platform/vsp1/vsp1_clu.c    |  43 +--
>>>>  drivers/media/platform/vsp1/vsp1_drm.c    |  11 +-
>>>>  drivers/media/platform/vsp1/vsp1_entity.c |  15 +-
>>>>  drivers/media/platform/vsp1/vsp1_entity.h |  27 +--
>>>>  drivers/media/platform/vsp1/vsp1_hgo.c    |  12 +-
>>>>  drivers/media/platform/vsp1/vsp1_hgt.c    |  12 +-
>>>>  drivers/media/platform/vsp1/vsp1_hsit.c   |  12 +-
>>>>  drivers/media/platform/vsp1/vsp1_lif.c    |  12 +-
>>>>  drivers/media/platform/vsp1/vsp1_lut.c    |  24 +-
>>>>  drivers/media/platform/vsp1/vsp1_rpf.c    | 162 ++++++-------
>>>>  drivers/media/platform/vsp1/vsp1_sru.c    |  12 +-
>>>>  drivers/media/platform/vsp1/vsp1_uds.c    |  55 ++--
>>>>  drivers/media/platform/vsp1/vsp1_video.c  |  24 +--
>>>>  drivers/media/platform/vsp1/vsp1_wpf.c    | 297 ++++++++++++-----------
>>>>  15 files changed, 359 insertions(+), 371 deletions(-)
>>>
>>> [snip]
>>>
>>>> diff --git a/drivers/media/platform/vsp1/vsp1_clu.c
>>>> b/drivers/media/platform/vsp1/vsp1_clu.c index 175717018e11..5f65ce3ad97f
>>>> 100644
>>>> --- a/drivers/media/platform/vsp1/vsp1_clu.c
>>>> +++ b/drivers/media/platform/vsp1/vsp1_clu.c
>>>> @@ -213,37 +213,37 @@ static const struct v4l2_subdev_ops clu_ops = {
>>>>
>>>>  /*
>>>>  -----------------------------------------------------------------------
>>>>  -
>>>>  
>>>>   * VSP1 Entity Operations
>>>>   */
>>>>
>>>> +static void clu_prepare(struct vsp1_entity *entity,
>>>> +			struct vsp1_pipeline *pipe,
>>>> +			struct vsp1_dl_list *dl)
>>>> +{
>>>> +	struct vsp1_clu *clu = to_clu(&entity->subdev);
>>>> +
>>>> +	/*
>>>> +	 * The format can't be changed during streaming, only verify it
>>>> +	 * at setup time and store the information internally for future
>>>> +	 * runtime configuration calls.
>>>> +	 */
>>>
>>> I know you're just moving the comment around, but let's fix it at the same
>>> time. There's no verification here (and no "setup time" either). I'd write
>>> it as
>>>
>>> 	/*
>>> 	
>>> 	 * The format can't be changed during streaming. Cache it internally
>>> 	 * for future runtime configuration calls.
>>> 	 */
>>
>> I think I'm ok with that and I've updated the patch - but I'm not sure we
>> are really caching the 'format' here, as much as the yuv_mode ...
> 
> Yes, it's the YUV mode we're caching, feel free to update the comment.

Done.

> 
>> I'll ponder ...
>>
>>>> +	struct v4l2_mbus_framefmt *format;
>>>> +
>>>> +	format = vsp1_entity_get_pad_format(&clu->entity,
>>>> +					    clu->entity.config,
>>>> +					    CLU_PAD_SINK);
>>>> +	clu->yuv_mode = format->code == MEDIA_BUS_FMT_AYUV8_1X32;
>>>> +}
>>>
>>> [snip]
>>>
>>>> diff --git a/drivers/media/platform/vsp1/vsp1_entity.h
>>>> b/drivers/media/platform/vsp1/vsp1_entity.h index
>>>> 408602ebeb97..2f33e343ccc6 100644
>>>> --- a/drivers/media/platform/vsp1/vsp1_entity.h
>>>> +++ b/drivers/media/platform/vsp1/vsp1_entity.h
>>>
>>> [snip]
>>>
>>>> @@ -80,8 +68,10 @@ struct vsp1_route {
>>>>
>>>>  /**
>>>>  
>>>>   * struct vsp1_entity_operations - Entity operations
>>>>   * @destroy:	Destroy the entity.
>>>>
>>>> - * @configure:	Setup the hardware based on the entity state
>>>> (pipeline, formats,
>>>> - *		selection rectangles, ...)
>>>> + * @prepare:	Setup the initial hardware parameters for the stream
>>>> (pipeline,
>>>> + *		formats)
>>>> + * @configure:	Configure the runtime parameters for each partition
>>>> (rectangles,
>>>> + *		buffer addresses, ...)
>>>
>>> Now moving to the bikeshedding territory, I'm not sure if prepare and
>>> configure are the best names for those operations.


Would init() and configure() be more suitable for you ?

Or 'setup()' and 'configure() or perhaps 'runtime()' ?

I'm not convinced on either init() or setup() yet, as they might refer to
'initialising' the object, rather than portraying the configuration of the
object into a body...

>>> I'd like to also point
>>> out that we could go one step further by caching the partition-related
>>> parameters too, in which case we would need a third operation (or
>>> possibly passing the partition number to the prepare operation). While I
>>> won't mind if you implement this now, the issue could also be addressed
>>> later, but I'd like the operations to already support that use case to
>>> avoid yet another painful rename patch.
>>
>> Ok, understood - but I think I'll have to defer to a v4 for now ... I'm
>> running out of time.>>>
>>>>   * @max_width:	Return the max supported width of data that the entity
>>>>
>>>> can
>>>>
>>>>   *		process in a single operation.
>>>>   * @partition:	Process the partition construction based on this
>>>>
>>>> entity's
>>>
>>> [snip]
>>>
>>> The rest of the patch looks good to me.
> 



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux