| Message ID | 20230505124337.854845-3-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mgag200: Use DMA to copy the framebuffer to the VRAM | expand |
Hi Am 05.05.23 um 14:43 schrieb Jocelyn Falempe: > Now that the driver handles only 16, 24 and 32-bit framebuffer, > it can be simplified. I think it should say that the driver never really handled 8-bit colors. Or at least I'm not aware of. > > No functional changes. > > offset: > 16bit: (bppshift = 1) > offset = width >> (4 - bppshift) => width / 8 => pitch / 16 > > 24bit: (bppshift = 0) > offset = (width * 3) >> (4 - bppshift) => width * 3 / 16 => pitch / 16 > > 32bit: (bppshift = 2) > offset = width >> (4 - bppshift) => width / 4 => pitch / 16 > > scale: > 16bit: > scale = (1 << bppshift) - 1 => 1 > 24bit: > scale = ((1 << bppshift) * 3) - 1 => 2 > 32bit: > scale = (1 << bppshift) - 1 => 3 > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > --- > drivers/gpu/drm/mgag200/mgag200_mode.c | 63 +++++++------------------- > 1 file changed, 16 insertions(+), 47 deletions(-) > > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > index 9a24b9c00745..7d8c65372ac4 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c > @@ -280,30 +280,16 @@ void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mod > WREG8(MGA_MISC_OUT, misc); > } > > -static u8 mgag200_get_bpp_shift(const struct drm_format_info *format) > -{ > - static const u8 bpp_shift[] = {0, 1, 0, 2}; > - > - return bpp_shift[format->cpp[0] - 1]; > -} > - > /* > * Calculates the HW offset value from the framebuffer's pitch. The > * offset is a multiple of the pixel size and depends on the display > - * format. > + * format. With width in pixels and pitch in bytes, the formula is: > + * offset = width * bpp / 128 = pitch / 16 > */ > static u32 mgag200_calculate_offset(struct mga_device *mdev, > const struct drm_framebuffer *fb) > { > - u32 offset = fb->pitches[0] / fb->format->cpp[0]; > - u8 bppshift = mgag200_get_bpp_shift(fb->format); > - > - if (fb->format->cpp[0] * 8 == 24) > - offset = (offset * 3) >> (4 - bppshift); > - else > - offset = offset >> (4 - bppshift); > - > - return offset; > + return fb->pitches[0] >> 4; 24-bit is different from the rest. I don't understand how you simplified this code. Best regards Thomas > } > > static void mgag200_set_offset(struct mga_device *mdev, > @@ -326,48 +312,25 @@ static void mgag200_set_offset(struct mga_device *mdev, > void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_info *format) > { > struct drm_device *dev = &mdev->base; > - unsigned int bpp, bppshift, scale; > + unsigned int scale; > u8 crtcext3, xmulctrl; > > - bpp = format->cpp[0] * 8; > - > - bppshift = mgag200_get_bpp_shift(format); > - switch (bpp) { > - case 24: > - scale = ((1 << bppshift) * 3) - 1; > - break; > - default: > - scale = (1 << bppshift) - 1; > - break; > - } > - > - RREG_ECRT(3, crtcext3); > - > - switch (bpp) { > - case 8: > - xmulctrl = MGA1064_MUL_CTL_8bits; > - break; > - case 16: > - if (format->depth == 15) > - xmulctrl = MGA1064_MUL_CTL_15bits; > - else > - xmulctrl = MGA1064_MUL_CTL_16bits; > + switch (format->format) { > + case DRM_FORMAT_RGB565: > + xmulctrl = MGA1064_MUL_CTL_16bits; > break; > - case 24: > + case DRM_FORMAT_RGB888: > xmulctrl = MGA1064_MUL_CTL_24bits; > break; > - case 32: > + case DRM_FORMAT_XRGB8888: > xmulctrl = MGA1064_MUL_CTL_32_24bits; > break; > default: > /* BUG: We should have caught this problem already. */ > - drm_WARN_ON(dev, "invalid format depth\n"); > + drm_WARN_ON(dev, "invalid drm format\n"); > return; > } > > - crtcext3 &= ~GENMASK(2, 0); > - crtcext3 |= scale; > - > WREG_DAC(MGA1064_MUL_CTL, xmulctrl); > > WREG_GFX(0, 0x00); > @@ -383,6 +346,12 @@ void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_in > WREG_GFX(7, 0x0f); > WREG_GFX(8, 0x0f); > > + /* scale is the number of bytes per pixels - 1 */ > + scale = format->cpp[0] - 1; > + > + RREG_ECRT(3, crtcext3); > + crtcext3 &= ~GENMASK(2, 0); > + crtcext3 |= scale; > WREG_ECRT(3, crtcext3); > } >
On 08/05/2023 09:44, Thomas Zimmermann wrote: > Hi > > Am 05.05.23 um 14:43 schrieb Jocelyn Falempe: >> Now that the driver handles only 16, 24 and 32-bit framebuffer, >> it can be simplified. > > I think it should say that the driver never really handled 8-bit colors. > Or at least I'm not aware of. Ok I can change that. This patch is just a cleanup, and is not really necessary for DMA. > >> >> No functional changes. >> >> offset: >> 16bit: (bppshift = 1) >> offset = width >> (4 - bppshift) => width / 8 => pitch / 16 >> >> 24bit: (bppshift = 0) >> offset = (width * 3) >> (4 - bppshift) => width * 3 / 16 => pitch / 16 >> >> 32bit: (bppshift = 2) >> offset = width >> (4 - bppshift) => width / 4 => pitch / 16 >> >> scale: >> 16bit: >> scale = (1 << bppshift) - 1 => 1 >> 24bit: >> scale = ((1 << bppshift) * 3) - 1 => 2 >> 32bit: >> scale = (1 << bppshift) - 1 => 3 >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> --- >> drivers/gpu/drm/mgag200/mgag200_mode.c | 63 +++++++------------------- >> 1 file changed, 16 insertions(+), 47 deletions(-) >> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c >> b/drivers/gpu/drm/mgag200/mgag200_mode.c >> index 9a24b9c00745..7d8c65372ac4 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c >> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c >> @@ -280,30 +280,16 @@ void mgag200_set_mode_regs(struct mga_device >> *mdev, const struct drm_display_mod >> WREG8(MGA_MISC_OUT, misc); >> } >> -static u8 mgag200_get_bpp_shift(const struct drm_format_info *format) >> -{ >> - static const u8 bpp_shift[] = {0, 1, 0, 2}; >> - >> - return bpp_shift[format->cpp[0] - 1]; >> -} >> - >> /* >> * Calculates the HW offset value from the framebuffer's pitch. The >> * offset is a multiple of the pixel size and depends on the display >> - * format. >> + * format. With width in pixels and pitch in bytes, the formula is: >> + * offset = width * bpp / 128 = pitch / 16 >> */ >> static u32 mgag200_calculate_offset(struct mga_device *mdev, >> const struct drm_framebuffer *fb) >> { >> - u32 offset = fb->pitches[0] / fb->format->cpp[0]; >> - u8 bppshift = mgag200_get_bpp_shift(fb->format); >> - >> - if (fb->format->cpp[0] * 8 == 24) >> - offset = (offset * 3) >> (4 - bppshift); >> - else >> - offset = offset >> (4 - bppshift); >> - >> - return offset; >> + return fb->pitches[0] >> 4; > > 24-bit is different from the rest. I don't understand how you simplified > this code. This code was overly complex, but this special case is compensated by the "bpp_shift" thing. The formula width * bpp / 128 is in the G200 documentation 4.6.5 u32 offset = fb->pitches[0] / fb->format->cpp[0]; so offset is now the width in pixels. (pitches[0] is the width in bytes) bppshift is 0 for 24 bits, 1 for 16 bits and 2 for 32 bits offset: 16bit: (bppshift = 1) ; pitch = width * 2 offset = width >> (4 - bppshift) => width / 8 => pitch / 16 24bit: (bppshift = 0) ; pitch = width * 3 offset = (width * 3) >> (4 - bppshift) => width * 3 / 16 => pitch / 16 32bit: (bppshift = 2) ; pith = width * 4 offset = width >> (4 - bppshift) => width / 4 => pitch / 16 > > Best regards > Thomas > >> } >> static void mgag200_set_offset(struct mga_device *mdev, >> @@ -326,48 +312,25 @@ static void mgag200_set_offset(struct mga_device >> *mdev, >> void mgag200_set_format_regs(struct mga_device *mdev, const struct >> drm_format_info *format) >> { >> struct drm_device *dev = &mdev->base; >> - unsigned int bpp, bppshift, scale; >> + unsigned int scale; >> u8 crtcext3, xmulctrl; >> - bpp = format->cpp[0] * 8; >> - >> - bppshift = mgag200_get_bpp_shift(format); >> - switch (bpp) { >> - case 24: >> - scale = ((1 << bppshift) * 3) - 1; >> - break; >> - default: >> - scale = (1 << bppshift) - 1; >> - break; >> - } >> - >> - RREG_ECRT(3, crtcext3); >> - >> - switch (bpp) { >> - case 8: >> - xmulctrl = MGA1064_MUL_CTL_8bits; >> - break; >> - case 16: >> - if (format->depth == 15) >> - xmulctrl = MGA1064_MUL_CTL_15bits; >> - else >> - xmulctrl = MGA1064_MUL_CTL_16bits; >> + switch (format->format) { >> + case DRM_FORMAT_RGB565: >> + xmulctrl = MGA1064_MUL_CTL_16bits; >> break; >> - case 24: >> + case DRM_FORMAT_RGB888: >> xmulctrl = MGA1064_MUL_CTL_24bits; >> break; >> - case 32: >> + case DRM_FORMAT_XRGB8888: >> xmulctrl = MGA1064_MUL_CTL_32_24bits; >> break; >> default: >> /* BUG: We should have caught this problem already. */ >> - drm_WARN_ON(dev, "invalid format depth\n"); >> + drm_WARN_ON(dev, "invalid drm format\n"); >> return; >> } >> - crtcext3 &= ~GENMASK(2, 0); >> - crtcext3 |= scale; >> - >> WREG_DAC(MGA1064_MUL_CTL, xmulctrl); >> WREG_GFX(0, 0x00); >> @@ -383,6 +346,12 @@ void mgag200_set_format_regs(struct mga_device >> *mdev, const struct drm_format_in >> WREG_GFX(7, 0x0f); >> WREG_GFX(8, 0x0f); >> + /* scale is the number of bytes per pixels - 1 */ >> + scale = format->cpp[0] - 1; >> + >> + RREG_ECRT(3, crtcext3); >> + crtcext3 &= ~GENMASK(2, 0); >> + crtcext3 |= scale; >> WREG_ECRT(3, crtcext3); >> } >
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 9a24b9c00745..7d8c65372ac4 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -280,30 +280,16 @@ void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mod WREG8(MGA_MISC_OUT, misc); } -static u8 mgag200_get_bpp_shift(const struct drm_format_info *format) -{ - static const u8 bpp_shift[] = {0, 1, 0, 2}; - - return bpp_shift[format->cpp[0] - 1]; -} - /* * Calculates the HW offset value from the framebuffer's pitch. The * offset is a multiple of the pixel size and depends on the display - * format. + * format. With width in pixels and pitch in bytes, the formula is: + * offset = width * bpp / 128 = pitch / 16 */ static u32 mgag200_calculate_offset(struct mga_device *mdev, const struct drm_framebuffer *fb) { - u32 offset = fb->pitches[0] / fb->format->cpp[0]; - u8 bppshift = mgag200_get_bpp_shift(fb->format); - - if (fb->format->cpp[0] * 8 == 24) - offset = (offset * 3) >> (4 - bppshift); - else - offset = offset >> (4 - bppshift); - - return offset; + return fb->pitches[0] >> 4; } static void mgag200_set_offset(struct mga_device *mdev, @@ -326,48 +312,25 @@ static void mgag200_set_offset(struct mga_device *mdev, void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_info *format) { struct drm_device *dev = &mdev->base; - unsigned int bpp, bppshift, scale; + unsigned int scale; u8 crtcext3, xmulctrl; - bpp = format->cpp[0] * 8; - - bppshift = mgag200_get_bpp_shift(format); - switch (bpp) { - case 24: - scale = ((1 << bppshift) * 3) - 1; - break; - default: - scale = (1 << bppshift) - 1; - break; - } - - RREG_ECRT(3, crtcext3); - - switch (bpp) { - case 8: - xmulctrl = MGA1064_MUL_CTL_8bits; - break; - case 16: - if (format->depth == 15) - xmulctrl = MGA1064_MUL_CTL_15bits; - else - xmulctrl = MGA1064_MUL_CTL_16bits; + switch (format->format) { + case DRM_FORMAT_RGB565: + xmulctrl = MGA1064_MUL_CTL_16bits; break; - case 24: + case DRM_FORMAT_RGB888: xmulctrl = MGA1064_MUL_CTL_24bits; break; - case 32: + case DRM_FORMAT_XRGB8888: xmulctrl = MGA1064_MUL_CTL_32_24bits; break; default: /* BUG: We should have caught this problem already. */ - drm_WARN_ON(dev, "invalid format depth\n"); + drm_WARN_ON(dev, "invalid drm format\n"); return; } - crtcext3 &= ~GENMASK(2, 0); - crtcext3 |= scale; - WREG_DAC(MGA1064_MUL_CTL, xmulctrl); WREG_GFX(0, 0x00); @@ -383,6 +346,12 @@ void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_in WREG_GFX(7, 0x0f); WREG_GFX(8, 0x0f); + /* scale is the number of bytes per pixels - 1 */ + scale = format->cpp[0] - 1; + + RREG_ECRT(3, crtcext3); + crtcext3 &= ~GENMASK(2, 0); + crtcext3 |= scale; WREG_ECRT(3, crtcext3); }
Now that the driver handles only 16, 24 and 32-bit framebuffer, it can be simplified. No functional changes. offset: 16bit: (bppshift = 1) offset = width >> (4 - bppshift) => width / 8 => pitch / 16 24bit: (bppshift = 0) offset = (width * 3) >> (4 - bppshift) => width * 3 / 16 => pitch / 16 32bit: (bppshift = 2) offset = width >> (4 - bppshift) => width / 4 => pitch / 16 scale: 16bit: scale = (1 << bppshift) - 1 => 1 24bit: scale = ((1 << bppshift) * 3) - 1 => 2 32bit: scale = (1 << bppshift) - 1 => 3 Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> --- drivers/gpu/drm/mgag200/mgag200_mode.c | 63 +++++++------------------- 1 file changed, 16 insertions(+), 47 deletions(-)