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
