On Thu, 2022-06-30 at 22:47 +0200, Marek Vasut wrote:
> On 6/30/22 10:30, Liu Ying wrote:
> > Hi Marek,
>
> Hi,
>
> > > drivers/gpu/drm/Makefile | 2 +-
> > > drivers/gpu/drm/mxsfb/Kconfig | 16 +
> > > drivers/gpu/drm/mxsfb/Makefile | 2 +
> > > drivers/gpu/drm/mxsfb/lcdif_drv.c | 341 ++++++++++++++++++++
> > > drivers/gpu/drm/mxsfb/lcdif_drv.h | 44 +++
> > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 483 +++++++++++++++++++++++++++++
> > > drivers/gpu/drm/mxsfb/lcdif_regs.h | 257 +++++++++++++++
> >
> > The mxsfb-drm driver is in the mxsfb directory as a successor of the
> > legacy mxsfb fbdev driver. The fbdev driver is for i.MX23/28 lcdif.
>
> That mxsfb-drm is also used on iMX8M{,M,N}
Right. But i.MX23/28/8MM/8MN LCDIFs are basically the same. i.MX8MP
LCDIFv3 is a completely different display controller. That's why
'fsl,imx8mp-lcdif' is not added in mxsfb_dt_ids[].
>
> > So, in order to avoid confusion, maybe don't put the new lcdifv3 driver
> > here. I would choose to create a new sub-directory in
> > drivers/gpu/drm/imx and put it there. The full path can be
> > drivers/gpu/drm/imx/lcdifv3, which matches the IP name(as called by
> > design team).
>
> It also matches the NXP downstream vendor kernel paths.
Yes. I don't see any particular issue with that path.
>
> > If people don't like it because i.MX23 lcdif version
> > register reads major version3, drivers/gpu/drm/imx/imx8mp-lcdif
>
> It cannot, because this IP is also used in iMXRT.
IMHO, 'first SoC name + IP name' clearly tells a IP without confusion.
It can be used as the directory name. Not sure if i.MXRT or i.MX8mp
comes first from design PoV. I think 'i.MXRT SoC name + lcdif' is also
okay. But, i.MX8MP LCDIFv3 will be the first LCDIFv3 supported in
Linux, so it seems 'imx8mp-lcdif' is fine.
>
> > can be
> > an alternative, though longer directory name. I tend to use lcdifv3
> > since separate directory(imx vs mxsfb) hints totally different display
> > controllers.
>
> We've had this discussion about naming/placement before.
Yes, in v1 review cycle, AFAICS. There, people did have comments on the
directory to put and too generic namings. 'Lets postpone this
discussion until the technical bits are settled.' was mentioned.
And, at this time point, I don't think it's sufficiently discussed
yet.
>
> Placing the lcdif driver into mxsfb would allow code deduplication
> between the two drivers, that's why it is in mxsfb directory.
Well, again, i.MX8MP LCDIFv3 is a completely different display
controller... Boilerplate code for things like drm dev registeration
is kinda trivial.
>
> > > 7 files changed, 1144 insertions(+), 1 deletion(-)
> > > create mode 100644 drivers/gpu/drm/mxsfb/lcdif_drv.c
> > > create mode 100644 drivers/gpu/drm/mxsfb/lcdif_drv.h
> > > create mode 100644 drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > create mode 100644 drivers/gpu/drm/mxsfb/lcdif_regs.h
> > >
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 13ef240b3d2b2..75b5ac7c2663c 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -130,7 +130,7 @@ obj-y += bridge/
> > > obj-$(CONFIG_DRM_FSL_DCU) += fsl-dcu/
> > > obj-$(CONFIG_DRM_ETNAVIV) += etnaviv/
> > > obj-y += hisilicon/
> > > -obj-$(CONFIG_DRM_MXSFB) += mxsfb/
> > > +obj-y += mxsfb/
> > > obj-y += tiny/
> > > obj-$(CONFIG_DRM_PL111) += pl111/
> > > obj-$(CONFIG_DRM_TVE200) += tve200/
> > > diff --git a/drivers/gpu/drm/mxsfb/Kconfig b/drivers/gpu/drm/mxsfb/Kconfig
> > > index 987170e16ebd6..873551b4552f5 100644
> > > --- a/drivers/gpu/drm/mxsfb/Kconfig
> > > +++ b/drivers/gpu/drm/mxsfb/Kconfig
> > > @@ -19,3 +19,19 @@ config DRM_MXSFB
> > > i.MX28, i.MX6SX, i.MX7 and i.MX8M).
> > >
> > > If M is selected the module will be called mxsfb.
> > > +
> > > +config DRM_IMX_LCDIF
> >
> > Perhaps, choose a less generic name here in case yet another new IP
> > with similar name in future... Use DRM_IMX_LCDIFV3?
> >
> > > + tristate "i.MX LCDIFv3 LCD controller"
> > > + depends on DRM && OF
> > > + depends on COMMON_CLK
> > > + select DRM_MXS
> > > + select DRM_KMS_HELPER
> > > + select DRM_GEM_CMA_HELPER
> > > + select DRM_PANEL
> > > + select DRM_PANEL_BRIDGE
> > > + help
> > > + Choose this option if you have an LCDIFv3 LCD controller.
> > > + Those devices are found in various i.MX SoC (i.MX8MP,
> > > + i.MXRT).
> > > +
> > > + If M is selected the module will be called imx-lcdif.
> >
> > Same here. Use imx-lcdifv3?
>
> The mxsfb LCDIF also supports multiple versions of the LCDIF IP (v3, v4,
> v6 at least). So calling a driver LCDIF v3 is about as confusing, since
> you cannot tell whether it is the iMX23 LCDIF v3 or the iMXRT/iMX8MP
> LCDIFv3 .
IMHO, if this driver is in drivers/gpu/drm/imx/lcdifv3, then people
tend to think it's a completely display controller(not related to iMX23
LCDIF v3). So, from code PoV, confusion is not that considerable. From
user PoV, imx-lcdifv3.ko could be confusing, not sure. Maybe, use
imx8mp-lcdif or imxrtXXX-lcdif?
>
> > Similar comment applies to the entire driver - less generic name.
>
> [...]
>
> > > +struct lcdif_drm_private {
> > > + void __iomem *base; /* registers */
> > > + struct clk *clk;
> > > + struct clk *clk_axi;
> > > + struct clk *clk_disp_axi;
> >
> > i.MX8mp RM mentions that clocks are pix_clk, apb_clk and b_clk.
> > Why not use them?
>
> That's only because the DT bindings are aligned between old and new LCDIF.
I remember I suggested a dedicated DT binding doc for i.MX8MP LCDIFv3.
If people are ok with one single doc, then why not respect the RM and
check compatible string to set different clock-names?
>
> [...]
>
> > > + switch (format) {
> > > + case DRM_FORMAT_RGB565:
> > > + writel(CTRLDESCL0_5_BPP_16_RGB565,
> > > + lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > > + break;
> > > + case DRM_FORMAT_RGB888:
> > > + writel(CTRLDESCL0_5_BPP_24_RGB888,
> > > + lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > > + break;
> > > + case DRM_FORMAT_XRGB1555:
> >
> > DRM_FORMAT_ARGB1555?
>
> Why would there be formats with Alpha channel here ?
> Because of the (unsupported) overlay planes ?
Because the RM says it's ARGB1555. When overlay planes are supported,
userspace doesn't want to see alpha component accidentally takes effect
when it is told to use XRGB1555.
>
> > > + writel(CTRLDESCL0_5_BPP_32_ARGB8888,
> > > + lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > > + break;
> > > + default:
> > > + dev_err(drm->dev, "Unknown pixel format 0x%x\n", format);
> > > + break;
> > > + }
> >
> > lcdif_set_formats() is called in lcdif_crtc_mode_set_nofb(), so no fb,
> > which means the framebuffer pixel format should be set in plane
> > callback.
>
> Do you have iMXRT and are/or you able to test overlay planes with this
> IP (the overlay planes which are currently not supported) ? Or is there
> any other iMX which has overlay plane available ?
I don't have any i.MXRT SoCs. I don't have any SoCs to test overlay
planes, either. Seems that there are i.MXRT SoCs are/will be supported
by Linux, so overlay does need some attention.
Regards,
Liu Ying
