| Message ID | 54deec2ec533e90544faa8c60a0c2518c58f3e9c.1689252746.git.geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1 | expand |
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert, Thanks for your patch! > The page height must be taken into account only for vertical coordinates > and heights, not for horizontal coordinates and widths. > > Fixes: 179a790aaf2a0127 ("drm/ssd130x: Set the page height value in the device info data") > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > drivers/gpu/drm/solomon/ssd130x.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c > index afb08a8aa9fcdaf2..b4c376962629580b 100644 > --- a/drivers/gpu/drm/solomon/ssd130x.c > +++ b/drivers/gpu/drm/solomon/ssd130x.c > @@ -596,7 +596,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m > rect->y1 = round_down(rect->y1, page_height); > rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height); > > - dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height); > + dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); > That's true for ssd130x controllers that use R1, but when doing that change one of my goals was to prepare the driver for supporting the ssd132x family that use a 16-grayscale pixel format (R4). For those controllers, the pixels are encoded in 4-bit and each page has two pixels. So for those controllers the dst_pitch will need to be DIV_ROUND_UP(drm_rect_width(rect), 2) instead since the width is not 8 in that case. So I would prefer to skip this patch from your set, because otherwise we will need to revert it when adding support for SSD132x controllers.
Hi Javier, On Fri, Jul 14, 2023 at 11:34 AM Javier Martinez Canillas <javierm@redhat.com> wrote: > Geert Uytterhoeven <geert@linux-m68k.org> writes: > > The page height must be taken into account only for vertical coordinates > > and heights, not for horizontal coordinates and widths. > > > > Fixes: 179a790aaf2a0127 ("drm/ssd130x: Set the page height value in the device info data") > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > --- a/drivers/gpu/drm/solomon/ssd130x.c > > +++ b/drivers/gpu/drm/solomon/ssd130x.c > > @@ -596,7 +596,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m > > rect->y1 = round_down(rect->y1, page_height); > > rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height); > > > > - dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height); > > + dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); > > > > That's true for ssd130x controllers that use R1, but when doing that > change one of my goals was to prepare the driver for supporting the > ssd132x family that use a 16-grayscale pixel format (R4). > > For those controllers, the pixels are encoded in 4-bit and each page > has two pixels. So for those controllers the dst_pitch will need to > be DIV_ROUND_UP(drm_rect_width(rect), 2) instead since the width is > not 8 in that case. > > So I would prefer to skip this patch from your set, because otherwise > we will need to revert it when adding support for SSD132x controllers. My point is that the 8 as used here is related to the number of bits per pixel, not to the page height. The page height might also be impacted by the number of bits per pixel, but that is orthogonal. Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: > Hi Javier, > > On Fri, Jul 14, 2023 at 11:34 AM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> Geert Uytterhoeven <geert@linux-m68k.org> writes: >> > The page height must be taken into account only for vertical coordinates >> > and heights, not for horizontal coordinates and widths. >> > >> > Fixes: 179a790aaf2a0127 ("drm/ssd130x: Set the page height value in the device info data") >> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > >> > --- a/drivers/gpu/drm/solomon/ssd130x.c >> > +++ b/drivers/gpu/drm/solomon/ssd130x.c >> > @@ -596,7 +596,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m >> > rect->y1 = round_down(rect->y1, page_height); >> > rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height); >> > >> > - dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height); >> > + dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); >> > >> >> That's true for ssd130x controllers that use R1, but when doing that >> change one of my goals was to prepare the driver for supporting the >> ssd132x family that use a 16-grayscale pixel format (R4). >> >> For those controllers, the pixels are encoded in 4-bit and each page >> has two pixels. So for those controllers the dst_pitch will need to >> be DIV_ROUND_UP(drm_rect_width(rect), 2) instead since the width is >> not 8 in that case. >> >> So I would prefer to skip this patch from your set, because otherwise >> we will need to revert it when adding support for SSD132x controllers. > > My point is that the 8 as used here is related to the number of bits per pixel, > not to the page height. The page height might also be impacted by the > number of bits per pixel, but that is orthogonal. > Ah, I see what you mean. Yes, you are right. We can later add a different variable when adding support for controllers using R4. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Javier Martinez Canillas <javierm@redhat.com> writes: Hello Geert, > Geert Uytterhoeven <geert@linux-m68k.org> writes: > [...] >> >> My point is that the 8 as used here is related to the number of bits per pixel, >> not to the page height. The page height might also be impacted by the >> number of bits per pixel, but that is orthogonal. >> > > Ah, I see what you mean. Yes, you are right. We can later add a > different variable when adding support for controllers using R4. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Pushed to drm-misc (drm-misc-next) since this fix is independent of the rest of the patches. Thanks!
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index afb08a8aa9fcdaf2..b4c376962629580b 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -596,7 +596,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m rect->y1 = round_down(rect->y1, page_height); rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height); - dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height); + dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); if (ret)
The page height must be taken into account only for vertical coordinates and heights, not for horizontal coordinates and widths. Fixes: 179a790aaf2a0127 ("drm/ssd130x: Set the page height value in the device info data") Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> --- drivers/gpu/drm/solomon/ssd130x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)