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

[Jagan Teki]

On Mon, Mar 11, 2019 at 7:39 PM Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:
>
> On Thu, Mar 07, 2019 at 09:24:02PM +0530, Jagan Teki wrote:
> > 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?
>
> It is.
>
> > 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.
>
> This is what I'm telling you. If you consider the back porch as only
> the back porch, then the result of that BSP calculation you mentionned
> earlier is the front porch and the sync length.
>
> You imply that the back porch should actually be treated as the back
> porch and the sync length in your calculation. What makes you say so?

I'm consider the computations accordingly to drm_modes.h and which
matches similar like BSP code.

mode->hsync_end - mode->hdisplay = front porch + sync

but the actual computation is
mode->hsync_start - mode->hdisplay =>  front porch

Which is similar to what BSP is using.

=> panel->lcd_ht -    panel->lcd_x - panel->lcd_hbp
=> (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x)
   - panel->lcd_x - panel->hbp
=> timmings->hor_front_porch
=> mode->hsync_start - mode->hdisplay

>
> > 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
>
> I'm aware of that patch, but I have no idea why do you bring it up in
> that discussion.

I'm trying to give valid computation which we wrongly mentioned
before. just a reference.

>
> > 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.
>
> This is the same discussion over and over again: which panel, which
> datasheet, not working how?

Like I said before. This bananapi,s070wv20-ct16 DSI bridge and we
don't have exact datasheet other than all  the information that I sent
in previous mail. Chen-Yu has mentioned all these references before[1]

>
> > > > 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.
>
> How is that relevant?
>
> > > > 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.
>
> The previous attachment was for a display that doesn't even have the
> same bus. How is that relevant?

No, it's same datasheet for RGB, DSI and we have only of that.

[1] https://patchwork.kernel.org/patch/10618421/