Message ID | 20230411114235.366042-1-biju.das.jz@bp.renesas.com |
---|---|
Headers | show |
Series | Add RCar DU lib support | expand |
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), the only common point between the RZ/G2L and R-Car DU is the name. 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. 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. 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://patchwork.kernel.org/project/linux-renesas-soc/patch/20220312084205.31462-2-biju.das.jz@bp.renesas.com/ > > 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://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-18-biju.das.jz@bp.renesas.com/ > https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-12-biju.das.jz@bp.renesas.com/ > https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-13-biju.das.jz@bp.renesas.com/ > https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-19-biju.das.jz@bp.renesas.com/ > > 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
Hi Laurent, Thanks for the feedback. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: Wednesday, April 12, 2023 4:43 PM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: David Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Mauro > Carvalho Chehab <mchehab@kernel.org>; Rob Herring <robh+dt@kernel.org>; > Krzysztof Kozlowski <krzk+dt@kernel.org>; dri-devel@lists.freedesktop.org; > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > media@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>; Chris > Paterson <Chris.Paterson2@renesas.com>; Prabhakar Mahadev Lad > <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas- > soc@vger.kernel.org; Kieran Bingham <kieran.bingham@ideasonboard.com> > 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