by Laurent Pinchart

[permalink] [raw]

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), 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/[email protected]/
>
> 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/[email protected]/
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/[email protected]/
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/[email protected]/
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/[email protected]/
>
> 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

2023-04-13 09:54:49

by Biju Das

[permalink] [raw]

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

Hi Laurent,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Wednesday, April 12, 2023 4:43 PM
> To: Biju Das <[email protected]>
> Cc: David Airlie <[email protected]>; Daniel Vetter <[email protected]>; Mauro
> Carvalho Chehab <[email protected]>; Rob Herring <[email protected]>;
> Krzysztof Kozlowski <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Geert Uytterhoeven <[email protected]>; Chris
> Paterson <[email protected]>; Prabhakar Mahadev Lad
> <[email protected]>; linux-renesas-
> [email protected]; Kieran Bingham <[email protected]>
> 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