Message-ID: <OS0PR01MB5922717E4CCFE07F3C25FBC986989@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Date:   Thu, 13 Apr 2023 09:50:53 +0000
From:   Biju Das <biju.das.jz@...renesas.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC:     David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Chris Paterson <Chris.Paterson2@...esas.com>,
        Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        Kieran Bingham <kieran.bingham@...asonboard.com>,
        Fabrizio Castro <fabrizio.castro.jz@...esas.com>
Subject: RE: [PATCH v7 00/17] Add RCar DU lib support


Hi Laurent,

Thanks for the feedback.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Sent: Wednesday, April 12, 2023 4:43 PM
> To: Biju Das <biju.das.jz@...renesas.com>
> Cc: David Airlie <airlied@...il.com>; Daniel Vetter <daniel@...ll.ch>; Mauro
> Carvalho Chehab <mchehab@...nel.org>; Rob Herring <robh+dt@...nel.org>;
> Krzysztof Kozlowski <krzk+dt@...nel.org>; dri-devel@...ts.freedesktop.org;
> linux-kernel@...r.kernel.org; devicetree@...r.kernel.org; linux-
> media@...r.kernel.org; Geert Uytterhoeven <geert+renesas@...der.be>; Chris
> Paterson <Chris.Paterson2@...esas.com>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@...renesas.com>; linux-renesas-
> soc@...r.kernel.org; Kieran Bingham <kieran.bingham@...asonboard.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

Powered by blists - more mailing lists