Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set
On Thu, Aug 11, 2022 at 08:27:42PM +0200, Thomas Zimmermann wrote: > > > Am 11.08.22 um 20:26 schrieb Thomas Zimmermann: > > Hi Daniel > > > > Am 11.08.22 um 19:23 schrieb Daniel Vetter: > > > On Wed, 27 Jul 2022 at 09:53, Thomas Zimmermann > > > wrote: > > > > > > > > Hi > > > > > > > > Am 25.07.22 um 17:13 schrieb Javier Martinez Canillas: > > > > > Hello Geert, > > > > > > > > > > On 7/21/22 16:46, Geert Uytterhoeven wrote: > > > > > > Hi Thomas, > > > > > > > > > > > > On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann > > > > > > wrote: > > > > > > > Compute the framebuffer's scanline stride length if not given by > > > > > > > the simplefb data. > > > > > > > > > > > > > > Signed-off-by: Thomas Zimmermann > > > > > > > > > > > > Thanks for your patch! > > > > > > > > > > > > > --- a/drivers/gpu/drm/tiny/simpledrm.c > > > > > > > +++ b/drivers/gpu/drm/tiny/simpledrm.c > > > > > > > @@ -743,6 +743,9 @@ static struct simpledrm_device > > > > > > > *simpledrm_device_create(struct drm_driver *drv, > > > > > > > drm_err(dev, "no simplefb configuration > > > > > > > found\n"); > > > > > > > return ERR_PTR(-ENODEV); > > > > > > > } > > > > > > > + if (!stride) > > > > > > > + stride = format->cpp[0] * width; > > > > > > > > > > > > DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8) > > > > > > > > > > > > > > > > I think you meant here: > > > > > > > > > > DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ? > > > > > > > > I guess, that's the right function. My original code is correct, but cpp > > > > is also deprecated. > > > > > > You all mean drm_format_info_min_pitch(). > > > > Thanks a lot. I wasn't even aware of this function, but I had almost > > written my own implementation of it. I'll update the patch accordingly. > > Arghh, too late. I merged that patch already. Reviewed-by: Daniel Vetter Preemptively, if you can do the fixup patch (and it's not yet merged)? -Daniel > > > > > Best regards > > Thomas > > > > > > > > I really don't want drivers to go grab any of the legacy format info > > > fields like bpp or depth. switch() statements on the fourcc code for > > > programming registers, or one of the real helper functions in > > > drm_fourcc.c (there might be some gaps), but not ever going through > > > legacy concepts. Anything else just leads to subtle bugs when new > > > formats get added and oops suddenly the assumptions don't hold. > > > > > > Those should be strictly limited to legacy (i.e. not drm_fourcc aware) > > > interfaces. Heck I think even fbdev emulation should completely switch > > > over to drm_fourcc/drm_format_info, but alas that's a pile of work and > > > not much payoff. > > > > > > I'm trying to volunteer Same to add a legacy_bpp tag to the above > > > helper and appropriately limit it, I think limiting to formats with > > > depth!=0 is probably the right thing. And then we should probably > > > remove a pile of the cargo-culted depth!=0 entries too. > > > -Daniel > > > > > > > > > > > Best regards > > > > Thomas > > > > > > > > > > > > > > With that change, > > > > > > > > > > Acked-by: Javier Martinez Canillas > > > > > > > > > > > > > -- > > > > Thomas Zimmermann > > > > Graphics Driver Developer > > > > SUSE Software Solutions Germany GmbH > > > > Maxfeldstr. 5, 90409 Nürnberg, Germany > > > > (HRB 36809, AG Nürnberg) > > > > Geschäftsführer: Ivo Totev > > > > > > > > > > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set
Am 11.08.22 um 20:26 schrieb Thomas Zimmermann: Hi Daniel Am 11.08.22 um 19:23 schrieb Daniel Vetter: On Wed, 27 Jul 2022 at 09:53, Thomas Zimmermann wrote: Hi Am 25.07.22 um 17:13 schrieb Javier Martinez Canillas: Hello Geert, On 7/21/22 16:46, Geert Uytterhoeven wrote: Hi Thomas, On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann wrote: Compute the framebuffer's scanline stride length if not given by the simplefb data. Signed-off-by: Thomas Zimmermann Thanks for your patch! --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -743,6 +743,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv, drm_err(dev, "no simplefb configuration found\n"); return ERR_PTR(-ENODEV); } + if (!stride) + stride = format->cpp[0] * width; DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8) I think you meant here: DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ? I guess, that's the right function. My original code is correct, but cpp is also deprecated. You all mean drm_format_info_min_pitch(). Thanks a lot. I wasn't even aware of this function, but I had almost written my own implementation of it. I'll update the patch accordingly. Arghh, too late. I merged that patch already. Best regards Thomas I really don't want drivers to go grab any of the legacy format info fields like bpp or depth. switch() statements on the fourcc code for programming registers, or one of the real helper functions in drm_fourcc.c (there might be some gaps), but not ever going through legacy concepts. Anything else just leads to subtle bugs when new formats get added and oops suddenly the assumptions don't hold. Those should be strictly limited to legacy (i.e. not drm_fourcc aware) interfaces. Heck I think even fbdev emulation should completely switch over to drm_fourcc/drm_format_info, but alas that's a pile of work and not much payoff. I'm trying to volunteer Same to add a legacy_bpp tag to the above helper and appropriately limit it, I think limiting to formats with depth!=0 is probably the right thing. And then we should probably remove a pile of the cargo-culted depth!=0 entries too. -Daniel Best regards Thomas With that change, Acked-by: Javier Martinez Canillas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set
Hi Daniel Am 11.08.22 um 19:23 schrieb Daniel Vetter: On Wed, 27 Jul 2022 at 09:53, Thomas Zimmermann wrote: Hi Am 25.07.22 um 17:13 schrieb Javier Martinez Canillas: Hello Geert, On 7/21/22 16:46, Geert Uytterhoeven wrote: Hi Thomas, On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann wrote: Compute the framebuffer's scanline stride length if not given by the simplefb data. Signed-off-by: Thomas Zimmermann Thanks for your patch! --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -743,6 +743,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv, drm_err(dev, "no simplefb configuration found\n"); return ERR_PTR(-ENODEV); } + if (!stride) + stride = format->cpp[0] * width; DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8) I think you meant here: DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ? I guess, that's the right function. My original code is correct, but cpp is also deprecated. You all mean drm_format_info_min_pitch(). Thanks a lot. I wasn't even aware of this function, but I had almost written my own implementation of it. I'll update the patch accordingly. Best regards Thomas I really don't want drivers to go grab any of the legacy format info fields like bpp or depth. switch() statements on the fourcc code for programming registers, or one of the real helper functions in drm_fourcc.c (there might be some gaps), but not ever going through legacy concepts. Anything else just leads to subtle bugs when new formats get added and oops suddenly the assumptions don't hold. Those should be strictly limited to legacy (i.e. not drm_fourcc aware) interfaces. Heck I think even fbdev emulation should completely switch over to drm_fourcc/drm_format_info, but alas that's a pile of work and not much payoff. I'm trying to volunteer Same to add a legacy_bpp tag to the above helper and appropriately limit it, I think limiting to formats with depth!=0 is probably the right thing. And then we should probably remove a pile of the cargo-culted depth!=0 entries too. -Daniel Best regards Thomas With that change, Acked-by: Javier Martinez Canillas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set
On Wed, 27 Jul 2022 at 09:53, Thomas Zimmermann wrote: > > Hi > > Am 25.07.22 um 17:13 schrieb Javier Martinez Canillas: > > Hello Geert, > > > > On 7/21/22 16:46, Geert Uytterhoeven wrote: > >> Hi Thomas, > >> > >> On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann > >> wrote: > >>> Compute the framebuffer's scanline stride length if not given by > >>> the simplefb data. > >>> > >>> Signed-off-by: Thomas Zimmermann > >> > >> Thanks for your patch! > >> > >>> --- a/drivers/gpu/drm/tiny/simpledrm.c > >>> +++ b/drivers/gpu/drm/tiny/simpledrm.c > >>> @@ -743,6 +743,9 @@ static struct simpledrm_device > >>> *simpledrm_device_create(struct drm_driver *drv, > >>> drm_err(dev, "no simplefb configuration found\n"); > >>> return ERR_PTR(-ENODEV); > >>> } > >>> + if (!stride) > >>> + stride = format->cpp[0] * width; > >> > >> DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8) > >> > > > > I think you meant here: > > > > DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ? > > I guess, that's the right function. My original code is correct, but cpp > is also deprecated. You all mean drm_format_info_min_pitch(). I really don't want drivers to go grab any of the legacy format info fields like bpp or depth. switch() statements on the fourcc code for programming registers, or one of the real helper functions in drm_fourcc.c (there might be some gaps), but not ever going through legacy concepts. Anything else just leads to subtle bugs when new formats get added and oops suddenly the assumptions don't hold. Those should be strictly limited to legacy (i.e. not drm_fourcc aware) interfaces. Heck I think even fbdev emulation should completely switch over to drm_fourcc/drm_format_info, but alas that's a pile of work and not much payoff. I'm trying to volunteer Same to add a legacy_bpp tag to the above helper and appropriately limit it, I think limiting to formats with depth!=0 is probably the right thing. And then we should probably remove a pile of the cargo-culted depth!=0 entries too. -Daniel > > Best regards > Thomas > > > > > With that change, > > > > Acked-by: Javier Martinez Canillas > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set
Hi Am 25.07.22 um 17:13 schrieb Javier Martinez Canillas: Hello Geert, On 7/21/22 16:46, Geert Uytterhoeven wrote: Hi Thomas, On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann wrote: Compute the framebuffer's scanline stride length if not given by the simplefb data. Signed-off-by: Thomas Zimmermann Thanks for your patch! --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -743,6 +743,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv, drm_err(dev, "no simplefb configuration found\n"); return ERR_PTR(-ENODEV); } + if (!stride) + stride = format->cpp[0] * width; DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8) I think you meant here: DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ? I guess, that's the right function. My original code is correct, but cpp is also deprecated. Best regards Thomas With that change, Acked-by: Javier Martinez Canillas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set
Hello Geert, On 7/21/22 16:46, Geert Uytterhoeven wrote: > Hi Thomas, > > On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann wrote: >> Compute the framebuffer's scanline stride length if not given by >> the simplefb data. >> >> Signed-off-by: Thomas Zimmermann > > Thanks for your patch! > >> --- a/drivers/gpu/drm/tiny/simpledrm.c >> +++ b/drivers/gpu/drm/tiny/simpledrm.c >> @@ -743,6 +743,9 @@ static struct simpledrm_device >> *simpledrm_device_create(struct drm_driver *drv, >> drm_err(dev, "no simplefb configuration found\n"); >> return ERR_PTR(-ENODEV); >> } >> + if (!stride) >> + stride = format->cpp[0] * width; > > DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8) > I think you meant here: DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8) ? With that change, Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set
Hi Thomas,
On Wed, Jul 20, 2022 at 4:27 PM Thomas Zimmermann wrote:
> Compute the framebuffer's scanline stride length if not given by
> the simplefb data.
>
> Signed-off-by: Thomas Zimmermann
Thanks for your patch!
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -743,6 +743,9 @@ static struct simpledrm_device
> *simpledrm_device_create(struct drm_driver *drv,
> drm_err(dev, "no simplefb configuration found\n");
> return ERR_PTR(-ENODEV);
> }
> + if (!stride)
> + stride = format->cpp[0] * width;
DIV_ROUND_UP(drm_format_info_bpp(format) * width, 8)
> +
> sdev->mode = simpledrm_mode(width, height);
> sdev->format = format;
> sdev->pitch = stride;
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
[PATCH v2 04/10] drm/simpledrm: Compute framebuffer stride if not set
Compute the framebuffer's scanline stride length if not given by the simplefb data. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/tiny/simpledrm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c index 7de477835d44..cff9e7f71f80 100644 --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -743,6 +743,9 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv, drm_err(dev, "no simplefb configuration found\n"); return ERR_PTR(-ENODEV); } + if (!stride) + stride = format->cpp[0] * width; + sdev->mode = simpledrm_mode(width, height); sdev->format = format; sdev->pitch = stride; -- 2.36.1
