RE: [PATCH v7 00/17] Add RCar DU lib support

From: Biju Das
Date: Thu Apr 13 2023 - 05:51:06 EST


Hi Laurent,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Wednesday, April 12, 2023 4:43 PM
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; Mauro
> Carvalho Chehab <mchehab@xxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> media@xxxxxxxxxxxxxxx; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>; Chris
> Paterson <Chris.Paterson2@xxxxxxxxxxx>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>; linux-renesas-
> soc@xxxxxxxxxxxxxxx; Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v7 00/17] Add RCar DU lib support
>
> Hi Biju,
>
> (CC'ing Kieran who has missed the series so far)
>
> Thank you for the patch.
>
> On Tue, Apr 11, 2023 at 12:42:18PM +0100, Biju Das wrote:
> > The DU controller on RZ/G2L LCDC is similar to R-Car as it is
> > connected to VSPD. RCar DU lib is created for sharing kms, vsp and
> > encoder driver code between both RCar and RZ/G2L alike SoCs.
>
> I'm afraid that my opinion hasn't changed much compared to the previous
> versions :-(
>
> The RZ/G2L LCD Controller (LCDC) is indeed made of FCP, VSP and DU hardware
> blocks, like in R-Car. While the VSP is similar to its R-Car counterpart,
> and the FCP may be as well (I haven't checked),

Both RCar and RZ/G2L have same FCPV registers and same functionality
--------------------------------------------------------------------
VSP for blending and display output

FCP version control register FCP_VCR
FCPV configuration register FCP_CFG0
FCP reset register FCP_RST
FCP status register FCP_STA

and VSPD for RCar and RZ/G2L has similar functionality and similar register layout.

RCar VSPD:
---------
• Supports various data formats and conversion
— Supports YCbCr444/422/420, RGB, α RGB, α plane.
— Color space conversion and changes to the number of colors by dithering
— Color keying
— Supports combination between pixel alpha and global alpha.
— Supports generating pre multiplied alpha.
• Video processing
— Blending of five picture layers and raster operations (ROPs)
— Vertical flipping in case of output to memory.
• Direct connection to display module
— Supporting 4096 pixels in horizontal direction [R-Car H3/R-Car H3-N/R-Car M3-W/R-Car M3-W+/ R-Car M3-
N]
— Supporting 2048 pixels in horizontal direction [R-Car V3M/R-Car V3H/R-Car V3H_2/R-Car D3/R-Car E3]
— Writing back image data which is transferred to Display Unit (DU) to memory.

RZ/G2L VSPD
-----------
− Supports various data formats and conversion
− Supports YCbCr444/422/420, RGB, α RGB, α plane
− Color space conversion and changes to the number of colors by dithering
− Color keying
− Supports combination between pixel alpha and global alpha
− Supports generating pre multiplied alpha
− Video processing
− Blending of two picture layers and raster operations (ROPs)
− Clipping
− 1D look up table
− Vertical flipping in case of output to memory
− Direct connection to display module
− Supporting 1920 pixels in horizontal direction
− Writing back image data which is transferred to Display Unit (DU) to memory

So all media drivers, we can use and it is already upstreamed by[1] and pending binding patches[2]
for fcpv.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/media/platform/renesas/vsp1/vsp1_drv.c?h=next-20230412&id=882bda188f691320a001c6adc738c4a7ec102a8d

[2] https://patchwork.linuxtv.org/project/linux-media/list/?submitter=8264


the only common point
> between the RZ/G2L and R-Car DU is the name.

I agree DU hardware is different apart from connection to VSPD.

>
> This patch series is turning the R-Car DU driver into a generic library to
> support the unrelated RZ/G2L DU. This makes the code more complex, and
> significantly more difficult to maintain. Not only would changes for R-Car
> then need to be tested on RZ/G2L as well (which is a platform I don't have
> access to), but refactoring of the code to address R-Car use cases may
> become more difficult due to RZ/G2L support.

OK, if that is the case, Maybe we should create rzg2l_du folder and
add support for DU driver by linking with VSP driver.

In this way, RCar DU is separate, and you can address more R-Car use cases
and any changes on RCar Du won't affect RZ/G2L DU and vice versa
as both are separate.


>
> The only hardware-specific similarity I see between the RZ/G2L and R-Car DU
> is usage of the VSP as an external composer. That part is mostly handled by
> the VSP driver which is already an external component to the DU driver.
> There is a thin glue layer in the DU driver to translate the KMS plane API
> to the VSP internal API, and some code may be reused on the RZ/G2L, but I
> expect that to be fairly limited, especially given that the interface with
> the VSP isn't exactly the same on the two platforms. Still, *if* that code
> could be nicely split to a library shared by the two platforms, *without*
> causing more pain (significant maintenance burden) than gain (code sharing),
> I would be fine with that.

Creating separate rzg2-du folder will address these issues. It won't interfere
with RCar-DU as we are making totally different drivers as DU hardware is
different.

Cheers,
Biju

>
> What I don't like is how intrusive the patch series is. You're turning the
> whole DU driver into a library, for parts where the two platforms have no
> common hardware implementation. If there are significant portions of the DU
> driver that would be duplicated for RZ/G2L, it may be a sign that that code
> could be factored out to a library, but it should in that case not be a DU
> library, but a DRM core helper.
>
> The DRM core helpers and the VSP helpers are two independent components, so
> I would be fine if you decide to only implement one of the two.
>
> > Tested this patch series on RZ/{G2M, G2L, G2LC} and RZ/V2L platforms.
> >
> > v6->v7:
> > * Split DU lib and RZ/G2L du driver as separate patch series as
> > DU support added to more platforms based on RZ/G2L alike SoCs.
> > * Rebased to latest drm-tip.
> > v5->v6:
> > * Merged DU lib and RZ/G2L du driver in same patch series
> > * Rebased to latest drm-misc.
> > * Merged patch#1 to RZ/G2L Driver patch.
> > * Updated KConfig dependency from ARCH_RENESAS->ARCH_RZG2L.
> > * Optimized rzg2l_du_output_name() by removing unsupported outputs.
> >
> > v4->v5:
> > * Added Rb tag from Rob for binding patch.
> > * Started using RCar DU libs(kms, vsp and encoder)
> > * Started using rcar_du_device, rcar_du_write, rcar_du_crtc,
> > rcar_du_format_info and rcar_du_encoder.
> > v3->v4:
> > * Changed compatible name from
> > renesas,du-r9a07g044->renesas,r9a07g044-du
> > * started using same compatible for RZ/G2{L,LC}
> > * Removed rzg2l_du_group.h and struct rzg2l_du_group
> > * Renamed __rzg2l_du_group_start_stop->rzg2l_du_start_stop
> > * Removed rzg2l_du_group_restart
> > * Updated rzg2l_du_crtc_set_display_timing
> > * Removed mode_valid callback.
> > * Updated rzg2l_du_crtc_create() parameters
> > * Updated compatible
> > * Removed RZG2L_DU_MAX_GROUPS
> > V2->v3:
> > * Added new bindings for RZ/G2L DU
> > * Removed indirection and created new DRM driver based on R-Car DU
> > v1->v2:
> > * Based on [1], all references to 'rzg2l_lcdc' replaced with 'rzg2l_du'
> > * Updated commit description for bindings
> > * Removed LCDC references from bindings
> > * Changed clock name from du.0->aclk from bindings
> > * Changed reset name from du.0->du from bindings
> > * Replaced crtc_helper_funcs->rcar_crtc_helper_funcs
> > * Updated macro DRM_RZG2L_LCDC->DRM_RZG2L_DU
> > * Replaced rzg2l-lcdc-drm->rzg2l-du-drm
> > * Added forward declaration for struct reset_control
> >
> > [1]
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-renesas-soc%2Fpatch%2F2022031208420
> > 5.31462-2-biju.das.jz%40bp.renesas.com%2F&data=05%7C01%7Cbiju.das.jz%4
> > 0bp.renesas.com%7C7976707c06404ddaab5f08db3b6ca35a%7C53d82571da1947e49
> > cb4625a166a4a2a%7C0%7C0%7C638169110007214274%7CUnknown%7CTWFpbGZsb3d8e
> > yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C30
> > 00%7C%7C%7C&sdata=hJHaJVrDnkAOMV3XFRn2QMonzguldagfeMCCWaUELU8%3D&reser
> > ved=0
> >
> > RFC->v1:
> > * Changed minItems->maxItems for renesas,vsps.
> > * Added RZ/G2L LCDC driver with special handling for CRTC reusing
> > most of RCar DU code
> > * Fixed the comments for num_rpf from rpf's->RPFs/ and vsp->VSP.
> > RFC:
> >
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-renesas-soc%2Fpatch%2F2022011217461
> > 2.10773-18-biju.das.jz%40bp.renesas.com%2F&data=05%7C01%7Cbiju.das.jz%
> > 40bp.renesas.com%7C7976707c06404ddaab5f08db3b6ca35a%7C53d82571da1947e4
> > 9cb4625a166a4a2a%7C0%7C0%7C638169110007214274%7CUnknown%7CTWFpbGZsb3d8
> > eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3
> > 000%7C%7C%7C&sdata=AVICqj6u9WRI88ImS7PZDuAo8qalzzuEK%2Fo4kwQq27c%3D&re
> > served=0
> >
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-renesas-soc%2Fpatch%2F2022011217461
> > 2.10773-12-biju.das.jz%40bp.renesas.com%2F&data=05%7C01%7Cbiju.das.jz%
> > 40bp.renesas.com%7C7976707c06404ddaab5f08db3b6ca35a%7C53d82571da1947e4
> > 9cb4625a166a4a2a%7C0%7C0%7C638169110007214274%7CUnknown%7CTWFpbGZsb3d8
> > eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3
> > 000%7C%7C%7C&sdata=OOlSmShGKbXZclgHA5oawcHm3W0EwX5tw9PvFmyFlzc%3D&rese
> > rved=0
> >
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-renesas-soc%2Fpatch%2F2022011217461
> > 2.10773-13-biju.das.jz%40bp.renesas.com%2F&data=05%7C01%7Cbiju.das.jz%
> > 40bp.renesas.com%7C7976707c06404ddaab5f08db3b6ca35a%7C53d82571da1947e4
> > 9cb4625a166a4a2a%7C0%7C0%7C638169110007214274%7CUnknown%7CTWFpbGZsb3d8
> > eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3
> > 000%7C%7C%7C&sdata=t9SAsFWzMTRvblRmj6QzvKXZIsZFWleOVceQJGlSg6E%3D&rese
> > rved=0
> >
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-renesas-soc%2Fpatch%2F2022011217461
> > 2.10773-19-biju.das.jz%40bp.renesas.com%2F&data=05%7C01%7Cbiju.das.jz%
> > 40bp.renesas.com%7C7976707c06404ddaab5f08db3b6ca35a%7C53d82571da1947e4
> > 9cb4625a166a4a2a%7C0%7C0%7C638169110007214274%7CUnknown%7CTWFpbGZsb3d8
> > eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3
> > 000%7C%7C%7C&sdata=yMbFsZYJ6soLJmGAzqNDmwfEI4nyZARzX6VhjyUh4WU%3D&rese
> > rved=0
> >
> > Biju Das (17):
> > drm: rcar-du: Add encoder lib support
> > drm: rcar-du: Add kms lib support
> > drm: rcar-du: Add vsp lib support
> > drm: rcar-du: Move rcar_du_vsp_atomic_begin()
> > drm: rcar-du: Move rcar_du_vsp_atomic_flush()
> > drm: rcar-du: Move rcar_du_vsp_{map,unmap}_fb()
> > drm: rcar-du: Move rcar_du_dumb_create()
> > drm: rcar-du: Move rcar_du_gem_prime_import_sg_table()
> > drm: rcar-du: Add rcar_du_lib_vsp_init()
> > drm: rcar-du: Move rcar_du_vsp_plane_prepare_fb()
> > drm: rcar-du: Move rcar_du_vsp_plane_cleanup_fb()
> > drm: rcar-du: Move rcar_du_vsp_plane_atomic_update()
> > drm: rcar-du: Add rcar_du_lib_fb_create()
> > drm: rcar-du: Add rcar_du_lib_mode_cfg_helper_get()
> > drm: rcar-du: Move rcar_du_encoders_init()
> > drm: rcar-du: Move rcar_du_properties_init()
> > drm: rcar-du: Add rcar_du_lib_vsps_init()
> >
> > drivers/gpu/drm/rcar-du/Kconfig | 10 +
> > drivers/gpu/drm/rcar-du/Makefile | 4 +
> > drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 117 +--
> > drivers/gpu/drm/rcar-du/rcar_du_encoder.h | 14 +-
> > drivers/gpu/drm/rcar-du/rcar_du_encoder_lib.c | 138 ++++
> > drivers/gpu/drm/rcar-du/rcar_du_encoder_lib.h | 30 +
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 694 +---------------
> > drivers/gpu/drm/rcar-du/rcar_du_kms.h | 29 +-
> > drivers/gpu/drm/rcar-du/rcar_du_kms_lib.c | 744 ++++++++++++++++++
> > drivers/gpu/drm/rcar-du/rcar_du_kms_lib.h | 61 ++
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 407 +---------
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 26 +-
> > drivers/gpu/drm/rcar-du/rcar_du_vsp_lib.c | 436 ++++++++++
> > drivers/gpu/drm/rcar-du/rcar_du_vsp_lib.h | 76 ++
> > 14 files changed, 1515 insertions(+), 1271 deletions(-) create mode
> > 100644 drivers/gpu/drm/rcar-du/rcar_du_encoder_lib.c
> > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_encoder_lib.h
> > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_kms_lib.c
> > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_kms_lib.h
> > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_vsp_lib.c
> > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_vsp_lib.h
>
> --
> Regards,
>
> Laurent Pinchart