Re: [PATCH 2/5] drm: rcar-du: Move CRTC outputs bitmask to private CRTC state

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Hi Laurent,

Thank you for the patch,

On 25/11/2018 14:40, Laurent Pinchart wrote:
> The rcar_du_crtc outputs field stores a bitmask of the outputs driven by
> the CRTC. This changes based on the configuration requested by
> userspace, and is used for the sole purpose of configuring the hardware.
> The field thus belongs to the CRTC state. Move it to the
> rcar_du_crtc_state structure.
> 
> As a result the rcar_du_crtc_route_output() function loses most of its
> purpose. In order to remove it, move dpad0_source calculation to
> rcar_du_atomic_commit_tail(), until the field gets moved to a state
> structure. In order to simplify the rcar_du_group_set_routing()
> implementation, we also store the DPAD1 source in a new dpad1_source
> field which will move to a state structure with dpad0_source.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

that was a fairly tough read - but aside from one comment near the
bottom regarding initialising dpad0 which I'm sure you can handle
correctly, and another comment which I think we could improve things
outside of this patch:

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


> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c    | 42 +++++++++++------------
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h    |  7 ++--
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h     |  1 +
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 10 ------
>  drivers/gpu/drm/rcar-du/rcar_du_group.c   |  4 +--
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c     | 22 ++++++++++++
>  6 files changed, 47 insertions(+), 39 deletions(-)

+8 ... It's a good job you bought -13 lines in the previous patch ;)



> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 90dacab67be5..40b7f17159b0 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -22,6 +22,7 @@
>  
>  #include "rcar_du_crtc.h"
>  #include "rcar_du_drv.h"
> +#include "rcar_du_encoder.h"
>  #include "rcar_du_kms.h"
>  #include "rcar_du_plane.h"
>  #include "rcar_du_regs.h"
> @@ -316,26 +317,6 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>  	rcar_du_crtc_write(rcrtc, DEWR,  mode->hdisplay);
>  }
>  
> -void rcar_du_crtc_route_output(struct drm_crtc *crtc,
> -			       enum rcar_du_output output)
> -{
> -	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> -	struct rcar_du_device *rcdu = rcrtc->group->dev;
> -
> -	/*
> -	 * Store the route from the CRTC output to the DU output. The DU will be
> -	 * configured when starting the CRTC.
> -	 */
> -	rcrtc->outputs |= BIT(output);
> -
> -	/*
> -	 * Store RGB routing to DPAD0, the hardware will be configured when
> -	 * starting the CRTC.
> -	 */
> -	if (output == RCAR_DU_OUTPUT_DPAD0)
> -		rcdu->dpad0_source = rcrtc->index;
> -}
> -
>  static unsigned int plane_zpos(struct rcar_du_plane *plane)
>  {
>  	return plane->plane.state->normalized_zpos;
> @@ -655,6 +636,24 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>   * CRTC Functions
>   */
>  
> +static int rcar_du_crtc_atomic_check(struct drm_crtc *crtc,
> +				     struct drm_crtc_state *state)
> +{
> +	struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(state);
> +	struct drm_encoder *encoder;
> +
> +	/* Store the routes from the CRTC output to the DU outputs. */
> +	rstate->outputs = 0;
> +
> +	drm_for_each_encoder_mask(encoder, crtc->dev, state->encoder_mask) {
> +		struct rcar_du_encoder *renc = to_rcar_encoder(encoder);
> +
> +		rstate->outputs |= BIT(renc->output);
> +	}
> +
> +	return 0;
> +}
> +
>  static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>  				       struct drm_crtc_state *old_state)
>  {
> @@ -678,8 +677,6 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
>  		crtc->state->event = NULL;
>  	}
>  	spin_unlock_irq(&crtc->dev->event_lock);
> -
> -	rcrtc->outputs = 0;
>  }
>  
>  static void rcar_du_crtc_atomic_begin(struct drm_crtc *crtc,
> @@ -755,6 +752,7 @@ enum drm_mode_status rcar_du_crtc_mode_valid(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs crtc_helper_funcs = {
> +	.atomic_check = rcar_du_crtc_atomic_check,
>  	.atomic_begin = rcar_du_crtc_atomic_begin,
>  	.atomic_flush = rcar_du_crtc_atomic_flush,
>  	.atomic_enable = rcar_du_crtc_atomic_enable,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 59ac6e7d22c9..ec47f164e69b 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -37,7 +37,6 @@ struct rcar_du_vsp;
>   * @vblank_lock: protects vblank_wait and vblank_count
>   * @vblank_wait: wait queue used to signal vertical blanking
>   * @vblank_count: number of vertical blanking interrupts to wait for
> - * @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
> @@ -61,8 +60,6 @@ struct rcar_du_crtc {
>  	wait_queue_head_t vblank_wait;
>  	unsigned int vblank_count;
>  
> -	unsigned int outputs;
> -
>  	struct rcar_du_group *group;
>  	struct rcar_du_vsp *vsp;
>  	unsigned int vsp_pipe;
> @@ -77,11 +74,13 @@ struct rcar_du_crtc {
>   * struct rcar_du_crtc_state - Driver-specific CRTC state
>   * @state: base DRM CRTC state
>   * @crc: CRC computation configuration
> + * @outputs: bitmask of the outputs (enum rcar_du_output) driven by this CRTC
>   */
>  struct rcar_du_crtc_state {
>  	struct drm_crtc_state state;
>  
>  	struct vsp1_du_crc_config crc;
> +	unsigned int outputs;
>  };
>  
>  #define to_rcar_crtc_state(s) container_of(s, struct rcar_du_crtc_state, state)
> @@ -102,8 +101,6 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
>  void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc);
>  void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc);
>  
> -void rcar_du_crtc_route_output(struct drm_crtc *crtc,
> -			       enum rcar_du_output output);
>  void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc);
>  
>  void rcar_du_crtc_dsysr_clr_set(struct rcar_du_crtc *rcrtc, u32 clr, u32 set);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index 8ef9165957cb..8b47b8546fc8 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -90,6 +90,7 @@ struct rcar_du_device {
>  	} props;
>  
>  	unsigned int dpad0_source;
> +	unsigned int dpad1_source;
>  	unsigned int vspd1_sink;
>  };
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> index 1877764bd6d9..a123c28ea6ed 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> @@ -22,17 +22,7 @@
>   * Encoder
>   */
>  
> -static void rcar_du_encoder_mode_set(struct drm_encoder *encoder,
> -				     struct drm_crtc_state *crtc_state,
> -				     struct drm_connector_state *conn_state)
> -{
> -	struct rcar_du_encoder *renc = to_rcar_encoder(encoder);
> -
> -	rcar_du_crtc_route_output(crtc_state->crtc, renc->output);
> -}
> -
>  static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> -	.atomic_mode_set = rcar_du_encoder_mode_set,
>  };
>  
>  static const struct drm_encoder_funcs encoder_funcs = {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 3366cda6086c..7e440f61977f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -289,7 +289,7 @@ int rcar_du_set_dpad0_vsp1_routing(struct rcar_du_device *rcdu)
>  
>  int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
>  {
> -	struct rcar_du_crtc *crtc0 = &rgrp->dev->crtcs[rgrp->index * 2];
> +	struct rcar_du_device *rcdu = rgrp->dev;
>  	u32 dorcr = rcar_du_group_read(rgrp, DORCR);
>  
>  	dorcr &= ~(DORCR_PG2T | DORCR_DK2S | DORCR_PG2D_MASK);
> @@ -299,7 +299,7 @@ int rcar_du_group_set_routing(struct rcar_du_group *rgrp)
>  	 * CRTC 1 in all other cases to avoid cloning CRTC 0 to DPAD0 and DPAD1
>  	 * by default.
>  	 */
> -	if (crtc0->outputs & BIT(RCAR_DU_OUTPUT_DPAD1))
> +	if (rcdu->dpad1_source == rgrp->index * 2)

The magic of getting (rgrp->index * 2) is a little bit ... magic ...

The what is happening ( ({0,1} => {0,2}) ) is clear - but not the why.

This happens a bit throughout as a means to convert from CRTC to Group
indexes, and back. But it could be clearer with a helper.
	(not in this patch)


>  		dorcr |= DORCR_PG2D_DS1;
>  	else
>  		dorcr |= DORCR_PG2T | DORCR_DK2S | DORCR_PG2D_DS2;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index fe6f65c94eef..bd3c2c5ea66f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -285,6 +285,28 @@ static int rcar_du_atomic_check(struct drm_device *dev,
>  static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  {
>  	struct drm_device *dev = old_state->dev;
> +	struct rcar_du_device *rcdu = dev->dev_private;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	unsigned int i;
> +
> +	/*
> +	 * Store RGB routing to DPAD0 and DPAD1, the hardware will be configured
> +	 * when starting the CRTCs.
> +	 */
> +	rcdu->dpad1_source = -1;

Why do we initialise dpad1_source but not dpad0_source?

Are we *guaranteed* to always have RCAR_DU_OUTPUT_DPAD0 set in one of
the rcrtc_state->outputs ?

If so - then this isn't an issue.


> +
> +	for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) {
> +		struct rcar_du_crtc_state *rcrtc_state =
> +			to_rcar_crtc_state(crtc_state);
> +		struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +
> +		if (rcrtc_state->outputs & BIT(RCAR_DU_OUTPUT_DPAD0))
> +			rcdu->dpad0_source = rcrtc->index;
> +
> +		if (rcrtc_state->outputs & BIT(RCAR_DU_OUTPUT_DPAD1))
> +			rcdu->dpad1_source = rcrtc->index;
> +	}
>  
>  	/* Apply the atomic update. */
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> 

-- 
Regards
--
Kieran
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel