Re: [PATCH v9 2/5] drm/sun4i: sun6i_mipi_dsi: Fix TCON DRQ set bits

[Jagan Teki]

On Thu, Mar 7, 2019 at 9:09 PM Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:
>
> On Thu, Mar 07, 2019 at 05:49:07PM +0530, Jagan Teki wrote:
> > On Mon, Mar 4, 2019 at 9:13 PM Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:
> > >
> > > On Sun, Mar 03, 2019 at 11:05:24PM +0530, Jagan Teki wrote:
> > > > TCON DRQ for non-burst DSI mode can computed based on horizontal
> > > > front porch value, but the current driver trying to include sync
> > > > timings along with front porch resulting wrong drq.
> > > >
> > > > This patch is trying to update the drq by subtracting hsync_start
> > > > with hdisplay, which is horizontal front porch.
> > > >
> > > > Current code:
> > > > ------------
> > > > mode->hsync_end - mode->hdisplay => horizontal front porch + sync
> > > >
> > > > With this patch:
> > > > ----------------
> > > > mode->hsync_start - mode->hdisplay => horizontal front porch
> > > >
> > > > BSP code form BPI-M64-bsp is computing TCON DRQ set bits
> > > > for non-burts as (from linux-sunxi/
> > > > drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c)
> > > >
> > > > => panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
> > > > => (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
> > >                                   ^ + sync length +
> > > >    - panel->lcd_x - panel->hbp
> > > > => timmings->hor_front_porch
> > >                                ^ + sync
> > > > => mode->hsync_start - mode->hdisplay
> > >
> > > s/hsync_start/hsync_end/
> >
> > No, it should be front porch so it is hsync_start. This change is
> > trying to update DRQ set to use front porch and above evaluation from
> > BSP, result the same front front porch
> >
> > Current driver has hsync_end - hdisplay which is not front porch
> > timing (it is adding extra sync timing).
>
> It would be if you considered that the back porch actually was the
> back porch plus the sync length. I have found no such evidence, quite
> the opposite actually, everything seems to point at the fact that
> unlike the TCON, the DSI block uses the back porch as only the back
> porch.

Sorry, I'm not clear about back porch here.

The current code has mode->hsync_end - mode->hdisplay which is Front
porch + sync time do you think it's not? DRQ set time is pure front
porch value (according BSP) as I didn't see any information about DRQ
set bits in manual or anywhere.

fyi: atleast if you didn't trust me, here is existing applied patch
for about equations.
https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c?id=2cfdc24d2f8d9b14704567c065beb2a118a578fa

So, this patch fixed to remove sync time by updating hsync_start -
hdisplay which is pure front porch. and it's clear that pane is not
working without this.

>
>
> > I believe this is something similar like fixed patches for VBP, HBLK
> > timings.
> >
> > > Did you encounter any panel where this was fixing something? If so,
> > > which one, and what is the matching timings and / or datasheet?
> >
> > W/O this change Bananapi  s070wv20 panel has issue on striped lines on
> > the panel[1] and timings are
> >
> > static const struct drm_display_mode s070wv20_default_mode = {
> >         .clock = 30000,
> >         .vrefresh = 60,
> >
> >         .hdisplay = 800,
> >         .hsync_start = 800 + 40,
> >         .hsync_end = 800 + 40 + 48,
> >         .htotal = 800 + 40 + 48 + 40,
> >
> >         .vdisplay = 480,
> >         .vsync_start = 480 + 13,
> >         .vsync_end = 480 + 13 + 3,
> >         .vtotal = 480 + 13 + 3 + 29,
> > };
> >
> > Which is similar like in panel-simple "bananapi,s070wv20-ct16"
> >
> > Here is the DSI panel patches and sequence:
> > [pixel clock is 30Mhz] https://patchwork.kernel.org/patch/10680331/
> > https://github.com/yesnoandor/x300/blob/master/kernel/arch/arm/boot/dts/erobbing/x300/x300.dtsi#L81
> > https://github.com/wxzed/Raspberry_5MIPI_Display/blob/master/I2C_Slave/USER/main.c#L15
> > https://github.com/eliot-shao/qcom/blob/master/icn6211_cxn0102/kernel/drivers/video/msm/mdss/mdss_i2c_interface.c#L152
>
> What are those supposed to be? It doesn't look like timings but rather initialization sequences
>
> > matches timings for
> > https://github.com/eliot-shao/qcom/blob/master/icn6211_cxn0102/kernel/arch/arm/boot/dts/qcom/dsi-mipi-2-rgb_1280p_video.dtsi#L20
>
> That's not even the same resolution..

fyi about the sequence.

>
> > https://github.com/zestroly/micromat/blob/master/test/raspberry/ICN6211.cpp#L169
>
> And this isn't a set of timings either.
>
> > Attached is panel datasheet.
>
> Which is for an RGB panel... not a MIPI-DSI one.

Same panel timings It is a DSI ICN6211 bridge, I have attached the
patch above https://patchwork.kernel.org/patch/10680331/ for
information about the display please find previous mail attachment.