| Message ID | 20230123072358.1060670-1-victor.liu@nxp.com |
|---|---|
| Headers | show |
| Series | drm: lcdif: Add i.MX93 LCDIF support | expand |
On 23/01/2023 08:23, Liu Ying wrote: > There is one LCDIF embedded in i.MX93 SoC to connect with > MIPI DSI controller through LCDIF cross line pattern(controlled > by mediamix blk-ctrl) or connect with LVDS display bridge(LDB) > directly or connect with a parallel display through parallel > display format(also controlled by mediamix blk-ctrl). i.MX93 > LCDIF IP is essentially the same to i.MX8MP LCDIF IP. Add device > tree binding for i.MX93 LCDIF. > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 1/23/23 08:23, Liu Ying wrote: > There is one LCDIF embedded in i.MX93 SoC to connect with > MIPI DSI controller through LCDIF cross line pattern(controlled > by mediamix blk-ctrl) or connect with LVDS display bridge(LDB) > directly or connect with a parallel display through parallel > display format(also controlled by mediamix blk-ctrl). i.MX93 > LCDIF IP is essentially the same to i.MX8MP LCDIF IP. Add device > tree binding for i.MX93 LCDIF. > > Signed-off-by: Liu Ying <victor.liu@nxp.com> Reviewed-by: Marek Vasut <marex@denx.de>
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. > 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
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 > > > >
On 1/24/23 12:15, 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. I agree on the ease of review. [...]