Re: [PATCH v7 6/8] media: vsp1: Refactor display list configure operations
Hi Laurent, On 07/04/18 00:38, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thursday, 8 March 2018 02:05:29 EEST 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, '.configure_stream()' and >> '.configure_frame()', merging both the VSP1_ENTITY_PARAMS_RUNTIME and >> VSP1_ENTITY_PARAMS_PARTITION stages into a single call through the >> .configure_frame(). 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 >> >> --- >> v7 >> - Fix formatting and white space >> - s/prepare/configure_stream/ >> - s/configure/configure_frame/ >> >> drivers/media/platform/vsp1/vsp1_bru.c| 12 +- >> drivers/media/platform/vsp1/vsp1_clu.c| 50 +--- >> drivers/media/platform/vsp1/vsp1_dl.h | 1 +- >> drivers/media/platform/vsp1/vsp1_drm.c| 21 +-- >> drivers/media/platform/vsp1/vsp1_entity.c | 17 +- >> drivers/media/platform/vsp1/vsp1_entity.h | 33 +-- >> 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| 32 +- >> drivers/media/platform/vsp1/vsp1_rpf.c| 164 ++--- >> drivers/media/platform/vsp1/vsp1_sru.c| 12 +- >> drivers/media/platform/vsp1/vsp1_uds.c| 57 ++-- >> drivers/media/platform/vsp1/vsp1_video.c | 24 +-- >> drivers/media/platform/vsp1/vsp1_wpf.c| 299 --- >> 16 files changed, 378 insertions(+), 392 deletions(-) > > [snip] > >> diff --git a/drivers/media/platform/vsp1/vsp1_clu.c >> b/drivers/media/platform/vsp1/vsp1_clu.c index b2a39a6ef7e4..b8d8af6d4910 >> 100644 >> --- a/drivers/media/platform/vsp1/vsp1_clu.c >> +++ b/drivers/media/platform/vsp1/vsp1_clu.c >> @@ -213,37 +213,36 @@ static const struct v4l2_subdev_ops clu_ops = { >> /* >> --- >> -- * VSP1 Entity Operations >> */ >> +static void clu_configure_stream(struct vsp1_entity *entity, >> + struct vsp1_pipeline *pipe, >> + struct vsp1_dl_list *dl) >> +{ >> +struct vsp1_clu *clu = to_clu(&entity->subdev); >> + >> +/* >> + * The yuv_mode can't be changed during streaming. Cache it internally >> + * for future runtime configuration calls. >> + */ > > I'd move this comment right before the vsp1_entity_get_pad_format() call to > keep all variable declarations together. Agreed, Done. > >> +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_dl.h >> b/drivers/media/platform/vsp1/vsp1_dl.h index 7e820ac6865a..f45083251644 >> 100644 >> --- a/drivers/media/platform/vsp1/vsp1_dl.h >> +++ b/drivers/media/platform/vsp1/vsp1_dl.h >> @@ -41,7 +41,6 @@ vsp1_dl_body_pool_create(struct vsp1_device *vsp1, >> unsigned int num_bodies, void vsp1_dl_body_pool_destroy(struct >> vsp1_dl_body_pool *pool); >> struct vsp1_dl_body *vsp1_dl_body_get(struct vsp1_dl_body_pool *pool); >> void vsp1_dl_body_put(struct vsp1_dl_body *dlb); >> - > > This is an unrelated change. > Removed >> void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32 reg, u32 data); >> int vsp1_dl_list_add_body(struct vsp1_dl_list *dl, struct vsp1_dl_body >> *dlb); >> int vsp1_dl_list_add_chain(struct vsp1_dl_list *head, struct vsp1_dl_list >> *dl); > > [snip] > >> diff --git a/drivers/media/platform/vsp1/vsp1_entity.h >> b/drivers/media/platform/vsp1/vsp1_entity.h index >> 408602ebeb97..b44ed5414fc3 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, ...) >> + * @configure_stream: Setup the initial hardware parameters for the > stream >> + * (pipeline, formats) > > Instead of initial I would say "Setup hardware paramete
Re: [PATCH v7 6/8] media: vsp1: Refactor display list configure operations
Hi Kieran, Thank you for the patch. On Thursday, 8 March 2018 02:05:29 EEST 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, '.configure_stream()' and > '.configure_frame()', merging both the VSP1_ENTITY_PARAMS_RUNTIME and > VSP1_ENTITY_PARAMS_PARTITION stages into a single call through the > .configure_frame(). 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 > > --- > v7 > - Fix formatting and white space > - s/prepare/configure_stream/ > - s/configure/configure_frame/ > > drivers/media/platform/vsp1/vsp1_bru.c| 12 +- > drivers/media/platform/vsp1/vsp1_clu.c| 50 +--- > drivers/media/platform/vsp1/vsp1_dl.h | 1 +- > drivers/media/platform/vsp1/vsp1_drm.c| 21 +-- > drivers/media/platform/vsp1/vsp1_entity.c | 17 +- > drivers/media/platform/vsp1/vsp1_entity.h | 33 +-- > 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| 32 +- > drivers/media/platform/vsp1/vsp1_rpf.c| 164 ++--- > drivers/media/platform/vsp1/vsp1_sru.c| 12 +- > drivers/media/platform/vsp1/vsp1_uds.c| 57 ++-- > drivers/media/platform/vsp1/vsp1_video.c | 24 +-- > drivers/media/platform/vsp1/vsp1_wpf.c| 299 --- > 16 files changed, 378 insertions(+), 392 deletions(-) [snip] > diff --git a/drivers/media/platform/vsp1/vsp1_clu.c > b/drivers/media/platform/vsp1/vsp1_clu.c index b2a39a6ef7e4..b8d8af6d4910 > 100644 > --- a/drivers/media/platform/vsp1/vsp1_clu.c > +++ b/drivers/media/platform/vsp1/vsp1_clu.c > @@ -213,37 +213,36 @@ static const struct v4l2_subdev_ops clu_ops = { > /* > --- > -- * VSP1 Entity Operations > */ > +static void clu_configure_stream(struct vsp1_entity *entity, > + struct vsp1_pipeline *pipe, > + struct vsp1_dl_list *dl) > +{ > + struct vsp1_clu *clu = to_clu(&entity->subdev); > + > + /* > + * The yuv_mode can't be changed during streaming. Cache it internally > + * for future runtime configuration calls. > + */ I'd move this comment right before the vsp1_entity_get_pad_format() call to keep all variable declarations together. > + 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_dl.h > b/drivers/media/platform/vsp1/vsp1_dl.h index 7e820ac6865a..f45083251644 > 100644 > --- a/drivers/media/platform/vsp1/vsp1_dl.h > +++ b/drivers/media/platform/vsp1/vsp1_dl.h > @@ -41,7 +41,6 @@ vsp1_dl_body_pool_create(struct vsp1_device *vsp1, > unsigned int num_bodies, void vsp1_dl_body_pool_destroy(struct > vsp1_dl_body_pool *pool); > struct vsp1_dl_body *vsp1_dl_body_get(struct vsp1_dl_body_pool *pool); > void vsp1_dl_body_put(struct vsp1_dl_body *dlb); > - This is an unrelated change. > void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32 reg, u32 data); > int vsp1_dl_list_add_body(struct vsp1_dl_list *dl, struct vsp1_dl_body > *dlb); > int vsp1_dl_list_add_chain(struct vsp1_dl_list *head, struct vsp1_dl_list > *dl); [snip] > diff --git a/drivers/media/platform/vsp1/vsp1_entity.h > b/drivers/media/platform/vsp1/vsp1_entity.h index > 408602ebeb97..b44ed5414fc3 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, ...) > + * @configure_stream:Setup the initial hardware parameters for the stream > + * (pipeline, formats) Instead of initial I would say "Setup hardware parameters that stay constant for the whole stream (pipeline, formats)", or possible "that don't vary between frames" instead. > + * @configure_frame: Configure the runtime parameters for each partition > + *
[PATCH v7 6/8] media: vsp1: Refactor display list configure operations
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, '.configure_stream()' and '.configure_frame()', merging both the VSP1_ENTITY_PARAMS_RUNTIME and VSP1_ENTITY_PARAMS_PARTITION stages into a single call through the .configure_frame(). 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 --- v7 - Fix formatting and white space - s/prepare/configure_stream/ - s/configure/configure_frame/ drivers/media/platform/vsp1/vsp1_bru.c| 12 +- drivers/media/platform/vsp1/vsp1_clu.c| 50 +--- drivers/media/platform/vsp1/vsp1_dl.h | 1 +- drivers/media/platform/vsp1/vsp1_drm.c| 21 +-- drivers/media/platform/vsp1/vsp1_entity.c | 17 +- drivers/media/platform/vsp1/vsp1_entity.h | 33 +-- 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| 32 +- drivers/media/platform/vsp1/vsp1_rpf.c| 164 ++--- drivers/media/platform/vsp1/vsp1_sru.c| 12 +- drivers/media/platform/vsp1/vsp1_uds.c| 57 ++-- drivers/media/platform/vsp1/vsp1_video.c | 24 +-- drivers/media/platform/vsp1/vsp1_wpf.c| 299 --- 16 files changed, 378 insertions(+), 392 deletions(-) diff --git a/drivers/media/platform/vsp1/vsp1_bru.c b/drivers/media/platform/vsp1/vsp1_bru.c index e8fd2ae3b3eb..d6fd265eaccb 100644 --- a/drivers/media/platform/vsp1/vsp1_bru.c +++ b/drivers/media/platform/vsp1/vsp1_bru.c @@ -285,19 +285,15 @@ static const struct v4l2_subdev_ops bru_ops = { * VSP1 Entity Operations */ -static void bru_configure(struct vsp1_entity *entity, - struct vsp1_pipeline *pipe, - struct vsp1_dl_list *dl, - enum vsp1_entity_params params) +static void bru_configure_stream(struct vsp1_entity *entity, +struct vsp1_pipeline *pipe, +struct vsp1_dl_list *dl) { struct vsp1_bru *bru = to_bru(&entity->subdev); struct v4l2_mbus_framefmt *format; unsigned int flags; unsigned int i; - if (params != VSP1_ENTITY_PARAMS_INIT) - return; - format = vsp1_entity_get_pad_format(&bru->entity, bru->entity.config, bru->entity.source_pad); @@ -404,7 +400,7 @@ static void bru_configure(struct vsp1_entity *entity, } static const struct vsp1_entity_operations bru_entity_ops = { - .configure = bru_configure, + .configure_stream = bru_configure_stream, }; /* - diff --git a/drivers/media/platform/vsp1/vsp1_clu.c b/drivers/media/platform/vsp1/vsp1_clu.c index b2a39a6ef7e4..b8d8af6d4910 100644 --- a/drivers/media/platform/vsp1/vsp1_clu.c +++ b/drivers/media/platform/vsp1/vsp1_clu.c @@ -213,37 +213,36 @@ static const struct v4l2_subdev_ops clu_ops = { /* - * VSP1 Entity Operations */ +static void clu_configure_stream(struct vsp1_entity *entity, +struct vsp1_pipeline *pipe, +struct vsp1_dl_list *dl) +{ + struct vsp1_clu *clu = to_clu(&entity->subdev); + + /* +* The yuv_mode can't be changed during streaming. Cache it internally +* for future runtime configuration calls. +*/ + 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; +} -static void clu_configure(struct vsp1_entity *entity, - struct vsp1_pipeline *pipe, - struct vsp1_dl_list *dl, - enum vsp1_entity_params params) +static void clu_configure_frame(struct vsp1_entity *entity, + struct vsp1_pipeline *pipe, + struct vsp1_dl_list *dl, + unsigned int partition) { struct vsp1_clu *clu = to_clu(&entity->subdev); struct vsp1_dl_body *dlb; unsigned long flags; u32 ctrl = VI6_CLU_CTRL_AAI | VI6_CLU_CTRL_MVS | VI6_CLU_CTRL_EN; - swi