| Message ID | 20210121105759.1262699-1-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/vc4: Correct lbm size and calculation | expand |
Hi Maxime, >> This one should fix your issue >> Feel free to test it and let me know if it's not the case > I confirm that the patches fix the issue I was seeing. I also applied the sent patches [PATCH 1/2] drm/vc4: Correct lbm size and calculation [PATCH 2/2] drm/vc4: Correct POS1_SCL for hvs5 to the 5.10.9 kernel, and the symptom disappeared on my Raspberry Pi 4B 8GB. Thank you very much! Ryutaroh
Hi Maxime On Thu, 21 Jan 2021 at 10:58, Maxime Ripard <maxime@cerno.tech> wrote: > > From: Dom Cobley <popcornmix@gmail.com> > > LBM base address is measured in units of pixels per cycle. > That is 4 for 2711 (hvs5) and 2 for 2708. > > We are wasting 75% of lbm by indexing without the scaling. > But we were also using too high a size for the lbm resulting > in partial corruption (right hand side) of vertically > scaled images, usually at 4K or lower resolutions with more layers. > > The physical RAM of LBM on 2711 is 8 * 1920 * 16 * 12-bit > (pixels are stored 12-bits per component regardless of format). > > The LBM adress indexes work in units of pixels per clock, > so for 4 pixels per clock that means we have 32 * 1920 = 60K > > Fixes: c54619b0bfb3 ("drm/vc4: Add support for the BCM2711 HVS5") > Signed-off-by: Dom Cobley <popcornmix@gmail.com> > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/gpu/drm/vc4/vc4_hvs.c | 8 ++++---- > drivers/gpu/drm/vc4/vc4_plane.c | 7 ++++++- > 2 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c > index 2b3a597fa65f..c239045e05d6 100644 > --- a/drivers/gpu/drm/vc4/vc4_hvs.c > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c > @@ -622,11 +622,11 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) > * for now we just allocate globally. > */ > if (!hvs->hvs5) > - /* 96kB */ > - drm_mm_init(&hvs->lbm_mm, 0, 96 * 1024); > + /* 48k words of 2x12-bit pixels */ > + drm_mm_init(&hvs->lbm_mm, 0, 48 * 1024); > else > - /* 70k words */ > - drm_mm_init(&hvs->lbm_mm, 0, 70 * 2 * 1024); > + /* 60k words of 4x12-bit pixels */ > + drm_mm_init(&hvs->lbm_mm, 0, 60 * 1024); > > /* Upload filter kernels. We only have the one for now, so we > * keep it around for the lifetime of the driver. > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c > index 6bd8260aa9f2..b98eabb52920 100644 > --- a/drivers/gpu/drm/vc4/vc4_plane.c > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > @@ -437,6 +437,7 @@ static void vc4_write_ppf(struct vc4_plane_state *vc4_state, u32 src, u32 dst) > static u32 vc4_lbm_size(struct drm_plane_state *state) > { > struct vc4_plane_state *vc4_state = to_vc4_plane_state(state); > + struct vc4_dev *vc4 = to_vc4_dev(state->plane->dev); > u32 pix_per_line; > u32 lbm; > > @@ -472,7 +473,11 @@ static u32 vc4_lbm_size(struct drm_plane_state *state) > lbm = pix_per_line * 16; > } > > - lbm = roundup(lbm, 32); > + /* Align it to 64 or 128 (hvs5) bytes */ > + lbm = roundup(lbm, vc4->hvs->hvs5 ? 128 : 64); > + > + /* Each "word" of the LBM memory contains 2 or 4 (hvs5) pixels */ > + lbm /= vc4->hvs->hvs5 ? 4 : 2; > > return lbm; > } > -- > 2.29.2 >
Hi Lucas, On Thu, Jan 21, 2021 at 05:26:22PM +0100, Lucas Nussbaum wrote: > Hi Maxime, > > On 21/01/21 at 12:04 +0100, Maxime Ripard wrote: > > Hi Lucas, Ryutaroh, > > > > On Thu, Jan 21, 2021 at 11:57:58AM +0100, Maxime Ripard wrote: > > > From: Dom Cobley <popcornmix@gmail.com> > > > > > > LBM base address is measured in units of pixels per cycle. > > > That is 4 for 2711 (hvs5) and 2 for 2708. > > > > > > We are wasting 75% of lbm by indexing without the scaling. > > > But we were also using too high a size for the lbm resulting > > > in partial corruption (right hand side) of vertically > > > scaled images, usually at 4K or lower resolutions with more layers. > > > > > > The physical RAM of LBM on 2711 is 8 * 1920 * 16 * 12-bit > > > (pixels are stored 12-bits per component regardless of format). > > > > > > The LBM adress indexes work in units of pixels per clock, > > > so for 4 pixels per clock that means we have 32 * 1920 = 60K > > > > > > Fixes: c54619b0bfb3 ("drm/vc4: Add support for the BCM2711 HVS5") > > > Signed-off-by: Dom Cobley <popcornmix@gmail.com> > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > > This one should fix your issue > > > > Feel free to test it and let me know if it's not the case > > I confirm that the patches fix the issue I was seeing. Great. Can I add your Tested-by (and Ryutaroh, can I add yours as well?) Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index 2b3a597fa65f..c239045e05d6 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/drivers/gpu/drm/vc4/vc4_hvs.c @@ -622,11 +622,11 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) * for now we just allocate globally. */ if (!hvs->hvs5) - /* 96kB */ - drm_mm_init(&hvs->lbm_mm, 0, 96 * 1024); + /* 48k words of 2x12-bit pixels */ + drm_mm_init(&hvs->lbm_mm, 0, 48 * 1024); else - /* 70k words */ - drm_mm_init(&hvs->lbm_mm, 0, 70 * 2 * 1024); + /* 60k words of 4x12-bit pixels */ + drm_mm_init(&hvs->lbm_mm, 0, 60 * 1024); /* Upload filter kernels. We only have the one for now, so we * keep it around for the lifetime of the driver. diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 6bd8260aa9f2..b98eabb52920 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -437,6 +437,7 @@ static void vc4_write_ppf(struct vc4_plane_state *vc4_state, u32 src, u32 dst) static u32 vc4_lbm_size(struct drm_plane_state *state) { struct vc4_plane_state *vc4_state = to_vc4_plane_state(state); + struct vc4_dev *vc4 = to_vc4_dev(state->plane->dev); u32 pix_per_line; u32 lbm; @@ -472,7 +473,11 @@ static u32 vc4_lbm_size(struct drm_plane_state *state) lbm = pix_per_line * 16; } - lbm = roundup(lbm, 32); + /* Align it to 64 or 128 (hvs5) bytes */ + lbm = roundup(lbm, vc4->hvs->hvs5 ? 128 : 64); + + /* Each "word" of the LBM memory contains 2 or 4 (hvs5) pixels */ + lbm /= vc4->hvs->hvs5 ? 4 : 2; return lbm; }