Re: [PATCH 1/6] drm: bridge: samsung-dsim: Support multi-lane calculations

[Adam Ford]

On Mon, Apr 17, 2023 at 6:55 AM Adam Ford <aford173@xxxxxxxxx> wrote:
>
> On Mon, Apr 17, 2023 at 3:43 AM Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
> >
> > Hi Adam,
> >
> > Am Samstag, dem 15.04.2023 um 05:40 -0500 schrieb Adam Ford:
> > > If there is more than one lane, the HFP, HBP, and HSA is calculated in
> > > bytes/pixel, then they are divided amongst the different lanes with some
> > > additional overhead. This is necessary to achieve higher resolutions while
> > > keeping the pixel clocks lower as the number of lanes increase.
> > >
> >
> > In the testing I did to come up with my patch "drm: bridge: samsung-
> > dsim: fix blanking packet size calculation" the number of lanes didn't
> > make any difference. My testing might be flawed, as I could only
> > measure the blanking after translation from MIPI DSI to DPI, so I'm
> > interested to know what others did here. How did you validate the
> > blanking with your patch? Would you have a chance to test my patch and
> > see if it works or breaks in your setup?

Lucas,

I tried your patch instead of mine.  Yours is dependent on the
hs_clock being always set to the burst clock which is configured by
the device tree.  I unrolled a bit of my stuff and replaced it with
yours.  It worked at 1080p, but when I tried a few other resolutions,
they did not work.  I assume it's because the DSI clock is fixed and
not changing based on the pixel clock.  In the version I did, I only
did that math when the lanes were > 1. In your patch, you divide by 8,
and in mine, I fetch the bits-per-pixel (which is 8) and I divide by
that just in case the bpp ever changes from 8.  Overall,  I think our
patches basically do the same thing.

adam

>
> Mine was purely by trial and error.  I don't have a scope, nor do I
> have a copy of the MIPI DSI spec, so if the image sync'd with my
> monitor, I treated it as successful.
>
> I can give yours a try, but it might be a few days since I've only
> been working on this stuff a bit in my spare time.
>
> adam
>
> >
> > Regards,
> > Lucas
> >
> > > Signed-off-by: Adam Ford <aford173@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/bridge/samsung-dsim.c | 40 +++++++++++++++++++++++----
> > >  1 file changed, 34 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > index e0a402a85787..1ccbad4ea577 100644
> > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > @@ -215,6 +215,7 @@
> > >  #define DSI_RX_FIFO_SIZE             256
> > >  #define DSI_XFER_TIMEOUT_MS          100
> > >  #define DSI_RX_FIFO_EMPTY            0x30800002
> > > +#define DSI_HSYNC_PKT_OVERHEAD       6
> > >
> > >  #define OLD_SCLK_MIPI_CLK_NAME               "pll_clk"
> > >
> > > @@ -879,13 +880,40 @@ static void samsung_dsim_set_display_mode(struct samsung_dsim *dsi)
> > >                       | DSIM_MAIN_VBP(m->vtotal - m->vsync_end);
> > >               samsung_dsim_write(dsi, DSIM_MVPORCH_REG, reg);
> > >
> > > -             reg = DSIM_MAIN_HFP(m->hsync_start - m->hdisplay)
> > > -                     | DSIM_MAIN_HBP(m->htotal - m->hsync_end);
> > > -             samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
> > > +             /*
> > > +              * If there is more than one lane, the HFP, HBP, and HSA
> > > +              * is calculated in bytes/pixel, then they are divided
> > > +              * amongst the different lanes with some additional
> > > +              * overhead correction
> > > +              */
> > > +             if (dsi->lanes > 1) {
> > > +                     u32 hfp, hbp, hsa;
> > > +                     int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format) / 8;
> > > +
> > > +                     hfp = ((m->hsync_start - m->hdisplay) * bpp) / dsi->lanes;
> > > +                     hfp -= (hfp > DSI_HSYNC_PKT_OVERHEAD) ? DSI_HSYNC_PKT_OVERHEAD : 0;
> > > +
> > > +                     hbp = ((m->htotal - m->hsync_end) * bpp) / dsi->lanes;
> > > +                     hbp -= (hbp > DSI_HSYNC_PKT_OVERHEAD) ? DSI_HSYNC_PKT_OVERHEAD : 0;
> > >
> > > -             reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
> > > -                     | DSIM_MAIN_HSA(m->hsync_end - m->hsync_start);
> > > -             samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
> > > +                     hsa = ((m->hsync_end - m->hsync_start) * bpp) / dsi->lanes;
> > > +                     hsa -= (hsa > DSI_HSYNC_PKT_OVERHEAD) ? DSI_HSYNC_PKT_OVERHEAD : 0;
> > > +
> > > +                     reg = DSIM_MAIN_HFP(hfp) | DSIM_MAIN_HBP(hbp);
> > > +                     samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
> > > +
> > > +                     reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
> > > +                             | DSIM_MAIN_HSA(hsa);
> > > +                     samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
> > > +             } else {
> > > +                     reg = DSIM_MAIN_HFP(m->hsync_start - m->hdisplay)
> > > +                             | DSIM_MAIN_HBP(m->htotal - m->hsync_end);
> > > +                     samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
> > > +
> > > +                     reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
> > > +                             | DSIM_MAIN_HSA(m->hsync_end - m->hsync_start);
> > > +                     samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
> > > +             }
> > >       }
> > >       reg =  DSIM_MAIN_HRESOL(m->hdisplay, num_bits_resol) |
> > >               DSIM_MAIN_VRESOL(m->vdisplay, num_bits_resol);
> >