RE: [PATCH v3 2/4] drm: renesas: rz-du: Add RZ/G2UL DU Support

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

 



Hi Laurent,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Tuesday, August 13, 2024 8:49 PM
> Subject: Re: [PATCH v3 2/4] drm: renesas: rz-du: Add RZ/G2UL DU Support
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> On Mon, Aug 05, 2024 at 04:52:36PM +0100, Biju Das wrote:
> > The LCD controller is composed of Frame Compression Processor (FCPVD),
> > Video Signal Processor (VSPD), and Display Unit (DU).
> >
> > It has DPI interface and supports a maximum resolution of WXGA along
> > with 2 RPFs to support the blending of two picture layers and raster
> > operations (ROPs).
> >
> > The DU module is connected to VSPD. Add RZ/G2UL DU support.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v2->v3:
> >  * Avoided the line break in rzg2l_du_start_stop() for rstate.
> >  * Replaced port->du_output in  struct rzg2l_du_output_routing and
> >    dropped using the port number to indicate the output type in
> >    rzg2l_du_encoders_init().
> >  * Updated rzg2l_du_r9a07g043u_info and rzg2l_du_r9a07g044_info
> > v1->v2:
> >  * No change.
> > ---
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c |  8 +++++++-
> > drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c  | 18 ++++++++++++++++--
> > drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h  |  5 +++--
> > drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c  |  4 ++--
> >  4 files changed, 28 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > index 6e7aac6219be..fd7675c7f181 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > @@ -28,6 +28,7 @@
> >  #include "rzg2l_du_vsp.h"
> >
> >  #define DU_MCR0			0x00
> > +#define DU_MCR0_DPI_OE		BIT(0)
> >  #define DU_MCR0_DI_EN		BIT(8)
> >
> >  #define DU_DITR0		0x10
> > @@ -216,9 +217,14 @@ static void rzg2l_du_crtc_put(struct
> > rzg2l_du_crtc *rcrtc)
> >
> >  static void rzg2l_du_start_stop(struct rzg2l_du_crtc *rcrtc, bool
> > start)  {
> > +	struct rzg2l_du_crtc_state *rstate =
> > +to_rzg2l_crtc_state(rcrtc->crtc.state);
> >  	struct rzg2l_du_device *rcdu = rcrtc->dev;
> > +	u32 val = DU_MCR0_DI_EN;
> >
> > -	writel(start ? DU_MCR0_DI_EN : 0, rcdu->mmio + DU_MCR0);
> > +	if (rstate->outputs == BIT(RZG2L_DU_OUTPUT_DPAD0))
> 
> Is output supposed to contain a single bit, or can it contain multiple bits ? In the first case I
> would rename it to output, in the second case you should probably use '&' instead of '=='.

It may contain multiple bits for the future SoCs. So, I will use '&' instead.

> 
> > +		val |= DU_MCR0_DPI_OE;
> > +
> > +	writel(start ? val : 0, rcdu->mmio + DU_MCR0);
> >  }
> >
> >  static void rzg2l_du_crtc_start(struct rzg2l_du_crtc *rcrtc) diff
> > --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> > index e5eca8691a33..69b8e216ee1a 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> > @@ -25,21 +25,35 @@
> >   * Device Information
> >   */
> >
> > +static const struct rzg2l_du_device_info rzg2l_du_r9a07g043u_info = {
> > +	.channels_mask = BIT(0),
> > +	.routes = {
> > +		[RZG2L_DU_OUTPUT_DSI0] = {
> > +			.du_output = RZG2L_DU_OUTPUT_INVALID,
> > +		},
> 
> You can drop this entry, as well as the RZG2L_DU_OUTPUT_INVALID macro.
> See below.
OK.
> 
> > +		[RZG2L_DU_OUTPUT_DPAD0] = {
> > +			.possible_outputs = BIT(0),
> > +			.du_output = RZG2L_DU_OUTPUT_DPAD0,
> > +		},
> > +	},
> > +};
> > +
> >  static const struct rzg2l_du_device_info rzg2l_du_r9a07g044_info = {
> >  	.channels_mask = BIT(0),
> >  	.routes = {
> >  		[RZG2L_DU_OUTPUT_DSI0] = {
> >  			.possible_outputs = BIT(0),
> > -			.port = 0,
> > +			.du_output = RZG2L_DU_OUTPUT_DSI0,
> >  		},
> >  		[RZG2L_DU_OUTPUT_DPAD0] = {
> >  			.possible_outputs = BIT(0),
> > -			.port = 1,
> > +			.du_output = RZG2L_DU_OUTPUT_DPAD0,
> >  		}
> >  	}
> >  };
> >
> >  static const struct of_device_id rzg2l_du_of_table[] = {
> > +	{ .compatible = "renesas,r9a07g043u-du", .data =
> > +&rzg2l_du_r9a07g043u_info },
> >  	{ .compatible = "renesas,r9a07g044-du", .data = &rzg2l_du_r9a07g044_info },
> >  	{ /* sentinel */ }
> >  };
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> > index 58806c2a8f2b..ab82b5c86d6e 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> > @@ -29,7 +29,7 @@ enum rzg2l_du_output {
> >  /*
> >   * struct rzg2l_du_output_routing - Output routing specification
> >   * @possible_outputs: bitmask of possible outputs
> > - * @port: device tree port number corresponding to this output route
> > + * @du_output: DU output
> >   *
> >   * The DU has 2 possible outputs (DPAD0, DSI0). Output routing data
> >   * specify the valid SoC outputs, which CRTC can drive the output,
> > and the type @@ -37,7 +37,7 @@ enum rzg2l_du_output {
> >   */
> >  struct rzg2l_du_output_routing {
> >  	unsigned int possible_outputs;
> > -	unsigned int port;
> > +	unsigned int du_output;
> >  };
> >
> >  /*
> > @@ -53,6 +53,7 @@ struct rzg2l_du_device_info {
> >  #define RZG2L_DU_MAX_CRTCS		1
> >  #define RZG2L_DU_MAX_VSPS		1
> >  #define RZG2L_DU_MAX_DSI		1
> > +#define RZG2L_DU_OUTPUT_INVALID		-1
> >
> >  struct rzg2l_du_device {
> >  	struct device *dev;
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> > b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> > index 07b312b6f81e..361350f2999e 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> > @@ -183,8 +183,8 @@ static int rzg2l_du_encoders_init(struct
> > rzg2l_du_device *rcdu)
> >
> >  		/* Find the output route corresponding to the port number. */
> >  		for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
> > -			if (rcdu->info->routes[i].port == ep.port) {
> > -				output = i;
> > +			if (i == rcdu->info->routes[i].du_output) {
> 
> If I understand the code correctly, this will always be true except for the routes marked with
> RZG2L_DU_OUTPUT_INVALID, so you will match the first valid route, regardless of the value of ep.port.
> I don't think that's correct.
> 
> I would keep the port field in the rzg2l_du_output_routing, drop the newly added du_output field, and
> use the following logic:
> 
> 		for (i = 0; i < RZG2L_DU_OUTPUT_MAX; ++i) {
> 			if (rcdu->info->routes[i].possible_outputs &&
> 			    rcdu->info->routes[i].port == ep.port) {
> 				output = i;
> 				break;
> 			}
> 		}
> 
> Testing possible_outputs skips the routes that don't exist for the device, and the ep.port comparison
> picks the route corresponding to the port.

Thanks, this will work.

Cheers,
Biju

> 
> > +				output = rcdu->info->routes[i].du_output;
> >  				break;
> >  			}
> >  		}
> 
> --
> Regards,
> 
> Laurent Pinchart




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux