| Message ID | 20190303173527.31055-3-jagan@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: sun6i_mipi_dsi: Fixes/updates | expand |
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/ Did you encounter any panel where this was fixing something? If so, which one, and what is the matching timings and / or datasheet? Maxime
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@bootlin.com> 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. > 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.. > 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. Maxime
On Thu, Mar 7, 2019 at 9:09 PM Maxime Ripard <maxime.ripard@bootlin.com> 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@bootlin.com> 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.
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@bootlin.com> 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@bootlin.com> 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
On Mon, Mar 11, 2019 at 7:39 PM Maxime Ripard <maxime.ripard@bootlin.com> 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@bootlin.com> 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@bootlin.com> 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/
On Mon, Mar 11, 2019 at 08:28:22PM +0530, Jagan Teki wrote: > On Mon, Mar 11, 2019 at 7:39 PM Maxime Ripard <maxime.ripard@bootlin.com> 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@bootlin.com> 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@bootlin.com> 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 You're missing the point. What I'm asking for is why you think that panel->lcd_ht == (timmings->hor_front_porch + panel->lcd_hbp + panel->lcd_x) and not timmings->hor_front_porch + panel->lcd_hbp + sync_width + panel->lcd_x maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, Mar 19, 2019 at 4:26 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Mon, Mar 11, 2019 at 08:28:22PM +0530, Jagan Teki wrote: > > On Mon, Mar 11, 2019 at 7:39 PM Maxime Ripard <maxime.ripard@bootlin.com> 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@bootlin.com> 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@bootlin.com> 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 > > You're missing the point. What I'm asking for is why you think that > panel->lcd_ht == (timmings->hor_front_porch + panel->lcd_hbp + > panel->lcd_x) and not timmings->hor_front_porch + panel->lcd_hbp + > sync_width + panel->lcd_x No ie not true we can't include sync_width, please see here [1] timmings->hor_front_porch= panel_info->lcd_ht-panel_info->lcd_hbp - panel_info->lcd_x; panel_info->lcd_ht = timmings->hor_front_porch + panel_info->lcd_hbp + panel_info->lcd_x; and the result of this lcd_ht is exactly matched with Mainline timings or any supported panel in A64. [1] https://github.com/longsleep/linux-pine64/blob/pine64-hacks-1.2/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1839
On Thu, Mar 28, 2019 at 05:23:39PM +0530, Jagan Teki wrote: > On Tue, Mar 19, 2019 at 4:26 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Mon, Mar 11, 2019 at 08:28:22PM +0530, Jagan Teki wrote: > > > On Mon, Mar 11, 2019 at 7:39 PM Maxime Ripard <maxime.ripard@bootlin.com> 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@bootlin.com> 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@bootlin.com> 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 > > > > You're missing the point. What I'm asking for is why you think that > > panel->lcd_ht == (timmings->hor_front_porch + panel->lcd_hbp + > > panel->lcd_x) and not timmings->hor_front_porch + panel->lcd_hbp + > > sync_width + panel->lcd_x > > No ie not true we can't include sync_width, please see here [1] > > timmings->hor_front_porch= panel_info->lcd_ht-panel_info->lcd_hbp - > panel_info->lcd_x; > panel_info->lcd_ht = timmings->hor_front_porch + panel_info->lcd_hbp + > panel_info->lcd_x; > > and the result of this lcd_ht is exactly matched with Mainline timings > or any supported panel in A64. > > [1] https://github.com/longsleep/linux-pine64/blob/pine64-hacks-1.2/drivers/video/sunxi/disp2/disp/de/disp_lcd.c#L1839 Can we please start to reason in actual timings instead of whatever Allwinner comes up with in their BSP? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 465e7fc57899..140e55f5ed2e 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -436,9 +436,9 @@ static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi, SUN6I_DSI_BURST_LINE_SYNC_POINT(SUN6I_DSI_SYNC_POINT)); val = SUN6I_DSI_TCON_DRQ_ENABLE_MODE; - } else if ((mode->hsync_end - mode->hdisplay) > 20) { + } else if ((mode->hsync_start - mode->hdisplay) > 20) { /* Maaaaaagic */ - u16 drq = (mode->hsync_end - mode->hdisplay) - 20; + u16 drq = (mode->hsync_start - mode->hdisplay) - 20; drq *= mipi_dsi_pixel_format_to_bpp(device->format); drq /= 32;