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);
 
 	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
 	if (ret)
-- 
2.34.1

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.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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 -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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!

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat