Hi Laurent,

On 26/06/17 19:12, Laurent Pinchart wrote:

When the display start interrupt occurs, we know that the hardware has finished loading the active display list. The driver then proceeds to recycle the list, assuming it won't be needed anymore.

This assumption holds true for headerless display lists, as the VSP doesn't reload the list for the next frame if it hasn't changed. However, this isn't true anymore for header display lists, as they are loaded at every frame start regardless of whether they have been updated.

To prepare for header display lists usage in display pipelines, we need to postpone recycling the list until it gets replaced by a new one through a page flip. The driver already does so in the frame end interrupt handler, so all we need is to skip list recycling in the display start interrupt handler.

While the active list can be recycled at display start for headerless display lists, there's no real harm in postponing that to the frame end interrupt handler in all cases. This simplifies interrupt handling as we don't need to process the display start interrupt anymore.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Ok, I had skipped this one as I was concerned about its effects in relation to 11/14 but I see how that's working now.

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

drivers/media/platform/vsp1/vsp1_dl.c | 16 ---------------- drivers/media/platform/vsp1/vsp1_dl.h | 1 - drivers/media/platform/vsp1/vsp1_drm.c | 12 ++++-------- drivers/media/platform/vsp1/vsp1_drm.h | 2 -- drivers/media/platform/vsp1/vsp1_drv.c | 8 -------- 5 files changed, 4 insertions(+), 35 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c index dc47e236c780..bb92be4fe0f0 100644 --- a/drivers/media/platform/vsp1/vsp1_dl.c +++ b/drivers/media/platform/vsp1/vsp1_dl.c @@ -547,22 +547,6 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)

• Display List Manager

*/

-/* Interrupt Handling */ -void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm) -{

• spin_lock(&dlm->lock);
• /*

• * The display start interrupt signals the end of the display list
  

• * processing by the device. The active display list, if any, won't be
  

• * accessed anymore and can be reused.
  

• */
  

• __vsp1_dl_list_put(dlm->active);
• dlm->active = NULL;
• spin_unlock(&dlm->lock);

-}

/**

• vsp1_dlm_irq_frame_end - Display list handler for the frame end interrupt
• @dlm: the display list manager

diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h index 6ec1380a10af..ee3508172f0a 100644 --- a/drivers/media/platform/vsp1/vsp1_dl.h +++ b/drivers/media/platform/vsp1/vsp1_dl.h @@ -27,7 +27,6 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1, unsigned int prealloc); void vsp1_dlm_destroy(struct vsp1_dl_manager *dlm); void vsp1_dlm_reset(struct vsp1_dl_manager *dlm); -void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm); bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);

struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm); diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index 9377aafa8996..bc3fd9bc7126 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -32,11 +32,6 @@

• Interrupt Handling

*/

-void vsp1_drm_display_start(struct vsp1_device *vsp1) -{

• vsp1_dlm_irq_display_start(vsp1->drm->pipe.output->dlm);

-}

static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe) { struct vsp1_drm *drm = to_vsp1_drm(pipe); @@ -224,6 +219,10 @@ int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config *cfg) return ret; }

• /* Disable the display interrupts. */

• vsp1_write(vsp1, VI6_DISP_IRQ_STA, 0);

• vsp1_write(vsp1, VI6_DISP_IRQ_ENB, 0);

• dev_dbg(vsp1->dev, "%s: pipeline enabled\n", __func__);

  return 0;

@@ -529,13 +528,10 @@ void vsp1_du_atomic_flush(struct device *dev)

/* Start or stop the pipeline if needed. */ if (!vsp1->drm->num_inputs && pipe->num_inputs) {

• vsp1_write(vsp1, VI6_DISP_IRQ_STA, 0);
  

• vsp1_write(vsp1, VI6_DISP_IRQ_ENB, VI6_DISP_IRQ_ENB_DSTE);
  

  spin_lock_irqsave(&pipe->irqlock, flags); vsp1_pipeline_run(pipe); spin_unlock_irqrestore(&pipe->irqlock, flags); } else if (vsp1->drm->num_inputs && !pipe->num_inputs) {

• vsp1_write(vsp1, VI6_DISP_IRQ_ENB, 0);
  

  vsp1_pipeline_stop(pipe); }

} diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h index e9f80727ff92..cbdbb8a39883 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.h +++ b/drivers/media/platform/vsp1/vsp1_drm.h @@ -50,6 +50,4 @@ 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);

-void vsp1_drm_display_start(struct vsp1_device *vsp1);

#endif /* __VSP1_DRM_H__ */ diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c index 95c26edead85..6b35e043b554 100644 --- a/drivers/media/platform/vsp1/vsp1_drv.c +++ b/drivers/media/platform/vsp1/vsp1_drv.c @@ -68,14 +68,6 @@ static irqreturn_t vsp1_irq_handler(int irq, void *data) } }

• status = vsp1_read(vsp1, VI6_DISP_IRQ_STA);
• vsp1_write(vsp1, VI6_DISP_IRQ_STA, ~status & VI6_DISP_IRQ_STA_DST);
• if (status & VI6_DISP_IRQ_STA_DST) {

• vsp1_drm_display_start(vsp1);
  

• ret = IRQ_HANDLED;
  

• }
• return ret;

}

Hi Laurent,

This looks like a good simplification/removal of obfuscation to me!

On 26/06/17 19:12, Laurent Pinchart wrote:

The internal VSP entity source and sink pointers are stored as media_entity pointers, which are then cast to a vsp1_entity. As all sources and sinks are vsp1_entity instances, we can store the vsp1_entity pointers directly.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

drivers/media/platform/vsp1/vsp1_drm.c | 4 ++-- drivers/media/platform/vsp1/vsp1_drv.c | 2 +- drivers/media/platform/vsp1/vsp1_entity.c | 26 +++++++++++++------------- drivers/media/platform/vsp1/vsp1_entity.h | 4 ++-- 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index bc3fd9bc7126..2d5a74e95e09 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -587,7 +587,7 @@ int vsp1_drm_create_links(struct vsp1_device *vsp1) if (ret < 0) return ret;

• rpf->entity.sink = &vsp1->bru->entity.subdev.entity;
  

• rpf->entity.sink = &vsp1->bru->entity;
  

  rpf->entity.sink_pad = i; }

@@ -598,7 +598,7 @@ int vsp1_drm_create_links(struct vsp1_device *vsp1) if (ret < 0) return ret;

• vsp1->bru->entity.sink = &vsp1->wpf[0]->entity.subdev.entity;

• vsp1->bru->entity.sink = &vsp1->wpf[0]->entity; vsp1->bru->entity.sink_pad = RWPF_PAD_SINK;

  ret = media_create_pad_link(&vsp1->wpf[0]->entity.subdev.entity,

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c index 35087d5573ce..9b3a0790f92a 100644 --- a/drivers/media/platform/vsp1/vsp1_drv.c +++ b/drivers/media/platform/vsp1/vsp1_drv.c @@ -121,7 +121,7 @@ static int vsp1_create_sink_links(struct vsp1_device *vsp1, return ret;

	if (flags & MEDIA_LNK_FL_ENABLED)

• 		source->sink = entity;
  

• 		source->sink = sink;
  

  } }

diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c index 4bdb3b141611..71dd903263ad 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.c +++ b/drivers/media/platform/vsp1/vsp1_entity.c @@ -24,18 +24,11 @@ #include "vsp1_pipe.h" #include "vsp1_rwpf.h"

-static inline struct vsp1_entity * -media_entity_to_vsp1_entity(struct media_entity *entity) -{

• return container_of(entity, struct vsp1_entity, subdev.entity);

-}

void vsp1_entity_route_setup(struct vsp1_entity *entity, struct vsp1_pipeline *pipe, struct vsp1_dl_list *dl) { struct vsp1_entity *source;

• struct vsp1_entity *sink;

  if (entity->type == VSP1_ENTITY_HGO) { u32 smppt;

@@ -44,7 +37,7 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity, * The HGO is a special case, its routing is configured on the * sink pad. */

• source = media_entity_to_vsp1_entity(entity->sources[0]);
  

• source = entity->sources[0];
  

  smppt = (pipe->output->entity.index << VI6_DPR_SMPPT_TGW_SHIFT) | (source->route->output << VI6_DPR_SMPPT_PT_SHIFT);

@@ -57,7 +50,7 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity, * The HGT is a special case, its routing is configured on the * sink pad. */

• source = media_entity_to_vsp1_entity(entity->sources[0]);
  

• source = entity->sources[0];
  

  smppt = (pipe->output->entity.index << VI6_DPR_SMPPT_TGW_SHIFT) | (source->route->output << VI6_DPR_SMPPT_PT_SHIFT);

@@ -69,9 +62,8 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity, if (source->route->reg == 0) return;

• sink = media_entity_to_vsp1_entity(source->sink); vsp1_dl_list_write(dl, source->route->reg,

• 	   sink->route->inputs[source->sink_pad]);
  

• 	   source->sink->route->inputs[source->sink_pad]);
  

}

/* ----------------------------------------------------------------------------- @@ -316,6 +308,12 @@ int vsp1_subdev_enum_frame_size(struct v4l2_subdev *subdev,

• Media Operations

*/

+static inline struct vsp1_entity * +media_entity_to_vsp1_entity(struct media_entity *entity) +{

• return container_of(entity, struct vsp1_entity, subdev.entity);

+}

static int vsp1_entity_link_setup_source(const struct media_pad *source_pad, const struct media_pad *sink_pad, u32 flags) @@ -339,7 +337,7 @@ static int vsp1_entity_link_setup_source(const struct media_pad *source_pad, sink->type != VSP1_ENTITY_HGT) { if (source->sink) return -EBUSY;

• 	source->sink = sink_pad->entity;
  

• 	source->sink = sink;
  source->sink_pad = sink_pad->index;
  

  } } else {

@@ -355,15 +353,17 @@ static int vsp1_entity_link_setup_sink(const struct media_pad *source_pad, u32 flags) { struct vsp1_entity *sink;

• struct vsp1_entity *source;

  sink = media_entity_to_vsp1_entity(sink_pad->entity);

• source = media_entity_to_vsp1_entity(source_pad->entity);

  if (flags & MEDIA_LNK_FL_ENABLED) { /* Fan-in is limited to one. */ if (sink->sources[sink_pad->index]) return -EBUSY;

• sink->sources[sink_pad->index] = source_pad->entity;
  

• sink->sources[sink_pad->index] = source;
  

  } else { sink->sources[sink_pad->index] = NULL; }

diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h index c169a060b6d2..4362cd4e90ba 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.h +++ b/drivers/media/platform/vsp1/vsp1_entity.h @@ -104,8 +104,8 @@ struct vsp1_entity { struct media_pad *pads; unsigned int source_pad;

• struct media_entity **sources;
• struct media_entity *sink;

• struct vsp1_entity **sources;

• struct vsp1_entity *sink; unsigned int sink_pad;

  struct v4l2_subdev subdev;

On 26/06/17 19:12, Laurent Pinchart wrote:

In the H3 ES2.0 SoC the VSP2-DL instance has two connections to DU channels that need to be configured independently. Extend the VSP-DU API with a pipeline index to identify which pipeline the caller wants to operate on.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

A bit of comment merge between this and the next patch but it's minor and not worth the effort to change that ... so I'll happily ignore it if you do :)

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 12 ++++++------ drivers/media/platform/vsp1/vsp1_drm.c | 32 ++++++++++++++++++++++---------- include/media/vsp1.h | 10 ++++++---- 3 files changed, 34 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index f870445ebc8d..d46dce054442 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -81,22 +81,22 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) */ crtc->group->need_restart = true;

• vsp1_du_setup_lif(crtc->vsp->vsp, &cfg);

• vsp1_du_setup_lif(crtc->vsp->vsp, 0, &cfg);

}

void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) {

• vsp1_du_setup_lif(crtc->vsp->vsp, NULL);

• vsp1_du_setup_lif(crtc->vsp->vsp, 0, NULL);

}

void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) {

• vsp1_du_atomic_begin(crtc->vsp->vsp);

• vsp1_du_atomic_begin(crtc->vsp->vsp, 0);

}

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, 0);

}

/* Keep the two tables in sync. */ @@ -192,7 +192,7 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane) } }

• vsp1_du_atomic_update(plane->vsp->vsp, plane->index, &cfg);

• vsp1_du_atomic_update(plane->vsp->vsp, 0, plane->index, &cfg);

}

static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane, @@ -292,7 +292,7 @@ static void rcar_du_vsp_plane_atomic_update(struct drm_plane *plane, if (plane->state->crtc) rcar_du_vsp_plane_setup(rplane); else

• vsp1_du_atomic_update(rplane->vsp->vsp, rplane->index, NULL);
  

• vsp1_du_atomic_update(rplane->vsp->vsp, 0, rplane->index, NULL);
  

}

static const struct drm_plane_helper_funcs rcar_du_vsp_plane_helper_funcs = { diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index c72d021ff820..daaafe7885fa 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -58,21 +58,26 @@ EXPORT_SYMBOL_GPL(vsp1_du_init); /**

• vsp1_du_setup_lif - Setup the output part of the VSP pipeline
• @dev: the VSP device

• • @pipe_index: the DRM pipeline index
  • @cfg: the LIF configuration
  • Configure the output part of VSP DRM pipeline for the given frame @cfg.width

• • and @cfg.height. This sets up formats on the BRU source pad, the WPF0 sink
• • and source pads, and the LIF sink pad.

• • and @cfg.height. This sets up formats on the blend unit (BRU or BRS) source
• • pad, the WPF sink and source pads, and the LIF sink pad.

• • As the media bus code on the BRU source pad is conditioned by the
• • configuration of the BRU sink 0 pad, we also set up the formats on all BRU

• • The @pipe_index argument selects which DRM pipeline to setup. The number of
• • available pipelines depend on the VSP instance.
• • As the media bus code on the blend unit source pad is conditioned by the
• • configuration of its sink 0 pad, we also set up the formats on all blend unit
  • sinks, even if the configuration will be overwritten later by

• • vsp1_du_setup_rpf(). This ensures that the BRU configuration is set to a well
• • defined state.

• • vsp1_du_setup_rpf(). This ensures that the blend unit configuration is set to
• • a well defined state.

I presume those comment updates for the BRU/ blend-unit configuration are actually for the next patch - but I don't think it matters here - and isn't worth the effort to move the hunks.

It all reads OK.

• Return 0 on success or a negative error code on failure.

*/ -int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config *cfg) +int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,

•       const struct vsp1_du_lif_config *cfg)
  

{ struct vsp1_device *vsp1 = dev_get_drvdata(dev); struct vsp1_pipeline *pipe = &vsp1->drm->pipe; @@ -81,6 +86,9 @@ int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config *cfg) unsigned int i; int ret;

• if (pipe_index > 0)

• return -EINVAL;
  

• if (!cfg) { /*
  • NULL configuration means the CRTC is being disabled, stop

@@ -232,8 +240,9 @@ EXPORT_SYMBOL_GPL(vsp1_du_setup_lif); /**

• vsp1_du_atomic_begin - Prepare for an atomic update
• @dev: the VSP device

• • @pipe_index: the DRM pipeline index
  */

-void vsp1_du_atomic_begin(struct device *dev) +void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index) { struct vsp1_device *vsp1 = dev_get_drvdata(dev); struct vsp1_pipeline *pipe = &vsp1->drm->pipe; @@ -245,6 +254,7 @@ EXPORT_SYMBOL_GPL(vsp1_du_atomic_begin); /**

• vsp1_du_atomic_update - Setup one RPF input of the VSP pipeline
• @dev: the VSP device

• • @pipe_index: the DRM pipeline index
  • @rpf_index: index of the RPF to setup (0-based)
  • @cfg: the RPF configuration

@@ -271,7 +281,8 @@ EXPORT_SYMBOL_GPL(vsp1_du_atomic_begin);

• Return 0 on success or a negative error code on failure.

*/ -int vsp1_du_atomic_update(struct device *dev, unsigned int rpf_index, +int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,

• 	  unsigned int rpf_index,
    const struct vsp1_du_atomic_config *cfg)
  

{ struct vsp1_device *vsp1 = dev_get_drvdata(dev); @@ -437,8 +448,9 @@ 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

• • @pipe_index: the DRM pipeline index
  */

-void vsp1_du_atomic_flush(struct device *dev) +void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index) { struct vsp1_device *vsp1 = dev_get_drvdata(dev); struct vsp1_pipeline *pipe = &vsp1->drm->pipe; diff --git a/include/media/vsp1.h b/include/media/vsp1.h index c837383b2013..c8fc868fb0f2 100644 --- a/include/media/vsp1.h +++ b/include/media/vsp1.h @@ -38,7 +38,8 @@ struct vsp1_du_lif_config { void *callback_data; };

-int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config *cfg); +int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,

•       const struct vsp1_du_lif_config *cfg);
  
  

struct vsp1_du_atomic_config { u32 pixelformat; @@ -50,10 +51,11 @@ struct vsp1_du_atomic_config { unsigned int zpos; };

-void vsp1_du_atomic_begin(struct device *dev); -int vsp1_du_atomic_update(struct device *dev, unsigned int rpf, +void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index); +int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,

• 	  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, unsigned int pipe_index); int vsp1_du_map_sg(struct device *dev, struct sg_table *sgt); void vsp1_du_unmap_sg(struct device *dev, struct sg_table *sgt);

Em Fri, 14 Jul 2017 02:04:06 +0300 Laurent Pinchart laurent.pinchart@ideasonboard.com escreveu:

On Thursday 13 Jul 2017 14:16:03 Kieran Bingham wrote:

...

On 26/06/17 19:12, Laurent Pinchart wrote:

...

In the H3 ES2.0 SoC the VSP2-DL instance has two connections to DU channels that need to be configured independently. Extend the VSP-DU API with a pipeline index to identify which pipeline the caller wants to operate on.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

A bit of comment merge between this and the next patch but it's minor and not worth the effort to change that ... so I'll happily ignore it if you do :)

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

...

---

drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 12 ++++++------ drivers/media/platform/vsp1/vsp1_drm.c | 32 +++++++++++++++++++-------- include/media/vsp1.h | 10 ++++++---- 3 files changed, 34 insertions(+), 20 deletions(-)

[snip]

...

...

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index c72d021ff820..daaafe7885fa 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -58,21 +58,26 @@ EXPORT_SYMBOL_GPL(vsp1_du_init); /**

• vsp1_du_setup_lif - Setup the output part of the VSP pipeline
• @dev: the VSP device

• • @pipe_index: the DRM pipeline index
  • @cfg: the LIF configuration
  • Configure the output part of VSP DRM pipeline for the given frame
  @cfg.width

• • and @cfg.height. This sets up formats on the BRU source pad, the WPF0

sink

• • and source pads, and the LIF sink pad.

• • and @cfg.height. This sets up formats on the blend unit (BRU or BRS)

source

• • pad, the WPF sink and source pads, and the LIF sink pad.

• • As the media bus code on the BRU source pad is conditioned by the
• • configuration of the BRU sink 0 pad, we also set up the formats on all

BRU

• • The @pipe_index argument selects which DRM pipeline to setup. The

number of

• • available pipelines depend on the VSP instance.
• • As the media bus code on the blend unit source pad is conditioned by

the

• • configuration of its sink 0 pad, we also set up the formats on all

blend unit

• sinks, even if the configuration will be overwritten later by

• • vsp1_du_setup_rpf(). This ensures that the BRU configuration is set to

a well

• • defined state.

• • vsp1_du_setup_rpf(). This ensures that the blend unit configuration is

set to

• • a well defined state.

I presume those comment updates for the BRU/ blend-unit configuration are actually for the next patch - but I don't think it matters here - and isn't worth the effort to move the hunks.

Too late, I've fixed it already :-) Thanks for pointing it out.

Acked-by: Mauro Carvalho Chehab mchehab@s-opensource.com

Thanks, Mauro

On 26/06/17 19:12, Laurent Pinchart wrote:

The Blend/ROP Sub Unit (BRS) is a stripped-down version of the BRU found in several VSP2 instances. Compared to a regular BRU, it supports two inputs only, and thus has no ROP unit.

Add support for the BRS by modeling it as a new entity type, but reuse

s/modeling/modelling/

the vsp1_bru object underneath. Chaining the BRU and BRS entities seems to be supported by the hardware but isn't implemented yet as it isn't the primary use case for the BRS.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

drivers/media/platform/vsp1/vsp1.h | 2 + drivers/media/platform/vsp1/vsp1_bru.c | 45 ++++++++++++++-------- drivers/media/platform/vsp1/vsp1_bru.h | 4 +- drivers/media/platform/vsp1/vsp1_drv.c | 19 +++++++++- drivers/media/platform/vsp1/vsp1_entity.c | 13 ++++++- drivers/media/platform/vsp1/vsp1_entity.h | 1 + drivers/media/platform/vsp1/vsp1_pipe.c | 7 ++-- drivers/media/platform/vsp1/vsp1_regs.h | 26 +++++++++---- drivers/media/platform/vsp1/vsp1_video.c | 63 ++++++++++++++++++++----------- drivers/media/platform/vsp1/vsp1_wpf.c | 4 +- 10 files changed, 130 insertions(+), 54 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1.h b/drivers/media/platform/vsp1/vsp1.h index 847963b6e9eb..73858a0ed35c 100644 --- a/drivers/media/platform/vsp1/vsp1.h +++ b/drivers/media/platform/vsp1/vsp1.h @@ -54,6 +54,7 @@ struct vsp1_uds; #define VSP1_HAS_WPF_HFLIP (1 << 6) #define VSP1_HAS_HGO (1 << 7) #define VSP1_HAS_HGT (1 << 8) +#define VSP1_HAS_BRS (1 << 9)

struct vsp1_device_info { u32 version; @@ -76,6 +77,7 @@ struct vsp1_device { struct rcar_fcp_device *fcp; struct device *bus_master;

• struct vsp1_bru *brs; struct vsp1_bru *bru; struct vsp1_clu *clu; struct vsp1_hgo *hgo;

diff --git a/drivers/media/platform/vsp1/vsp1_bru.c b/drivers/media/platform/vsp1/vsp1_bru.c index 85362c5ef57a..e8fd2ae3b3eb 100644 --- a/drivers/media/platform/vsp1/vsp1_bru.c +++ b/drivers/media/platform/vsp1/vsp1_bru.c @@ -33,7 +33,7 @@ static inline void vsp1_bru_write(struct vsp1_bru *bru, struct vsp1_dl_list *dl, u32 reg, u32 data) {

• vsp1_dl_list_write(dl, reg, data);

• vsp1_dl_list_write(dl, bru->base + reg, data);

}

/* ----------------------------------------------------------------------------- @@ -332,11 +332,14 @@ static void bru_configure(struct vsp1_entity *entity, /* * Route BRU input 1 as SRC input to the ROP unit and configure the ROP * unit with a NOP operation to make BRU input 1 available as the

• * Blend/ROP unit B SRC input.
  

• * Blend/ROP unit B SRC input. Only needed for BRU, the BRS has no ROP
  

• * unit.
  

  */

• vsp1_bru_write(bru, dl, VI6_BRU_ROP, VI6_BRU_ROP_DSTSEL_BRUIN(1) |

•        VI6_BRU_ROP_CROP(VI6_ROP_NOP) |
  

•        VI6_BRU_ROP_AROP(VI6_ROP_NOP));
  

• if (entity->type == VSP1_ENTITY_BRU)

• vsp1_bru_write(bru, dl, VI6_BRU_ROP,
  

• 	       VI6_BRU_ROP_DSTSEL_BRUIN(1) |
  

• 	       VI6_BRU_ROP_CROP(VI6_ROP_NOP) |
  

• 	       VI6_BRU_ROP_AROP(VI6_ROP_NOP));
  
  

  for (i = 0; i < bru->entity.source_pad; ++i) { bool premultiplied = false;

@@ -366,12 +369,13 @@ static void bru_configure(struct vsp1_entity *entity, ctrl |= VI6_BRU_CTRL_DSTSEL_VRPF;

/*

•  * Route BRU inputs 0 to 3 as SRC inputs to Blend/ROP units A to
  

•  * D in that order. The Blend/ROP unit B SRC is hardwired to the
  

•  * ROP unit output, the corresponding register bits must be set
  

•  * to 0.
  

•  * Route inputs 0 to 3 as SRC inputs to Blend/ROP units A to D
  

•  * in that order. In the BRU the Blend/ROP unit B SRC is
  

•  * hardwired to the ROP unit output, the corresponding register
  

•  * bits must be set to 0. The BRS has no ROP unit and doesn't
  

•  * need any special processing.
  

  */

• if (i != 1)
  

• if (!(entity->type == VSP1_ENTITY_BRU && i == 1))
  

If we're using this module for both BRU and BRS, would an is_bru(entity) and is_brs(entity) be cleaner here ?

Not required - just thinking outloud...

Actaully - it's only this line that would be affected so not really needed. I thought there would be more uses/differences.

	ctrl |= VI6_BRU_CTRL_SRCSEL_BRUIN(i);

vsp1_bru_write(bru, dl, VI6_BRU_CTRL(i), ctrl);

@@ -407,20 +411,31 @@ static const struct vsp1_entity_operations bru_entity_ops = {

• Initialization and Cleanup

*/

-struct vsp1_bru *vsp1_bru_create(struct vsp1_device *vsp1) +struct vsp1_bru *vsp1_bru_create(struct vsp1_device *vsp1,

• 		 enum vsp1_entity_type type)
  

{ struct vsp1_bru *bru;

• unsigned int num_pads;

• const char *name; int ret;

  bru = devm_kzalloc(vsp1->dev, sizeof(*bru), GFP_KERNEL); if (bru == NULL) return ERR_PTR(-ENOMEM);

• bru->base = type == VSP1_ENTITY_BRU ? VI6_BRU_BASE : VI6_BRS_BASE; bru->entity.ops = &bru_entity_ops;

• bru->entity.type = VSP1_ENTITY_BRU;

• bru->entity.type = type;
• if (type == VSP1_ENTITY_BRU) {

• num_pads = vsp1->info->num_bru_inputs + 1;
  

• name = "bru";
  

• } else {

• num_pads = 3;
  

• name = "brs";
  

• }

• ret = vsp1_entity_init(vsp1, &bru->entity, "bru",

• 	       vsp1->info->num_bru_inputs + 1, &bru_ops,
  

• ret = vsp1_entity_init(vsp1, &bru->entity, name, num_pads, &bru_ops, MEDIA_ENT_F_PROC_VIDEO_COMPOSER); if (ret < 0) return ERR_PTR(ret);

@@ -435,7 +450,7 @@ struct vsp1_bru *vsp1_bru_create(struct vsp1_device *vsp1) bru->entity.subdev.ctrl_handler = &bru->ctrls;

if (bru->ctrls.error) {

• dev_err(vsp1->dev, "bru: failed to initialize controls\n");
  

• dev_err(vsp1->dev, "%s: failed to initialize controls\n", name);
  

  ret = bru->ctrls.error; vsp1_entity_destroy(&bru->entity); return ERR_PTR(ret);

diff --git a/drivers/media/platform/vsp1/vsp1_bru.h b/drivers/media/platform/vsp1/vsp1_bru.h index 828a3fcadea8..c98ed96d8de6 100644 --- a/drivers/media/platform/vsp1/vsp1_bru.h +++ b/drivers/media/platform/vsp1/vsp1_bru.h @@ -26,6 +26,7 @@ struct vsp1_rwpf;

struct vsp1_bru { struct vsp1_entity entity;

• unsigned int base;

  struct v4l2_ctrl_handler ctrls;

@@ -41,6 +42,7 @@ static inline struct vsp1_bru *to_bru(struct v4l2_subdev *subdev) return container_of(subdev, struct vsp1_bru, entity.subdev); }

-struct vsp1_bru *vsp1_bru_create(struct vsp1_device *vsp1); +struct vsp1_bru *vsp1_bru_create(struct vsp1_device *vsp1,

• 		 enum vsp1_entity_type type);
  
  

#endif /* __VSP1_BRU_H__ */ diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c index 5a467b118a1c..6a9aeb71aedf 100644 --- a/drivers/media/platform/vsp1/vsp1_drv.c +++ b/drivers/media/platform/vsp1/vsp1_drv.c @@ -84,6 +84,10 @@ static irqreturn_t vsp1_irq_handler(int irq, void *data)

• • from a UDS to a UDS (UDS entities can't be chained)
• • from an entity to itself (no loops are allowed)

• • Furthermore, the BRS can't be connected to histogram generators, but no
• • special check is currently needed as all VSP instances that include a BRS
• • have no histogram generator.
  */

static int vsp1_create_sink_links(struct vsp1_device *vsp1, struct vsp1_entity *sink) @@ -261,8 +265,18 @@ static int vsp1_create_entities(struct vsp1_device *vsp1) }

/* Instantiate all the entities. */

• if (vsp1->info->features & VSP1_HAS_BRS) {

• vsp1->brs = vsp1_bru_create(vsp1, VSP1_ENTITY_BRS);
  

• if (IS_ERR(vsp1->brs)) {
  

• 	ret = PTR_ERR(vsp1->brs);
  

• 	goto done;
  

• }
  

• list_add_tail(&vsp1->brs->entity.list_dev, &vsp1->entities);
  

• }
• if (vsp1->info->features & VSP1_HAS_BRU) {

• vsp1->bru = vsp1_bru_create(vsp1);
  

• vsp1->bru = vsp1_bru_create(vsp1, VSP1_ENTITY_BRU);
  

  if (IS_ERR(vsp1->bru)) { ret = PTR_ERR(vsp1->bru); goto done;

@@ -502,6 +516,9 @@ static int vsp1_device_init(struct vsp1_device *vsp1) vsp1_write(vsp1, VI6_DPR_HSI_ROUTE, VI6_DPR_NODE_UNUSED); vsp1_write(vsp1, VI6_DPR_BRU_ROUTE, VI6_DPR_NODE_UNUSED);

• if (vsp1->info->features & VSP1_HAS_BRS)

• vsp1_write(vsp1, VI6_DPR_ILV_BRS_ROUTE, VI6_DPR_NODE_UNUSED);
  

• vsp1_write(vsp1, VI6_DPR_HGO_SMPPT, (7 << VI6_DPR_SMPPT_TGW_SHIFT) | (VI6_DPR_NODE_UNUSED << VI6_DPR_SMPPT_PT_SHIFT)); vsp1_write(vsp1, VI6_DPR_HGT_SMPPT, (7 << VI6_DPR_SMPPT_TGW_SHIFT) |

diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c index 71dd903263ad..c06f7db093db 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.c +++ b/drivers/media/platform/vsp1/vsp1_entity.c @@ -29,6 +29,7 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity, struct vsp1_dl_list *dl) { struct vsp1_entity *source;

• u32 route;

  if (entity->type == VSP1_ENTITY_HGO) { u32 smppt;

@@ -62,8 +63,14 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity, if (source->route->reg == 0) return;

• vsp1_dl_list_write(dl, source->route->reg,

• 	   source->sink->route->inputs[source->sink_pad]);
  

• route = source->sink->route->inputs[source->sink_pad];
• /*

• * The ILV and BRS share the same data path route. The extra BRSSEL bit
  

• * selects between the ILV and BRS.
  

• */
  

• if (source->type == VSP1_ENTITY_BRS)

• route |= VI6_DPR_ROUTE_BRSSEL;
  

• vsp1_dl_list_write(dl, source->route->reg, route);

}

/* ----------------------------------------------------------------------------- @@ -450,6 +457,8 @@ struct media_pad *vsp1_entity_remote_pad(struct media_pad *pad) { VI6_DPR_NODE_WPF(idx) }, VI6_DPR_NODE_WPF(idx) }

static const struct vsp1_route vsp1_routes[] = {

• { VSP1_ENTITY_BRS, 0, VI6_DPR_ILV_BRS_ROUTE,

•  { VI6_DPR_NODE_BRS_IN(0), VI6_DPR_NODE_BRS_IN(1) }, 0 },
  

  { VSP1_ENTITY_BRU, 0, VI6_DPR_BRU_ROUTE, { VI6_DPR_NODE_BRU_IN(0), VI6_DPR_NODE_BRU_IN(1), VI6_DPR_NODE_BRU_IN(2), VI6_DPR_NODE_BRU_IN(3),

diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h index 4362cd4e90ba..11f8363fa6b0 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.h +++ b/drivers/media/platform/vsp1/vsp1_entity.h @@ -23,6 +23,7 @@ struct vsp1_dl_list; struct vsp1_pipeline;

enum vsp1_entity_type {

• VSP1_ENTITY_BRS, VSP1_ENTITY_BRU, VSP1_ENTITY_CLU, VSP1_ENTITY_HGO,

diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c index e817623b84e0..9bb961298af2 100644 --- a/drivers/media/platform/vsp1/vsp1_pipe.c +++ b/drivers/media/platform/vsp1/vsp1_pipe.c @@ -373,10 +373,11 @@ void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline *pipe, return;

/*

• * The BRU background color has a fixed alpha value set to 255, the
  

• * output alpha value is thus always equal to 255.
  

• * The BRU and BRS background color has a fixed alpha value set to 255,
  

• * the output alpha value is thus always equal to 255.
  

  */

• if (pipe->uds_input->type == VSP1_ENTITY_BRU)

• if (pipe->uds_input->type == VSP1_ENTITY_BRU ||

•    pipe->uds_input->type == VSP1_ENTITY_BRS)
  

  alpha = 255;

  vsp1_uds_set_alpha(pipe->uds, dl, alpha);

diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h index cd3e32af6e3b..744217e020b9 100644 --- a/drivers/media/platform/vsp1/vsp1_regs.h +++ b/drivers/media/platform/vsp1/vsp1_regs.h @@ -238,6 +238,10 @@ #define VI6_WPF_SRCRPF_VIRACT_SUB (1 << 28) #define VI6_WPF_SRCRPF_VIRACT_MST (2 << 28) #define VI6_WPF_SRCRPF_VIRACT_MASK (3 << 28) +#define VI6_WPF_SRCRPF_VIRACT2_DIS (0 << 24) +#define VI6_WPF_SRCRPF_VIRACT2_SUB (1 << 24) +#define VI6_WPF_SRCRPF_VIRACT2_MST (2 << 24) +#define VI6_WPF_SRCRPF_VIRACT2_MASK (3 << 24) #define VI6_WPF_SRCRPF_RPF_ACT_DIS(n) (0 << ((n) * 2)) #define VI6_WPF_SRCRPF_RPF_ACT_SUB(n) (1 << ((n) * 2)) #define VI6_WPF_SRCRPF_RPF_ACT_MST(n) (2 << ((n) * 2)) @@ -321,6 +325,8 @@ #define VI6_DPR_HST_ROUTE 0x2044 #define VI6_DPR_HSI_ROUTE 0x2048 #define VI6_DPR_BRU_ROUTE 0x204c +#define VI6_DPR_ILV_BRS_ROUTE 0x2050 +#define VI6_DPR_ROUTE_BRSSEL (1 << 28) #define VI6_DPR_ROUTE_FXA_MASK (0xff << 16) #define VI6_DPR_ROUTE_FXA_SHIFT 16 #define VI6_DPR_ROUTE_FP_MASK (0x3f << 8) @@ -344,6 +350,7 @@ #define VI6_DPR_NODE_CLU 29 #define VI6_DPR_NODE_HST 30 #define VI6_DPR_NODE_HSI 31 +#define VI6_DPR_NODE_BRS_IN(n) (38 + (n)) #define VI6_DPR_NODE_LIF 55 #define VI6_DPR_NODE_WPF(n) (56 + (n)) #define VI6_DPR_NODE_UNUSED 63 @@ -476,7 +483,7 @@ #define VI6_HSI_CTRL_EN (1 << 0)

/* -----------------------------------------------------------------------------

• • BRU Control Registers

• • BRS and BRU Control Registers
  */

#define VI6_ROP_NOP 0 @@ -496,7 +503,10 @@ #define VI6_ROP_NAND 14 #define VI6_ROP_SET 15

-#define VI6_BRU_INCTRL 0x2c00 +#define VI6_BRU_BASE 0x2c00 +#define VI6_BRS_BASE 0x3900

+#define VI6_BRU_INCTRL 0x0000 #define VI6_BRU_INCTRL_NRM (1 << 28) #define VI6_BRU_INCTRL_DnON (1 << (16 + (n))) #define VI6_BRU_INCTRL_DITHn_OFF (0 << ((n) * 4)) @@ -508,19 +518,19 @@ #define VI6_BRU_INCTRL_DITHn_MASK (7 << ((n) * 4)) #define VI6_BRU_INCTRL_DITHn_SHIFT ((n) * 4)

-#define VI6_BRU_VIRRPF_SIZE 0x2c04 +#define VI6_BRU_VIRRPF_SIZE 0x0004 #define VI6_BRU_VIRRPF_SIZE_HSIZE_MASK (0x1fff << 16) #define VI6_BRU_VIRRPF_SIZE_HSIZE_SHIFT 16 #define VI6_BRU_VIRRPF_SIZE_VSIZE_MASK (0x1fff << 0) #define VI6_BRU_VIRRPF_SIZE_VSIZE_SHIFT 0

-#define VI6_BRU_VIRRPF_LOC 0x2c08 +#define VI6_BRU_VIRRPF_LOC 0x0008 #define VI6_BRU_VIRRPF_LOC_HCOORD_MASK (0x1fff << 16) #define VI6_BRU_VIRRPF_LOC_HCOORD_SHIFT 16 #define VI6_BRU_VIRRPF_LOC_VCOORD_MASK (0x1fff << 0) #define VI6_BRU_VIRRPF_LOC_VCOORD_SHIFT 0

-#define VI6_BRU_VIRRPF_COL 0x2c0c +#define VI6_BRU_VIRRPF_COL 0x000c #define VI6_BRU_VIRRPF_COL_A_MASK (0xff << 24) #define VI6_BRU_VIRRPF_COL_A_SHIFT 24 #define VI6_BRU_VIRRPF_COL_RCR_MASK (0xff << 16) @@ -530,7 +540,7 @@ #define VI6_BRU_VIRRPF_COL_BCB_MASK (0xff << 0) #define VI6_BRU_VIRRPF_COL_BCB_SHIFT 0

-#define VI6_BRU_CTRL(n) (0x2c10 + (n) * 8 + ((n) <= 3 ? 0 : 4)) +#define VI6_BRU_CTRL(n) (0x0010 + (n) * 8 + ((n) <= 3 ? 0 : 4)) #define VI6_BRU_CTRL_RBC (1 << 31) #define VI6_BRU_CTRL_DSTSEL_BRUIN(n) (((n) <= 3 ? (n) : (n)+1) << 20) #define VI6_BRU_CTRL_DSTSEL_VRPF (4 << 20) @@ -543,7 +553,7 @@ #define VI6_BRU_CTRL_AROP(rop) ((rop) << 0) #define VI6_BRU_CTRL_AROP_MASK (0xf << 0)

-#define VI6_BRU_BLD(n) (0x2c14 + (n) * 8 + ((n) <= 3 ? 0 : 4)) +#define VI6_BRU_BLD(n) (0x0014 + (n) * 8 + ((n) <= 3 ? 0 : 4)) #define VI6_BRU_BLD_CBES (1 << 31) #define VI6_BRU_BLD_CCMDX_DST_A (0 << 28) #define VI6_BRU_BLD_CCMDX_255_DST_A (1 << 28) @@ -576,7 +586,7 @@ #define VI6_BRU_BLD_COEFY_MASK (0xff << 0) #define VI6_BRU_BLD_COEFY_SHIFT 0

-#define VI6_BRU_ROP 0x2c30 +#define VI6_BRU_ROP 0x0030 /* Only available on BRU */ #define VI6_BRU_ROP_DSTSEL_BRUIN(n) (((n) <= 3 ? (n) : (n)+1) << 20) #define VI6_BRU_ROP_DSTSEL_VRPF (4 << 20) #define VI6_BRU_ROP_DSTSEL_MASK (7 << 20) diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c index 5af3486afe07..84139affb871 100644 --- a/drivers/media/platform/vsp1/vsp1_video.c +++ b/drivers/media/platform/vsp1/vsp1_video.c @@ -481,7 +481,7 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe, struct media_entity_enum ent_enum; struct vsp1_entity *entity; struct media_pad *pad;

• bool bru_found = false;

• struct vsp1_bru *bru = NULL; int ret;

  ret = media_entity_enum_init(&ent_enum, &input->entity.vsp1->media_dev);

@@ -511,16 +511,20 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe, media_entity_to_v4l2_subdev(pad->entity));

/*

•  * A BRU is present in the pipeline, store the BRU input pad
  

•  * A BRU or BRS is present in the pipeline, store its input pad
  

  • number in the input RPF for use when configuring the RPF.
  */

• if (entity->type == VSP1_ENTITY_BRU) {
  

• 	struct vsp1_bru *bru = to_bru(&entity->subdev);
  

• if (entity->type == VSP1_ENTITY_BRU ||
  

•     entity->type == VSP1_ENTITY_BRS) {
  

• 	/* BRU and BRS can't be chained. */
  

• 	if (bru) {
  

• 		ret = -EPIPE;
  

• 		goto out;
  

• 	}
  
  

• 	bru = to_bru(&entity->subdev);
  bru->inputs[pad->index].rpf = input;
  input->bru_input = pad->index;
  

• 	bru_found = true;
  

  }

  /* We've reached the WPF, we're done. */

@@ -542,8 +546,7 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe, }

	pipe->uds = entity;

• 	pipe->uds_input = bru_found ? pipe->bru
  

• 			: &input->entity;
  

• 	pipe->uds_input = bru ? &bru->entity : &input->entity;
  

  }

  /* Follow the source link, ignoring any HGO or HGT. */

@@ -589,30 +592,42 @@ static int vsp1_video_pipeline_build(struct vsp1_pipeline *pipe, e = to_vsp1_entity(subdev); list_add_tail(&e->list_pipe, &pipe->entities);

• if (e->type == VSP1_ENTITY_RPF) {
  

• switch (e->type) {
  

• case VSP1_ENTITY_RPF:
  rwpf = to_rwpf(subdev);
  pipe->inputs[rwpf->entity.index] = rwpf;
  rwpf->video->pipe_index = ++pipe->num_inputs;
  rwpf->pipe = pipe;
  

• } else if (e->type == VSP1_ENTITY_WPF) {
  

• 	break;
  

• case VSP1_ENTITY_WPF:
  rwpf = to_rwpf(subdev);
  pipe->output = rwpf;
  rwpf->video->pipe_index = 0;
  rwpf->pipe = pipe;
  

• } else if (e->type == VSP1_ENTITY_LIF) {
  

• 	break;
  

• case VSP1_ENTITY_LIF:
  pipe->lif = e;
  

• } else if (e->type == VSP1_ENTITY_BRU) {
  

• 	break;
  

• case VSP1_ENTITY_BRU:
  

• case VSP1_ENTITY_BRS:
  pipe->bru = e;
  

• } else if (e->type == VSP1_ENTITY_HGO) {
  

• 	struct vsp1_hgo *hgo = to_hgo(subdev);
  

• 	break;
  
  

• case VSP1_ENTITY_HGO:
  pipe->hgo = e;
  

• 	hgo->histo.pipe = pipe;
  

• } else if (e->type == VSP1_ENTITY_HGT) {
  

• 	struct vsp1_hgt *hgt = to_hgt(subdev);
  

• 	to_hgo(subdev)->histo.pipe = pipe;
  

• 	break;
  
  

• case VSP1_ENTITY_HGT:
  pipe->hgt = e;
  

• 	hgt->histo.pipe = pipe;
  

• 	to_hgt(subdev)->histo.pipe = pipe;
  

• 	break;
  

• default:
  

• 	break;
  

  } }

@@ -796,12 +811,14 @@ static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe) struct vsp1_uds *uds = to_uds(&pipe->uds->subdev);

/*

•  * If a BRU is present in the pipeline before the UDS, the alpha
  

•  * component doesn't need to be scaled as the BRU output alpha
  

•  * value is fixed to 255. Otherwise we need to scale the alpha
  

•  * component only when available at the input RPF.
  

•  * If a BRU or BRS is present in the pipeline before the UDS,
  

•  * the alpha component doesn't need to be scaled as the BRU and
  

•  * BRS output alpha value is fixed to 255. Otherwise we need to
  

•  * scale the alpha component only when available at the input
  

•  * RPF.
  

  */

• if (pipe->uds_input->type == VSP1_ENTITY_BRU) {
  

• if (pipe->uds_input->type == VSP1_ENTITY_BRU ||
  

•     pipe->uds_input->type == VSP1_ENTITY_BRS) {
  uds->scale_alpha = false;
  

  } else { struct vsp1_rwpf *rpf =

diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c index 32df109b119f..b6c902be225b 100644 --- a/drivers/media/platform/vsp1/vsp1_wpf.c +++ b/drivers/media/platform/vsp1/vsp1_wpf.c @@ -453,7 +453,9 @@ static void wpf_configure(struct vsp1_entity *entity, }

if (pipe->bru || pipe->num_inputs > 1)

• srcrpf |= VI6_WPF_SRCRPF_VIRACT_MST;
  

• srcrpf |= pipe->bru->type == VSP1_ENTITY_BRU
  

• 	? VI6_WPF_SRCRPF_VIRACT_MST
  

• 	: VI6_WPF_SRCRPF_VIRACT2_MST;
  
  

  vsp1_wpf_write(wpf, dl, VI6_WPF_SRCRPF, srcrpf);

Hi Kieran,

On Thursday 13 Jul 2017 18:49:19 Kieran Bingham wrote:

On 26/06/17 19:12, Laurent Pinchart wrote:

...

New Gen3 SoCs come with two new VSP2 variants names VSP2-BS and VSP2-DL, as well as a new VSP2-D variant on V3M and V3H SoCs. Add new entries for them in the VSP device info table.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Code in the patch looks OK - but I can't see where the difference between the horizontal widths are supported between VSPD H3/VC

I see this in the datasheet: (32.1.1.6 in this particular part)

Direct connection to display module — Supporting 4096 pixels in horizontal direction [R-Car H3/R-Car M3-W/ R-Car M3-N] — Supporting 2048 pixels in horizontal direction [R-Car V3M/R-Car V3H/R-Car D3/R-Car E3]

Do we have this information encoded anywhere? or are they just talking about maximum performance capability there?

No, we don't. It's a limit that we should have. I think we should fix that in a separate patch, as the 4096 pixels limit isn't implemented either.

Also some features that are implied as supported aren't mentioned - but that's not a blocker to adding in the initial devices at all.

Therefore:

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

...

---

drivers/media/platform/vsp1/vsp1_drv.c | 24 ++++++++++++++++++++++++ drivers/media/platform/vsp1/vsp1_regs.h | 15 +++++++++++++-- 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c index 6a9aeb71aedf..c4f2ac61f7d2 100644 --- a/drivers/media/platform/vsp1/vsp1_drv.c +++ b/drivers/media/platform/vsp1/vsp1_drv.c @@ -710,6 +710,14 @@ static const struct vsp1_device_info vsp1_device_infos[] = {> .num_bru_inputs = 5, .uapi = true, }, {

• .version = VI6_IP_VERSION_MODEL_VSPBS_GEN3,
  

• .model = "VSP2-BS",
  

• .gen = 3,
  

• .features = VSP1_HAS_BRS,
  

32.1.1.5 implies: | VSP1_HAS_WPF_VFLIP

But Figure 32.5 implies that it doesn't ...

The figures only tell whether the full combination of rotation and H/V flip is available. I think you're right, I'll add VSP1_HAS_WPF_VFLIP.

Figure 32.5 also implies that | VSP1_HAS_CLU is there too on both RPF0, and RPF1

Note that CLUT != CLU. I know it's confusing :-)

...

• .rpf_count = 2,
  

• .wpf_count = 1,
  

• .uapi = true,
  

• }, { .version = VI6_IP_VERSION_MODEL_VSPD_GEN3, .model = "VSP2-D", .gen = 3,

@@ -717,6 +725,22 @@ static const struct vsp1_device_info vsp1_device_infos[] = {> .rpf_count = 5, .wpf_count = 2, .num_bru_inputs = 5,

• }, {

• .version = VI6_IP_VERSION_MODEL_VSPD_V3,
  

• .model = "VSP2-D",
  

• .gen = 3,
  

• .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
  

• .rpf_count = 5,
  

• .wpf_count = 1,
  

• .num_bru_inputs = 5,
  

• }, {

• .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
  

• .model = "VSP2-DL",
  

• .gen = 3,
  

• .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF,
  

Hrm. 32.1.1.7 says: — Vertical flipping in case of output to memory. So thats some sort of a conditional : | VSP1_HAS_WPF_VFLIP

So looking at this and the settings of the existing models, I guess it looks like we don't support flip if we have an LIF output (as that would then be unsupported)

On Gen3 vertical flipping seems to always be supported, unlike on Gen2 where VSPD is specifically documented as not supporting vertical flipping. We could add the WFLIP on all VSP2-D* instances. This would create a corresponding control, which wouldn't do much harm as the VSPD instances on Gen3 are not exposed to userspace, but that would waste a bit of memory for no good purpose (beside correctness I suppose). I wonder if that's worth it, what do you think ? If so, VSP2-D should be fixed too, so I'd prefer doing that in a separate patch.

...

• .rpf_count = 5,
  

• .wpf_count = 2,
  

• .num_bru_inputs = 5,
  

  },

};

[snip]

New Gen3 SoCs come with two new VSP2 variants names VSP2-BS and VSP2-DL, as well as a new VSP2-D variant on V3M and V3H SoCs. Add new entries for them in the VSP device info table.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com --- Changes since v2:

- Added VSP1_HAS_WPF_VFLIP to VSP2-BS --- drivers/media/platform/vsp1/vsp1_drv.c | 24 ++++++++++++++++++++++++ drivers/media/platform/vsp1/vsp1_regs.h | 15 +++++++++++++-- 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c index 6a9aeb71aedf..d1682a9719af 100644 --- a/drivers/media/platform/vsp1/vsp1_drv.c +++ b/drivers/media/platform/vsp1/vsp1_drv.c @@ -710,6 +710,14 @@ static const struct vsp1_device_info vsp1_device_infos[] = { .num_bru_inputs = 5, .uapi = true, }, { + .version = VI6_IP_VERSION_MODEL_VSPBS_GEN3, + .model = "VSP2-BS", + .gen = 3, + .features = VSP1_HAS_BRS | VSP1_HAS_WPF_VFLIP, + .rpf_count = 2, + .wpf_count = 1, + .uapi = true, + }, { .version = VI6_IP_VERSION_MODEL_VSPD_GEN3, .model = "VSP2-D", .gen = 3, @@ -717,6 +725,22 @@ static const struct vsp1_device_info vsp1_device_infos[] = { .rpf_count = 5, .wpf_count = 2, .num_bru_inputs = 5, + }, { + .version = VI6_IP_VERSION_MODEL_VSPD_V3, + .model = "VSP2-D", + .gen = 3, + .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF, + .rpf_count = 5, + .wpf_count = 1, + .num_bru_inputs = 5, + }, { + .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3, + .model = "VSP2-DL", + .gen = 3, + .features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_LIF, + .rpf_count = 5, + .wpf_count = 2, + .num_bru_inputs = 5, }, };

diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h index 744217e020b9..ab439a60a100 100644 --- a/drivers/media/platform/vsp1/vsp1_regs.h +++ b/drivers/media/platform/vsp1/vsp1_regs.h @@ -699,9 +699,20 @@ #define VI6_IP_VERSION_MODEL_VSPBD_GEN3 (0x15 << 8) #define VI6_IP_VERSION_MODEL_VSPBC_GEN3 (0x16 << 8) #define VI6_IP_VERSION_MODEL_VSPD_GEN3 (0x17 << 8) +#define VI6_IP_VERSION_MODEL_VSPD_V3 (0x18 << 8) +#define VI6_IP_VERSION_MODEL_VSPDL_GEN3 (0x19 << 8) +#define VI6_IP_VERSION_MODEL_VSPBS_GEN3 (0x1a << 8) #define VI6_IP_VERSION_SOC_MASK (0xff << 0) -#define VI6_IP_VERSION_SOC_H (0x01 << 0) -#define VI6_IP_VERSION_SOC_M (0x02 << 0) +#define VI6_IP_VERSION_SOC_H2 (0x01 << 0) +#define VI6_IP_VERSION_SOC_V2H (0x01 << 0) +#define VI6_IP_VERSION_SOC_V3M (0x01 << 0) +#define VI6_IP_VERSION_SOC_M2 (0x02 << 0) +#define VI6_IP_VERSION_SOC_M3W (0x02 << 0) +#define VI6_IP_VERSION_SOC_V3H (0x02 << 0) +#define VI6_IP_VERSION_SOC_H3 (0x03 << 0) +#define VI6_IP_VERSION_SOC_D3 (0x04 << 0) +#define VI6_IP_VERSION_SOC_M3N (0x04 << 0) +#define VI6_IP_VERSION_SOC_E3 (0x04 << 0)

/* ----------------------------------------------------------------------------- * RPF CLUT Registers

Hi Laurent,

On 26/06/17 19:12, Laurent Pinchart wrote:

The VSP supports both header and headerless display lists. The latter is easier to use when the VSP feeds data directly to the DU in continuous mode, and the driver thus uses headerless display lists for DU operation and header display lists otherwise.

Headerless display lists are only available on WPF.0. This has never been an issue so far, as only WPF.0 is connected to the DU. However, on H3 ES2.0, the VSP-DL instance has both WPF.0 and WPF.1 connected to the DU. We thus can't use headerless display lists unconditionally for DU operation.

Would it be crazy to suggest we drop headerless display lists?

If we must support header display lists in continuous mode - Rather than having 2 cases for continuous modes to support (having to support headerless, on WPF.0, and header on WPF.1) if we just use your header loop trick - would that simplify our code maintenance?

(We can always remove headerless support later if you agree, this is more of an idea at the moment)

Implement support for continuous mode with header display lists, and use it for DU operation on WPF outputs that don't support headerless mode.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Except for future discussion points, I can't see anything wrong here so:

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

drivers/media/platform/vsp1/vsp1_dl.c | 195 +++++++++++++++++++++----------- drivers/media/platform/vsp1/vsp1_regs.h | 1 + 2 files changed, 127 insertions(+), 69 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c index bb92be4fe0f0..8b5cbb6b7a70 100644 --- a/drivers/media/platform/vsp1/vsp1_dl.c +++ b/drivers/media/platform/vsp1/vsp1_dl.c @@ -95,6 +95,7 @@ enum vsp1_dl_mode {

• struct vsp1_dl_manager - Display List manager
• @index: index of the related WPF
• @mode: display list operation mode (header or headerless)

• • @singleshot: execute the display list in single-shot mode
  • @vsp1: the VSP1 device
  • @lock: protects the free, active, queued, pending and gc_fragments lists
  • @free: array of all free display lists

@@ -107,6 +108,7 @@ enum vsp1_dl_mode { struct vsp1_dl_manager { unsigned int index; enum vsp1_dl_mode mode;

• bool singleshot; struct vsp1_device *vsp1;

  spinlock_t lock;

@@ -437,6 +439,7 @@ int vsp1_dl_list_add_chain(struct vsp1_dl_list *head,

static void vsp1_dl_list_fill_header(struct vsp1_dl_list *dl, bool is_last) {

• struct vsp1_dl_manager *dlm = dl->dlm; struct vsp1_dl_header_list *hdr = dl->header->lists; struct vsp1_dl_body *dlb; unsigned int num_lists = 0;

@@ -461,85 +464,145 @@ static void vsp1_dl_list_fill_header(struct vsp1_dl_list *dl, bool is_last)

dl->header->num_lists = num_lists;

• /*

• * If this display list's chain is not empty, we are on a list, where
  

• * the next item in the list is the display list entity which should be
  

• * automatically queued by the hardware.
  

• */
  

  if (!list_empty(&dl->chain) && !is_last) {

• /*
  

•  * If this display list's chain is not empty, we are on a list,
  

•  * and the next item is the display list that we must queue for
  

•  * automatic processing by the hardware.
  

•  */
  

  struct vsp1_dl_list *next = list_next_entry(dl, chain);

  dl->header->next_header = next->dma; dl->header->flags = VSP1_DLH_AUTO_START;

• } else if (!dlm->singleshot) {

• /*
  

•  * if the display list manager works in continuous mode, the VSP
  

•  * should loop over the display list continuously until
  

•  * instructed to do otherwise.
  

•  */
  

• dl->header->next_header = dl->dma;
  

• dl->header->flags = VSP1_DLH_INT_ENABLE | VSP1_DLH_AUTO_START;
  

This is a neat trick - and I wonder if actually - it gives us even more flexibility in our control.

Presently on continuous mode - I believe we have to reset the WPF to 'stop' ... In this mode - I suspect we could update the display list - (albeit that might be a bit racy) to disable the AUTO_START. Maybe that's better .. Maybe it's worse :)

} else {

• /*
  

•  * Otherwise, in mem-to-mem mode, we work in single-shot mode
  

•  * and the next display list must not be started automatically.
  

•  */
  

  dl->header->flags = VSP1_DLH_INT_ENABLE; }

}

-void vsp1_dl_list_commit(struct vsp1_dl_list *dl) +static bool vsp1_dl_list_hw_update_pending(struct vsp1_dl_manager *dlm) {

• struct vsp1_dl_manager *dlm = dl->dlm; struct vsp1_device *vsp1 = dlm->vsp1;

• unsigned long flags;

• bool update;

• if (dl->dlm->mode == VSP1_DL_MODE_HEADER) {

• struct vsp1_dl_list *dl_child;
  

• /*
  

•  * In header mode the caller guarantees that the hardware is
  

•  * idle at this point.
  

•  */
  
  

• /* Fill the header for the head and chained display lists. */
  

• vsp1_dl_list_fill_header(dl, list_empty(&dl->chain));
  

• list_for_each_entry(dl_child, &dl->chain, chain) {
  

• 	bool last = list_is_last(&dl_child->chain, &dl->chain);
  

• if (!dlm->queued)

• return false;
  
  

• 	vsp1_dl_list_fill_header(dl_child, last);
  

• }
  

• }

• /*

• * Check whether the VSP1 has taken the update. In headerless mode the
  

• * hardware indicates this by clearing the UPD bit in the DL_BODY_SIZE
  

• * register, and in header mode by clearing the UPDHDR bit in the CMD
  

• * register.
  

• */
  

• if (dlm->mode == VSP1_DL_MODE_HEADERLESS)

• return !!(vsp1_read(vsp1, VI6_DL_BODY_SIZE)
  

• 	  & VI6_DL_BODY_SIZE_UPD);
  

• else

• return !!(vsp1_read(vsp1, VI6_CMD(dlm->index) & VI6_CMD_UPDHDR));
  

+}

• spin_lock_irqsave(&dlm->lock, flags);

+static void vsp1_dl_list_hw_enqueue(struct vsp1_dl_list *dl) +{

• struct vsp1_dl_manager *dlm = dl->dlm;
• struct vsp1_device *vsp1 = dlm->vsp1;

• if (dl->dlm->mode == VSP1_DL_MODE_HEADER) {

• if (dlm->mode == VSP1_DL_MODE_HEADERLESS) { /*

•  * Commit the head display list to hardware. Chained headers
  

•  * will auto-start.
  

•  * In headerless mode, program the hardware directly with the
  

•  * display list body address and size and set the UPD bit. The
  

•  * bit will be cleared by the hardware when the display list
  

•  * processing starts.
  

•  */
  

• vsp1_write(vsp1, VI6_DL_HDR_ADDR(0), dl->body0.dma);
  

• vsp1_write(vsp1, VI6_DL_BODY_SIZE, VI6_DL_BODY_SIZE_UPD |
  

• 	   (dl->body0.num_entries * sizeof(*dl->header->lists)));
  

• } else {

• /*
  

•  * In header mode, program the display list header address. If
  

•  * the hardware is idle (single-shot mode or first frame in
  

•  * continuous mode) it will then be started independently. If
  

•  * the hardware is operating, the VI6_DL_HDR_REF_ADDR register
  

•  * will be updated with the display list address.
  

  */ vsp1_write(vsp1, VI6_DL_HDR_ADDR(dlm->index), dl->dma);

• dlm->active = dl;
  

• goto done;
  

  }

+}

+static void vsp1_dl_list_commit_continuous(struct vsp1_dl_list *dl) +{

• struct vsp1_dl_manager *dlm = dl->dlm;

  /*

• * Once the UPD bit has been set the hardware can start processing the
  

• * display list at any time and we can't touch the address and size
  

• * registers. In that case mark the update as pending, it will be
  

• * queued up to the hardware by the frame end interrupt handler.
  

• * If a previous display list has been queued to the hardware but not
  

• * processed yet, the VSP can start processing it at any time. In that
  

• * case we can't replace the queued list by the new one, as we could
  

• * race with the hardware. We thus mark the update as pending, it will
  

• * be queued up to the hardware by the frame end interrupt handler.
  

  */

• update = !!(vsp1_read(vsp1, VI6_DL_BODY_SIZE) & VI6_DL_BODY_SIZE_UPD);
• if (update) {

• if (vsp1_dl_list_hw_update_pending(dlm)) { __vsp1_dl_list_put(dlm->pending); dlm->pending = dl;

• goto done;
  

• return;
  

  }

  /*

• * Program the hardware with the display list body address and size.
  

• * The UPD bit will be cleared by the device when the display list is
  

• * processed.
  

• * Pass the new display list to the hardware and mark it as queued. It
  

• * will become active when the hardware starts processing it.
  

  */

• vsp1_write(vsp1, VI6_DL_HDR_ADDR(0), dl->body0.dma);
• vsp1_write(vsp1, VI6_DL_BODY_SIZE, VI6_DL_BODY_SIZE_UPD |

•    (dl->body0.num_entries * sizeof(*dl->header->lists)));
  

• vsp1_dl_list_hw_enqueue(dl);

  __vsp1_dl_list_put(dlm->queued); dlm->queued = dl;

+}

+static void vsp1_dl_list_commit_singleshot(struct vsp1_dl_list *dl) +{

• struct vsp1_dl_manager *dlm = dl->dlm;
• /*

• * When working in single-shot mode, the caller guarantees that the
  

• * hardware is idle at this point. Just commit the head display list
  

• * to hardware. Chained lists will be started automatically.
  

• */
  

• vsp1_dl_list_hw_enqueue(dl);
• dlm->active = dl;

+}

I'm pleased to see the _commit_singleshot split out. That certainly makes that case read easier :)

+void vsp1_dl_list_commit(struct vsp1_dl_list *dl) +{

• struct vsp1_dl_manager *dlm = dl->dlm;
• struct vsp1_dl_list *dl_child;
• unsigned long flags;
• if (dlm->mode == VSP1_DL_MODE_HEADER) {

• /* Fill the header for the head and chained display lists. */
  

• vsp1_dl_list_fill_header(dl, list_empty(&dl->chain));
  

• list_for_each_entry(dl_child, &dl->chain, chain) {
  

• 	bool last = list_is_last(&dl_child->chain, &dl->chain);
  

• 	vsp1_dl_list_fill_header(dl_child, last);
  

• }
  

• }
• spin_lock_irqsave(&dlm->lock, flags);
• if (dlm->singleshot)

• vsp1_dl_list_commit_singleshot(dl);
  

• else

• vsp1_dl_list_commit_continuous(dl);
  
  

-done: spin_unlock_irqrestore(&dlm->lock, flags); }

@@ -558,31 +621,28 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl) */ bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm) {

• struct vsp1_device *vsp1 = dlm->vsp1; bool completed = false;

  spin_lock(&dlm->lock);

• __vsp1_dl_list_put(dlm->active);

• dlm->active = NULL;

• /*

• * Header mode is used for mem-to-mem pipelines only. We don't need to
  

• * perform any operation as there can't be any new display list queued
  

• * in that case.
  

• * The mem-to-mem pipelines work in single-shot mode. No new display
  

• * list can be queued, we don't have to do anything.
  

  */

• if (dlm->mode == VSP1_DL_MODE_HEADER) {

• if (dlm->singleshot) {

• __vsp1_dl_list_put(dlm->active);
  

• dlm->active = NULL;
  

  completed = true; goto done; }

  /*

• * The UPD bit set indicates that the commit operation raced with the
  

• * interrupt and occurred after the frame end event and UPD clear but
  

• * before interrupt processing. The hardware hasn't taken the update
  

• * into account yet, we'll thus skip one frame and retry.
  

• * If the commit operation raced with the interrupt and occurred after
  

• * the frame end event but before interrupt processing, the hardware
  

• * hasn't taken the update into account yet. We have to skip one frame
  

• * and retry.
  

  */

• if (vsp1_read(vsp1, VI6_DL_BODY_SIZE) & VI6_DL_BODY_SIZE_UPD)

• if (vsp1_dl_list_hw_update_pending(dlm)) goto done;

  /*

@@ -590,24 +650,20 @@ bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm) * frame end interrupt. The display list thus becomes active. */ if (dlm->queued) {

• __vsp1_dl_list_put(dlm->active);
  

  dlm->active = dlm->queued; dlm->queued = NULL; completed = true; }

  /*

• * Now that the UPD bit has been cleared we can queue the next display
  

• * list to the hardware if one has been prepared.
  

• * Now that the VSP has started processing the queued display list, we
  

• * can queue the pending display list to the hardware if one has been
  

• * prepared.
  

  */ if (dlm->pending) {

• struct vsp1_dl_list *dl = dlm->pending;
  

• vsp1_write(vsp1, VI6_DL_HDR_ADDR(0), dl->body0.dma);
  

• vsp1_write(vsp1, VI6_DL_BODY_SIZE, VI6_DL_BODY_SIZE_UPD |
  

• 	   (dl->body0.num_entries *
  

• 	    sizeof(*dl->header->lists)));
  

• dlm->queued = dl;
  

• vsp1_dl_list_hw_enqueue(dlm->pending);
  

• dlm->queued = dlm->pending;
  

  dlm->pending = NULL; }

@@ -700,6 +756,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1, dlm->index = index; dlm->mode = index == 0 && !vsp1->info->uapi ? VSP1_DL_MODE_HEADERLESS : VSP1_DL_MODE_HEADER;

• dlm->singleshot = vsp1->info->uapi; dlm->vsp1 = vsp1;

  spin_lock_init(&dlm->lock);

diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h index 007e2bbc26c0..58d0bea963a6 100644 --- a/drivers/media/platform/vsp1/vsp1_regs.h +++ b/drivers/media/platform/vsp1/vsp1_regs.h @@ -18,6 +18,7 @@ */

#define VI6_CMD(n) (0x0000 + (n) * 4) +#define VI6_CMD_UPDHDR (1 << 4) #define VI6_CMD_STRCMD (1 << 0)

#define VI6_CLK_DCSWT 0x0018

Hi Laurent,

On 26/06/17 19:12, Laurent Pinchart wrote:

On R-Car H3 ES2.0, DU channels 0 and 3 are served by two separate pipelines from the same VSP. Support this in the DU driver.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

This looks good to me.

Minor nit / comment can be safely ignored. Mostly just me thinking outload.

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 3 ++ drivers/gpu/drm/rcar-du/rcar_du_kms.c | 91 ++++++++++++++++++++++++++++++---- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 37 +++++++------- drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 10 +++- 5 files changed, 110 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 345eff72f581..8f942ebdd0c6 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -721,7 +721,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index) rcrtc->index = index;

if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))

• primary = &rcrtc->vsp->planes[0].plane;
  

• primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
  

  else primary = &rgrp->planes[index % 2].plane;

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index b199ed5adf36..0b6d26ecfc38 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h @@ -35,6 +35,8 @@ struct rcar_du_vsp;

• @flip_wait: wait queue used to signal page flip completion
• @outputs: bitmask of the outputs (enum rcar_du_output) driven by this CRTC
• @group: CRTC group this CRTC belongs to

• • @vsp: VSP feeding video to this CRTC
• • @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
  */

struct rcar_du_crtc { struct drm_crtc crtc; @@ -52,6 +54,7 @@ struct rcar_du_crtc {

struct rcar_du_group *group; struct rcar_du_vsp *vsp;

• unsigned int vsp_pipe;

};

#define to_rcar_crtc(c) container_of(c, struct rcar_du_crtc, crtc) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index f4125c8ca902..82b978a5dae6 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -432,6 +432,83 @@ static int rcar_du_properties_init(struct rcar_du_device *rcdu) return 0; }

+static int rcar_du_vsps_init(struct rcar_du_device *rcdu) +{

• const struct device_node *np = rcdu->dev->of_node;
• struct of_phandle_args args;
• struct {

• struct device_node *np;
  

• unsigned int crtcs_mask;
  

• } vsps[RCAR_DU_MAX_VSPS] = { { 0, }, };
• unsigned int vsps_count = 0;
• unsigned int cells;
• unsigned int i;
• int ret;
• /*

• * First parse the DT vsps property to populate the list of VSPs. Each
  

• * entry contains a pointer to the VSP DT node and a bitmask of the
  

• * connected DU CRTCs.
  

• */
  

• cells = of_property_count_u32_elems(np, "vsps") / rcdu->num_crtcs - 1;
• if (cells > 1)

• return -EINVAL;
  

• for (i = 0; i < rcdu->num_crtcs; ++i) {

• unsigned int j;
  

• ret = of_parse_phandle_with_fixed_args(np, "vsps", cells, i,
  

• 				       &args);
  

• if (ret < 0)
  

• 	goto error;
  

• /*
  

•  * Add the VSP to the list or update the corresponding existing
  

•  * entry if the VSP has already been added.
  

•  */
  

• for (j = 0; j < vsps_count; ++j) {
  

• 	if (vsps[j].np == args.np)
  

• 		break;
  

• }
  

• if (j < vsps_count)
  

• 	of_node_put(args.np);
  

• else
  

• 	vsps[vsps_count++].np = args.np;
  

• vsps[j].crtcs_mask |= 1 << i;
  

I do love the BIT(x) macro personally - but it's not important :)

• /* Store the VSP pointer and pipe index in the CRTC. */
  

• rcdu->crtcs[i].vsp = &rcdu->vsps[j];
  

• rcdu->crtcs[i].vsp_pipe = cells >= 1 ? args.args[0] : 0;
  

• }
• /*

• * Then initialize all the VSPs from the node pointers and CRTCs bitmask
  

• * computed previously.
  

• */
  

• for (i = 0; i < vsps_count; ++i) {

• struct rcar_du_vsp *vsp = &rcdu->vsps[i];
  

• vsp->index = i;
  

• vsp->dev = rcdu;
  

• ret = rcar_du_vsp_init(vsp, vsps[i].np, vsps[i].crtcs_mask);
  

• if (ret < 0)
  

• 	goto error;
  

• }
• return 0;

+error:

• for (i = 0; i < ARRAY_SIZE(vsps); ++i) {

• if (vsps[i].np)
  

Minor nit: of_node_put already has NULL protection so we don't need this 'if' but it probably does make it clearer that we are only putting back nodes that we collected.

• 	of_node_put(vsps[i].np);
  

• }
• return ret;

+}

int rcar_du_modeset_init(struct rcar_du_device *rcdu) { static const unsigned int mmio_offsets[] = { @@ -499,17 +576,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)

/* Initialize the compositors. */ if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {

• for (i = 0; i < rcdu->num_crtcs; ++i) {
  

• 	struct rcar_du_vsp *vsp = &rcdu->vsps[i];
  

• 	vsp->index = i;
  

• 	vsp->dev = rcdu;
  

• 	rcdu->crtcs[i].vsp = vsp;
  

• 	ret = rcar_du_vsp_init(vsp);
  

• 	if (ret < 0)
  

• 		return ret;
  

• }
  

• ret = rcar_du_vsps_init(rcdu);
  

• if (ret < 0)
  

• 	return ret;
  

  }

  /* Create the CRTCs. */

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index d46dce054442..3e9098dac0d2 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -19,6 +19,7 @@ #include <drm/drm_gem_cma_helper.h> #include <drm/drm_plane_helper.h>

+#include <linux/bitops.h> #include <linux/dma-mapping.h> #include <linux/of_platform.h> #include <linux/scatterlist.h> @@ -81,22 +82,22 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) */ crtc->group->need_restart = true;

• vsp1_du_setup_lif(crtc->vsp->vsp, 0, &cfg);

• vsp1_du_setup_lif(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);

}

void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) {

• vsp1_du_setup_lif(crtc->vsp->vsp, 0, NULL);

• vsp1_du_setup_lif(crtc->vsp->vsp, crtc->vsp_pipe, NULL);

}

void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) {

• vsp1_du_atomic_begin(crtc->vsp->vsp, 0);

• vsp1_du_atomic_begin(crtc->vsp->vsp, crtc->vsp_pipe);

}

void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc) {

• vsp1_du_atomic_flush(crtc->vsp->vsp, 0);

• vsp1_du_atomic_flush(crtc->vsp->vsp, crtc->vsp_pipe);

}

/* Keep the two tables in sync. */ @@ -162,6 +163,7 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane) { struct rcar_du_vsp_plane_state *state = to_rcar_vsp_plane_state(plane->plane.state);

• struct rcar_du_crtc *crtc = to_rcar_crtc(state->state.crtc); struct drm_framebuffer *fb = plane->plane.state->fb; struct vsp1_du_atomic_config cfg = { .pixelformat = 0,

@@ -192,7 +194,8 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane) } }

• vsp1_du_atomic_update(plane->vsp->vsp, 0, plane->index, &cfg);

• vsp1_du_atomic_update(plane->vsp->vsp, crtc->vsp_pipe,

• 	      plane->index, &cfg);
  

}

static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane, @@ -288,11 +291,13 @@ static void rcar_du_vsp_plane_atomic_update(struct drm_plane *plane, struct drm_plane_state *old_state) { struct rcar_du_vsp_plane *rplane = to_rcar_vsp_plane(plane);

• struct rcar_du_crtc *crtc = to_rcar_crtc(old_state->crtc);

  if (plane->state->crtc) rcar_du_vsp_plane_setup(rplane); else

• vsp1_du_atomic_update(rplane->vsp->vsp, 0, rplane->index, NULL);
  

• vsp1_du_atomic_update(rplane->vsp->vsp, crtc->vsp_pipe,
  

• 		      rplane->index, NULL);
  

}

static const struct drm_plane_helper_funcs rcar_du_vsp_plane_helper_funcs = { @@ -391,23 +396,17 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { .atomic_get_property = rcar_du_vsp_plane_atomic_get_property, };

-int rcar_du_vsp_init(struct rcar_du_vsp *vsp) +int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,

•      unsigned int crtcs)
  

{ struct rcar_du_device *rcdu = vsp->dev; struct platform_device *pdev;

• struct device_node *np;

• unsigned int num_crtcs = hweight32(crtcs);

Ohhh a new one to me - I didn't know we had hweight in the kernel. I think I'd done bit-counts maually elsewhere. :S

/me updates mind wiki.

unsigned int i; int ret;

/* Find the VSP device and initialize it. */

• np = of_parse_phandle(rcdu->dev->of_node, "vsps", vsp->index);
• if (!np) {

• dev_err(rcdu->dev, "vsps node not found\n");
  

• return -ENXIO;
  

• }
• pdev = of_find_device_by_node(np);
• of_node_put(np); if (!pdev) return -ENXIO;

@@ -428,15 +427,15 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp) return -ENOMEM;

for (i = 0; i < vsp->num_planes; ++i) {

• enum drm_plane_type type = i ? DRM_PLANE_TYPE_OVERLAY
  

• 			 : DRM_PLANE_TYPE_PRIMARY;
  

• enum drm_plane_type type = i < num_crtcs
  

• 			 ? DRM_PLANE_TYPE_PRIMARY
  

• 			 : DRM_PLANE_TYPE_OVERLAY;
  

  struct rcar_du_vsp_plane *plane = &vsp->planes[i];

  plane->vsp = vsp; plane->index = i;

• ret = drm_universal_plane_init(rcdu->ddev, &plane->plane,
  

• 			       1 << vsp->index,
  

• ret = drm_universal_plane_init(rcdu->ddev, &plane->plane, crtcs,
  		       &rcar_du_vsp_plane_funcs,
  		       formats_kms,
  		       ARRAY_SIZE(formats_kms), type,
  

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h index 8861661590ff..f876c512163c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h @@ -64,13 +64,19 @@ to_rcar_vsp_plane_state(struct drm_plane_state *state) }

#ifdef CONFIG_DRM_RCAR_VSP -int rcar_du_vsp_init(struct rcar_du_vsp *vsp); +int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,

•      unsigned int crtcs);
  

void rcar_du_vsp_enable(struct rcar_du_crtc *crtc); void rcar_du_vsp_disable(struct rcar_du_crtc *crtc); void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc); void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc); #else -static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp) { return -ENXIO; }; +static inline int rcar_du_vsp_init(struct rcar_du_vsp *vsp,

• 		   struct device_node *np,
  

• 		   unsigned int crtcs)
  

+{

• return -ENXIO;

+} static inline void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) { }; static inline void rcar_du_vsp_disable(struct rcar_du_crtc *crtc) { }; static inline void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) { };

Hi Kieran,

On Tuesday 01 Aug 2017 15:06:20 Kieran Bingham wrote:

On 26/06/17 19:12, Laurent Pinchart wrote:

...

The H3 ES1.x exhibits dot clock duty cycle stability issues. We can work around them by configuring the DPLL to twice the desired frequency, coupled with a /2 post-divider. This isn't needed on other SoCs and breaks HDMI output on M3-W for a currently unknown reason, so restrict the workaround to H3 ES1.x.

From an implementation point of view, move work around handling outside of the rcar_du_dpll_divider() function by requesting a x2 DPLL output frequency explicitly. The existing post-divider calculation mechanism will then take care of dividing the clock by two automatically.

While at it, print a more useful debugging message to ease debugging clock rate issues.

Signed-off-by: Laurent Pinchart

laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 37 +++++++++++++++++++++------- 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 8f942ebdd0c6..6c29981377c0 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -13,6 +13,7 @@

#include <linux/clk.h> #include <linux/mutex.h> +#include <linux/sys_soc.h>

#include <drm/drmP.h> #include <drm/drm_atomic.h> @@ -129,10 +130,8 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc *rcrtc,> for (fdpll = 1; fdpll < 32; fdpll++) { unsigned long output;

• 		/* 1/2 (FRQSEL=1) for duty rate 50% */
  	output = input * (n + 1) / (m + 1)
  

• 		       / (fdpll + 1) / 2;
  

• 		       / (fdpll + 1);
  

I'm finding this hard to interpret vs the commit-message.

Here we remove the /2 (which affects all targets... is this a problem?)

The purpose of this function is to compute DPLL parameters for given input and output frequencies. However, the current implementation computes parameters that result in twice the requested frequency, assuming that the caller will configure a /2 post-divider.

I found this confusing, so the patch modifies the function to use the requested output frequency, and updates the caller accordingly. The function now performs the operation described by its name.

This indeed affects all targets, but there's no DPLL on Gen2, so in practice only H3 and M3-W are affected.

...

		if (output >= 400000000)
			continue;

[snip]

...

@@ -185,7 +189,20 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)> extclk = clk_get_rate(rcrtc->extclock); if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {

• 	rcar_du_dpll_divider(rcrtc, &dpll, extclk,
  

mode_clock);

• 	unsigned long target = mode_clock;
  

• 	/*
  

• 	 * The H3 ES1.x exhibits dot clock duty cycle
  

stability

• 	 * issues. We can work around them by configuring the
  

• 	 * DPLL to twice the desired frequency, coupled with a
  

• 	 * /2 post-divider. This isn't needed on other SoCs
  

and

But here we discuss 'coupling' it with a /2 post - divider.

My inference here then is that by setting a target that is 'twice' the value

• code later will provide the /2 post-divide?

Correct. The /2 post-divider is required on H3 ES1.x to obtain a stable output clock, so the output of the DPLL has to be twice the pixel clock frequency. Based on my understanding it shouldn't hurt to do the same on H3 ES2.0 and M3- W, but in practice that doesn't work. I've thus restricted the post-divider to H3 ES1.x.

...

• 	 * breaks HDMI output on M3-W for a currently unknown
  

• 	 * reason, so restrict the workaround to H3 ES1.x.
  

• 	 */
  

• 	if (soc_device_match(rcar_du_r8a7795_es1))
  

• 		target *= 2;
  

• 	rcar_du_dpll_divider(rcrtc, &dpll, extclk, target);
  extclk = dpll.output;
  

  }

@@ -197,8 +214,6 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) if (abs((long)extrate - (long)mode_clock) < abs((long)rate - (long)mode_clock)) {

• 	dev_dbg(rcrtc->group->dev->dev,
  

• 		"crtc%u: using external clock\n", rcrtc-
  

index);

...

	if (rcdu->info->dpll_ch & (1 << rcrtc->index)) {
		u32 dpllcr = DPLLCR_CODE | DPLLCR_CLKE

@@ -215,12 +230,14 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) rcar_du_group_write(rcrtc->group, DPLLCR, dpllcr);

• 		escr = ESCR_DCLKSEL_DCLKIN | 1;
  

• 	} else {
  

• 		escr = ESCR_DCLKSEL_DCLKIN | extdiv;
  }
  

• 	escr = ESCR_DCLKSEL_DCLKIN | extdiv;
  

Therefore - is this the post-divider?

Correct again. extdiv is computed as DPLL output frequency / pixel clock frequency, so there's no need for any SoC-specific code here.

If my inferences are correct - then OK:

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

...

}

• dev_dbg(rcrtc->group->dev->dev,
  

• 	"mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
  

• 	mode_clock, extrate, rate, escr);
  

  }

  rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,

Hi Kieran,

On Tuesday 01 Aug 2017 14:46:13 Kieran Bingham wrote:

On 26/06/17 19:12, Laurent Pinchart wrote:

...

On Gen3 SoCs DPAD0 routing is configured through the last CRTC group, unlike on Gen2 where it is configured through the first CRTC group. Fix the driver accordingly.

Fixes: 2427b3037710 ("drm: rcar-du: Add R8A7795 device support") Signed-off-by: Laurent Pinchart

laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/rcar-du/rcar_du_group.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c index 64738fca96d0..2abb2fdd143e 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c @@ -208,23 +208,30 @@ void rcar_du_group_restart(struct rcar_du_group *rgrp) int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu) {

• struct rcar_du_group *rgrp;

• struct rcar_du_crtc *crtc; int ret;

  if (!rcar_du_has(rcdu, RCAR_DU_FEATURE_EXT_CTRL_REGS)) return 0;

• /* RGB output routing to DPAD0 and VSP1D routing to DU0/1/2 are

• * configured in the DEFR8 register of the first group. As this
  

function

• * can be called with the DU0 and DU1 CRTCs disabled, we need to
  

enable

• * the first group clock before accessing the register.
  

• /*

• * RGB output routing to DPAD0 and VSP1D routing to DU0/1/2 are
  

• * configured in the DEFR8 register of the first group on Gen2 and the
  

• * last group on Gen3. As this function can be called with the DU
  

• * channels of the corresponding CRTCs disabled, we need to enable the
  

• * group clock before accessing the register.
  

  */

• ret = clk_prepare_enable(rcdu->crtcs[0].clock);

• rgrp = &rcdu->groups[DIV_ROUND_UP(rcdu->num_crtcs, 2) - 1];
• crtc = &rcdu->crtcs[rgrp->index * 2];

I'm not certain I understand how this makes a distinct difference between G2, and G3.

That's because, well, it doesn't :-)

Is rcdu->num_crtcs the distinguishing factor between the SoC's?

I'm not sure what I was thinking when I wrote this. I'll send a v3.

...

• ret = clk_prepare_enable(crtc->clock); if (ret < 0) return ret;

• rcar_du_group_setup_defr8(&rcdu->groups[0]);

• rcar_du_group_setup_defr8(rgrp);

• clk_disable_unprepare(rcdu->crtcs[0].clock);

• clk_disable_unprepare(crtc->clock);

  return 0;

}

Hi Laurent,

Thanks for the fast update.

On 01/08/17 18:20, Laurent Pinchart wrote:

On Gen3 SoCs DPAD0 routing is configured through the last CRTC group, unlike on Gen2 where it is configured through the first CRTC group. Fix the driver accordingly.

Fixes: 2427b3037710 ("drm: rcar-du: Add R8A7795 device support") Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

---

Changes since v2:

• Compute the group index correctly on Gen2

---

drivers/gpu/drm/rcar-du/rcar_du_group.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c index a3e29bb420fd..2f37ea901873 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c @@ -212,6 +212,9 @@ void rcar_du_group_restart(struct rcar_du_group *rgrp)

int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu) {

• struct rcar_du_group *rgrp;

• struct rcar_du_crtc *crtc;

• unsigned int index; int ret;

  if (!rcar_du_has(rcdu, RCAR_DU_FEATURE_EXT_CTRL_REGS))

@@ -219,17 +222,22 @@ int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu)

/* * RGB output routing to DPAD0 and VSP1D routing to DU0/1/2 are

• * configured in the DEFR8 register of the first group. As this function
  

• * can be called with the DU0 and DU1 CRTCs disabled, we need to enable
  

• * the first group clock before accessing the register.
  

• * configured in the DEFR8 register of the first group on Gen2 and the
  

• * last group on Gen3. As this function can be called with the DU
  

• * channels of the corresponding CRTCs disabled, we need to enable the
  

• * group clock before accessing the register.
  

  */

• ret = clk_prepare_enable(rcdu->crtcs[0].clock);

• index = rcdu->info->gen < 3 ? 0 : DIV_ROUND_UP(rcdu->num_crtcs, 2) - 1;

Ahh yes, that reads a lot better.

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

• rgrp = &rcdu->groups[index];
• crtc = &rcdu->crtcs[index * 2];
• ret = clk_prepare_enable(crtc->clock); if (ret < 0) return ret;

• rcar_du_group_setup_defr8(&rcdu->groups[0]);

• rcar_du_group_setup_defr8(rgrp);

• clk_disable_unprepare(rcdu->crtcs[0].clock);

• clk_disable_unprepare(crtc->clock);

  return 0;

}