mbox series

[0/5] drm/ssd130x: A few enhancements and cleanups

Message ID 20230605074753.562332-1-javierm@redhat.com (mailing list archive)
Headers show
Series drm/ssd130x: A few enhancements and cleanups | expand

Message

Javier Martinez Canillas June 5, 2023, 7:47 a.m. UTC
Hello,

While working on adding support for the SSD132X family of 4-bit grayscale
Solomon OLED panel controllers, I noticed a few things in the driver that
can be improved and make extending to support other chip families easier.

I've split the preparatory patches in this series and will post the actual
SSD132X support as a separate patch-set once this one is merged.

Best regards,
Javier


Javier Martinez Canillas (5):
  drm/ssd130x: Make default width and height to be controller dependent
  dt-bindings: display: ssd1307fb: Remove default width and height
    values
  drm/ssd130x: Set the page height value in the device info data
  drm/ssd130x: Don't allocate buffers on each plane update
  drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc()

 .../bindings/display/solomon,ssd1307fb.yaml   |   8 +-
 drivers/gpu/drm/solomon/ssd130x.c             | 124 ++++++++++++------
 drivers/gpu/drm/solomon/ssd130x.h             |   6 +
 3 files changed, 93 insertions(+), 45 deletions(-)

Comments

Thomas Zimmermann June 6, 2023, 8:01 a.m. UTC | #1
Hi Javierm,

I've read through the patches and they look correct to me.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

But I had one question about the page size. You round up to multiples of 
page_size in several places. That could lead to an out-of-bounds access. 
Do you need to allocate GEM buffers to be multiples of page_size as well?

Best regards
Thomas

Am 05.06.23 um 09:47 schrieb Javier Martinez Canillas:
> Hello,
> 
> While working on adding support for the SSD132X family of 4-bit grayscale
> Solomon OLED panel controllers, I noticed a few things in the driver that
> can be improved and make extending to support other chip families easier.
> 
> I've split the preparatory patches in this series and will post the actual
> SSD132X support as a separate patch-set once this one is merged.
> 
> Best regards,
> Javier
> 
> 
> Javier Martinez Canillas (5):
>    drm/ssd130x: Make default width and height to be controller dependent
>    dt-bindings: display: ssd1307fb: Remove default width and height
>      values
>    drm/ssd130x: Set the page height value in the device info data
>    drm/ssd130x: Don't allocate buffers on each plane update
>    drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc()
> 
>   .../bindings/display/solomon,ssd1307fb.yaml   |   8 +-
>   drivers/gpu/drm/solomon/ssd130x.c             | 124 ++++++++++++------
>   drivers/gpu/drm/solomon/ssd130x.h             |   6 +
>   3 files changed, 93 insertions(+), 45 deletions(-)
>
Javier Martinez Canillas June 7, 2023, 7:17 a.m. UTC | #2
Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Hi Javierm,
>
> I've read through the patches and they look correct to me.
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>

Thanks a lot for your review!

> But I had one question about the page size. You round up to multiples of 
> page_size in several places. That could lead to an out-of-bounds access. 
> Do you need to allocate GEM buffers to be multiples of page_size as well?
>

That's a good point and I would need to have a closer look to the driver
to determine if that's needed or not as well. If that's the case though,
the issue is already present in the driver. We could fix it as follow-up.

> Best regards
> Thomas
>