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

[Maxime Ripard]

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?

> 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.

> 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?

> > > 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?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature