Re: [PATCH 2/2] drm: lcdif: Add i.MX93 LCDIF support

[Liu Ying]

On Tue, 2023-01-24 at 12:15 +0100, Alexander Stein wrote:
> Hi,

Hi,

> 
> Am Dienstag, 24. Januar 2023, 08:59:39 CET schrieb Liu Ying:
> > On Mon, 2023-01-23 at 16:57 +0100, Marek Vasut wrote:
> > > On 1/23/23 08:23, Liu Ying wrote:
> > > > The LCDIF embedded in i.MX93 SoC is essentially the same to
> > > > those
> > > > in i.MX8mp SoC.  However, i.MX93 LCDIF may connect with MIPI
> > > > DSI
> > > > controller through LCDIF cross line pattern(controlled by
> > > > mediamix
> > > > blk-ctrl) or connect with LVDS display bridge(LDB) directly or
> > > > a
> > > > parallel display(also through mediamix blk-ctrl), so add
> > > > multiple
> > > > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in
> > > > the
> > > > LCDIF DRM driver and find a bridge to attach the relevant
> > > > encoder's
> > > > chain when needed.  While at it, derive lcdif_crtc_state
> > > > structure
> > > > from drm_crtc_state structure to introduce bus_format and
> > > > bus_flags
> > > > states so that the next downstream bridges may use consistent
> > > > bus
> > > > format and bus flags.
> > > 
> > > Would it be possible to split this patch into preparatory clean
> > > up
> > > and
> > > i.MX93 addition ? It seems like the patch is doing two things
> > > according
> > > to the commit message.
> > 
> > IMHO, all the patch does is for i.MX93 addition, not for clean up.
> > Note that the single LCDIF embedded in i.MX93 SoC may connect with
> > MIPI
> > DSI/LVDS/parallel related bridges to drive triple displays
> > _simultaneously_ in theory, while the three LCDIF instances
> > embedded in
> > i.MX8mp SoC connect with MIPI DSI/LVDS/HDMI displays
> > respectively(one
> > LCDIF maps to one display).  The multiple encoders addition and the
> > new
> > checks for consistent bus format and bus flags are only for i.MX93
> > LCDIF, not for i.MX8mp LCDIF.  Also, I think the multiple encoders
> > addition and the new checks should be done together - if the new
> > checks
> > come first, then the new checks do not make sense(no multiple
> > displays
> > driven by LCDIF); 
> 
> You are right on this one, but on the other hand there are lot of
> preparing 
> patches already. Even if it is useless by itself, having the bus
> format & flag 
> checks in a separate patch, it is easier to review, IMHO.

TBH, this way of separating patch looks too artificial. It's odd to
check consistent bus format and bus flags for multiple bridges while
there is only one encoder.

What I can do is to make a separate patch to introduce the
lcdif_crtc_state structure(with bus_format and bus_flags members) and
determine the format and flags in ->atomic_check() instead of
->atomic_enable().  And then, add i.MX93 LCDIF support with an another
patch which adds multiple encoders+bridges and new checks for
consistent bus format and bus flags.  Sounds good?

Regards,
Liu Ying  

> 
> > if the new checks come later, then it would be a bug
> > to allow inconsistent bus format and bus flags across the next
> > downstream bridges when only adding multiple encoders support(also,
> > I
> > don't know which encoder's bridge should determine the LCDIF output
> > bus
> > format and bus flags, since the three encoders come together with
> > the
> > three next bridges).
> 
> Agreed, this order is a no-go.
> 
> Best regards,
> Alexander
> 
> > Regards,
> > Liu Ying
> 
> 
> 
>