Re: [PATCH v2 10/10] drm/ofdrm: Support color management
Hi
Am 22.09.22 um 09:28 schrieb Maxime Ripard:
On Thu, Sep 22, 2022 at 08:42:23AM +0200, Thomas Zimmermann wrote:
Hi
Am 21.09.22 um 18:48 schrieb Geert Uytterhoeven:
Hi Thomas,
On Wed, Sep 21, 2022 at 2:55 PM Thomas Zimmermann wrote:
Am 05.08.22 um 02:19 schrieb Benjamin Herrenschmidt:
On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
+#if !defined(CONFIG_PPC)
+static inline void out_8(void __iomem *addr, int val)
+{ }
+static inline void out_le32(void __iomem *addr, int val)
+{ }
+static inline unsigned int in_le32(const void __iomem *addr)
+{
+ return 0;
+}
+#endif
These guys could just be replaced with readb/writel/readl respectively
(beware of the argument swap).
I only added them for COMPILE_TEST. There appears to be no portable
interface that implements out_le32() and in_le32()?
iowrite32() and ioread32()?
Do they always use little endian, as these *_le32 helpers do? I though they
use host byte order.
They use either outl or writel under the hood, which are always little-endian
I see. I'll replace the custom helpers.
Best regards
Thomas
Maxime
--
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 10/10] drm/ofdrm: Support color management
On Thu, Sep 22, 2022 at 08:42:23AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 21.09.22 um 18:48 schrieb Geert Uytterhoeven:
> > Hi Thomas,
> >
> > On Wed, Sep 21, 2022 at 2:55 PM Thomas Zimmermann
> > wrote:
> > > Am 05.08.22 um 02:19 schrieb Benjamin Herrenschmidt:
> > > > On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
> > > > > +#if !defined(CONFIG_PPC)
> > > > > +static inline void out_8(void __iomem *addr, int val)
> > > > > +{ }
> > > > > +static inline void out_le32(void __iomem *addr, int val)
> > > > > +{ }
> > > > > +static inline unsigned int in_le32(const void __iomem *addr)
> > > > > +{
> > > > > + return 0;
> > > > > +}
> > > > > +#endif
> > > >
> > > > These guys could just be replaced with readb/writel/readl respectively
> > > > (beware of the argument swap).
> > >
> > > I only added them for COMPILE_TEST. There appears to be no portable
> > > interface that implements out_le32() and in_le32()?
> >
> > iowrite32() and ioread32()?
>
> Do they always use little endian, as these *_le32 helpers do? I though they
> use host byte order.
They use either outl or writel under the hood, which are always little-endian
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2 10/10] drm/ofdrm: Support color management
Hi
Am 21.09.22 um 18:48 schrieb Geert Uytterhoeven:
Hi Thomas,
On Wed, Sep 21, 2022 at 2:55 PM Thomas Zimmermann wrote:
Am 05.08.22 um 02:19 schrieb Benjamin Herrenschmidt:
On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
+#if !defined(CONFIG_PPC)
+static inline void out_8(void __iomem *addr, int val)
+{ }
+static inline void out_le32(void __iomem *addr, int val)
+{ }
+static inline unsigned int in_le32(const void __iomem *addr)
+{
+ return 0;
+}
+#endif
These guys could just be replaced with readb/writel/readl respectively
(beware of the argument swap).
I only added them for COMPILE_TEST. There appears to be no portable
interface that implements out_le32() and in_le32()?
iowrite32() and ioread32()?
Do they always use little endian, as these *_le32 helpers do? I though
they use host byte order.
Best regards
Thomas
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
--
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 10/10] drm/ofdrm: Support color management
Hi Thomas,
On Wed, Sep 21, 2022 at 2:55 PM Thomas Zimmermann wrote:
> Am 05.08.22 um 02:19 schrieb Benjamin Herrenschmidt:
> > On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
> >> +#if !defined(CONFIG_PPC)
> >> +static inline void out_8(void __iomem *addr, int val)
> >> +{ }
> >> +static inline void out_le32(void __iomem *addr, int val)
> >> +{ }
> >> +static inline unsigned int in_le32(const void __iomem *addr)
> >> +{
> >> + return 0;
> >> +}
> >> +#endif
> >
> > These guys could just be replaced with readb/writel/readl respectively
> > (beware of the argument swap).
>
> I only added them for COMPILE_TEST. There appears to be no portable
> interface that implements out_le32() and in_le32()?
iowrite32() and ioread32()?
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
Re: [PATCH v2 10/10] drm/ofdrm: Support color management
Hi
Am 05.08.22 um 02:19 schrieb Benjamin Herrenschmidt:
On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
+#if !defined(CONFIG_PPC)
+static inline void out_8(void __iomem *addr, int val)
+{ }
+static inline void out_le32(void __iomem *addr, int val)
+{ }
+static inline unsigned int in_le32(const void __iomem *addr)
+{
+ return 0;
+}
+#endif
These guys could just be replaced with readb/writel/readl respectively
(beware of the argument swap).
I only added them for COMPILE_TEST. There appears to be no portable
interface that implements out_le32() and in_le32()?
Best regards
Thomas
Cheers,
Ben.
--
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 10/10] drm/ofdrm: Support color management
On Wed, 2022-07-27 at 10:41 +0200, Thomas Zimmermann wrote:
>
> > > +static void __iomem *ofdrm_mach64_cmap_ioremap(struct ofdrm_device *odev,
> > > +struct device_node *of_node,
> > > +u64 fb_base)
> > > +{
> > > + struct drm_device *dev = >dev;
> > > + u64 address;
> > > + void __iomem *cmap_base;
> > > +
> > > + address = fb_base & 0xff00ul;
> > > + address += 0x7ff000;
> > > +
> >
> > It would be good to know where these addresses are coming from. Maybe some
> > constant macros or a comment ? Same for the other places where addresses
> > and offsets are used.
>
> I have no idea where these values come from. I took them from offb. And
> I suspect that some of these CMAP helpers could be further merged if
> only it was clear where the numbers come from. But as i don't have the
> equipment for testing, I took most of this literally as-is from offb.
Ancient black magic :-) Old ATI mach64 chips had the registers sitting
at the end of the framebuffer. You can find an equivalent in
drivers/video/aty/atyfb_base.c:atyfb_setup_generic():
raddr = addr + 0x7ff000UL;
Cheers,
Ben.
Re: [PATCH v2 10/10] drm/ofdrm: Support color management
On Wed, 2022-07-20 at 16:27 +0200, Thomas Zimmermann wrote:
> +#if !defined(CONFIG_PPC)
> +static inline void out_8(void __iomem *addr, int val)
> +{ }
> +static inline void out_le32(void __iomem *addr, int val)
> +{ }
> +static inline unsigned int in_le32(const void __iomem *addr)
> +{
> + return 0;
> +}
> +#endif
These guys could just be replaced with readb/writel/readl respectively
(beware of the argument swap).
Cheers,
Ben.
Re: [PATCH v2 10/10] drm/ofdrm: Support color management
On 7/27/22 10:41, Thomas Zimmermann wrote:
[...]
>>
>>> +static void __iomem *ofdrm_mach64_cmap_ioremap(struct ofdrm_device *odev,
>>> + struct device_node *of_node,
>>> + u64 fb_base)
>>> +{
>>> + struct drm_device *dev = >dev;
>>> + u64 address;
>>> + void __iomem *cmap_base;
>>> +
>>> + address = fb_base & 0xff00ul;
>>> + address += 0x7ff000;
>>> +
>>
>> It would be good to know where these addresses are coming from. Maybe some
>> constant macros or a comment ? Same for the other places where addresses
>> and offsets are used.
>
> I have no idea where these values come from. I took them from offb. And
> I suspect that some of these CMAP helpers could be further merged if
> only it was clear where the numbers come from. But as i don't have the
> equipment for testing, I took most of this literally as-is from offb.
>
I see. As Michal mentioned maybe someone more familiar with this platform
could shed some light about these but in any case that could be done later.
[...]
>>> +
>>> + new_crtc_state = drm_atomic_get_new_crtc_state(new_state,
>>> new_plane_state->crtc);
>>> +
>>> + new_ofdrm_crtc_state = to_ofdrm_crtc_state(new_crtc_state);
>>> + new_ofdrm_crtc_state->format = new_fb->format;
>>> +
>>
>> Ah, I understand now why you didn't factor out the .atomic_check callbacks
>> for the two drivers in a fwfb helper. Maybe you can also add a comment to
>> mention that this updates the format so the CRTC palette can be applied in
>> the .atomic_flush callback ?
>
> Yeah, this code is one reason for not sharing atomic_check in fwfb. The
> other reason is that the fwfb code is only a wrapper around the atomic
> helpers with little extra value. I did have such fwfb helpers a some
> point, but removed them.
>
Got it.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v2 10/10] drm/ofdrm: Support color management
Hi
Am 26.07.22 um 15:49 schrieb Javier Martinez Canillas:
On 7/20/22 16:27, Thomas Zimmermann wrote:
Support the CRTC's color-management property and implement each model's
palette support.
The OF hardware has different methods of setting the palette. The
respective code has been taken from fbdev's offb and refactored into
per-model device functions. The device functions integrate this
functionality into the overall modesetting.
As palette handling is a CRTC property that depends on the primary
plane's color format, the plane's atomic_check helper now updates the
format field in ofdrm's custom CRTC state. The CRTC's atomic_flush
helper updates the palette for the format as needed.
Signed-off-by: Thomas Zimmermann
---
Reviewed-by: Javier Martinez Canillas
[...]
+static void __iomem *ofdrm_mach64_cmap_ioremap(struct ofdrm_device *odev,
+ struct device_node *of_node,
+ u64 fb_base)
+{
+ struct drm_device *dev = >dev;
+ u64 address;
+ void __iomem *cmap_base;
+
+ address = fb_base & 0xff00ul;
+ address += 0x7ff000;
+
It would be good to know where these addresses are coming from. Maybe some
constant macros or a comment ? Same for the other places where addresses
and offsets are used.
I have no idea where these values come from. I took them from offb. And
I suspect that some of these CMAP helpers could be further merged if
only it was clear where the numbers come from. But as i don't have the
equipment for testing, I took most of this literally as-is from offb.
[...]
static struct ofdrm_crtc_state *to_ofdrm_crtc_state(struct drm_crtc_state
*base)
@@ -376,10 +735,12 @@ static int ofdrm_primary_plane_helper_atomic_check(struct
drm_plane *plane,
struct drm_atomic_state
*new_state)
{
struct drm_plane_state *new_plane_state =
drm_atomic_get_new_plane_state(new_state, plane);
+ struct drm_framebuffer *new_fb = new_plane_state->fb;
struct drm_crtc_state *new_crtc_state;
+ struct ofdrm_crtc_state *new_ofdrm_crtc_state;
int ret;
- if (!new_plane_state->fb)
+ if (!new_fb)
return 0;
new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc);
@@ -391,6 +752,14 @@ static int ofdrm_primary_plane_helper_atomic_check(struct
drm_plane *plane,
if (ret)
return ret;
+ if (!new_plane_state->visible)
+ return 0;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(new_state,
new_plane_state->crtc);
+
+ new_ofdrm_crtc_state = to_ofdrm_crtc_state(new_crtc_state);
+ new_ofdrm_crtc_state->format = new_fb->format;
+
Ah, I understand now why you didn't factor out the .atomic_check callbacks
for the two drivers in a fwfb helper. Maybe you can also add a comment to
mention that this updates the format so the CRTC palette can be applied in
the .atomic_flush callback ?
Yeah, this code is one reason for not sharing atomic_check in fwfb. The
other reason is that the fwfb code is only a wrapper around the atomic
helpers with little extra value. I did have such fwfb helpers a some
point, but removed them.
Best regards
Thomas
--
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 10/10] drm/ofdrm: Support color management
On 7/20/22 16:27, Thomas Zimmermann wrote:
> Support the CRTC's color-management property and implement each model's
> palette support.
>
> The OF hardware has different methods of setting the palette. The
> respective code has been taken from fbdev's offb and refactored into
> per-model device functions. The device functions integrate this
> functionality into the overall modesetting.
>
> As palette handling is a CRTC property that depends on the primary
> plane's color format, the plane's atomic_check helper now updates the
> format field in ofdrm's custom CRTC state. The CRTC's atomic_flush
> helper updates the palette for the format as needed.
>
> Signed-off-by: Thomas Zimmermann
> ---
Reviewed-by: Javier Martinez Canillas
[...]
> +static void __iomem *ofdrm_mach64_cmap_ioremap(struct ofdrm_device *odev,
> +struct device_node *of_node,
> +u64 fb_base)
> +{
> + struct drm_device *dev = >dev;
> + u64 address;
> + void __iomem *cmap_base;
> +
> + address = fb_base & 0xff00ul;
> + address += 0x7ff000;
> +
It would be good to know where these addresses are coming from. Maybe some
constant macros or a comment ? Same for the other places where addresses
and offsets are used.
[...]
> static struct ofdrm_crtc_state *to_ofdrm_crtc_state(struct drm_crtc_state
> *base)
> @@ -376,10 +735,12 @@ static int
> ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> struct drm_atomic_state
> *new_state)
> {
> struct drm_plane_state *new_plane_state =
> drm_atomic_get_new_plane_state(new_state, plane);
> + struct drm_framebuffer *new_fb = new_plane_state->fb;
> struct drm_crtc_state *new_crtc_state;
> + struct ofdrm_crtc_state *new_ofdrm_crtc_state;
> int ret;
>
> - if (!new_plane_state->fb)
> + if (!new_fb)
> return 0;
>
> new_crtc_state = drm_atomic_get_new_crtc_state(new_state,
> new_plane_state->crtc);
> @@ -391,6 +752,14 @@ static int
> ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> if (ret)
> return ret;
>
> + if (!new_plane_state->visible)
> + return 0;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(new_state,
> new_plane_state->crtc);
> +
> + new_ofdrm_crtc_state = to_ofdrm_crtc_state(new_crtc_state);
> + new_ofdrm_crtc_state->format = new_fb->format;
> +
Ah, I understand now why you didn't factor out the .atomic_check callbacks
for the two drivers in a fwfb helper. Maybe you can also add a comment to
mention that this updates the format so the CRTC palette can be applied in
the .atomic_flush callback ?
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
[PATCH v2 10/10] drm/ofdrm: Support color management
Support the CRTC's color-management property and implement each model's
palette support.
The OF hardware has different methods of setting the palette. The
respective code has been taken from fbdev's offb and refactored into
per-model device functions. The device functions integrate this
functionality into the overall modesetting.
As palette handling is a CRTC property that depends on the primary
plane's color format, the plane's atomic_check helper now updates the
format field in ofdrm's custom CRTC state. The CRTC's atomic_flush
helper updates the palette for the format as needed.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/tiny/ofdrm.c | 434 ++-
1 file changed, 433 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
index 62136b8ab975..2964eb9951c5 100644
--- a/drivers/gpu/drm/tiny/ofdrm.c
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -29,6 +30,44 @@
#define DRIVER_MAJOR 1
#define DRIVER_MINOR 0
+#if !defined(CONFIG_PPC)
+static inline void out_8(void __iomem *addr, int val)
+{ }
+static inline void out_le32(void __iomem *addr, int val)
+{ }
+static inline unsigned int in_le32(const void __iomem *addr)
+{
+ return 0;
+}
+#endif
+
+#define OFDRM_GAMMA_LUT_SIZE 256
+
+/* Definitions used by the Avivo palette */
+#define AVIVO_DC_LUT_RW_SELECT 0x6480
+#define AVIVO_DC_LUT_RW_MODE0x6484
+#define AVIVO_DC_LUT_RW_INDEX 0x6488
+#define AVIVO_DC_LUT_SEQ_COLOR 0x648c
+#define AVIVO_DC_LUT_PWL_DATA 0x6490
+#define AVIVO_DC_LUT_30_COLOR 0x6494
+#define AVIVO_DC_LUT_READ_PIPE_SELECT 0x6498
+#define AVIVO_DC_LUT_WRITE_EN_MASK 0x649c
+#define AVIVO_DC_LUT_AUTOFILL 0x64a0
+#define AVIVO_DC_LUTA_CONTROL 0x64c0
+#define AVIVO_DC_LUTA_BLACK_OFFSET_BLUE 0x64c4
+#define AVIVO_DC_LUTA_BLACK_OFFSET_GREEN0x64c8
+#define AVIVO_DC_LUTA_BLACK_OFFSET_RED 0x64cc
+#define AVIVO_DC_LUTA_WHITE_OFFSET_BLUE 0x64d0
+#define AVIVO_DC_LUTA_WHITE_OFFSET_GREEN0x64d4
+#define AVIVO_DC_LUTA_WHITE_OFFSET_RED 0x64d8
+#define AVIVO_DC_LUTB_CONTROL 0x6cc0
+#define AVIVO_DC_LUTB_BLACK_OFFSET_BLUE 0x6cc4
+#define AVIVO_DC_LUTB_BLACK_OFFSET_GREEN0x6cc8
+#define AVIVO_DC_LUTB_BLACK_OFFSET_RED 0x6ccc
+#define AVIVO_DC_LUTB_WHITE_OFFSET_BLUE 0x6cd0
+#define AVIVO_DC_LUTB_WHITE_OFFSET_GREEN0x6cd4
+#define AVIVO_DC_LUTB_WHITE_OFFSET_RED 0x6cd8
+
enum ofdrm_model {
OFDRM_MODEL_UNKNOWN,
OFDRM_MODEL_MACH64, /* ATI Mach64 */
@@ -209,7 +248,14 @@ static enum ofdrm_model display_get_model_of(struct
drm_device *dev, struct devi
* Open Firmware display device
*/
+struct ofdrm_device;
+
struct ofdrm_device_funcs {
+ void __iomem *(*cmap_ioremap)(struct ofdrm_device *odev,
+ struct device_node *of_node,
+ u64 fb_bas);
+ void (*cmap_write)(struct ofdrm_device *odev, unsigned char index,
+ unsigned char r, unsigned char g, unsigned char b);
};
struct ofdrm_device {
@@ -221,6 +267,9 @@ struct ofdrm_device {
/* firmware framebuffer */
struct drm_fwfb fwfb;
+ /* colormap */
+ void __iomem *cmap_base;
+
/* modesetting */
uint32_t formats[8];
struct drm_plane primary_plane;
@@ -333,12 +382,322 @@ static struct resource *ofdrm_find_fb_resource(struct
ofdrm_device *odev,
return max_res;
}
+/*
+ * Colormap / Palette
+ */
+
+static void __iomem *get_cmap_address_of(struct ofdrm_device *odev, struct
device_node *of_node,
+int bar_no, unsigned long offset,
unsigned long size)
+{
+ struct drm_device *dev = >dev;
+ const __be32 *addr_p;
+ u64 max_size, address;
+ unsigned int flags;
+ void __iomem *mem;
+
+ addr_p = of_get_pci_address(of_node, bar_no, _size, );
+ if (!addr_p)
+ addr_p = of_get_address(of_node, bar_no, _size, );
+ if (!addr_p)
+ return ERR_PTR(-ENODEV);
+
+ if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
+ return ERR_PTR(-ENODEV);
+
+ if ((offset + size) >= max_size)
+ return ERR_PTR(-ENODEV);
+
+ address = of_translate_address(of_node, addr_p);
+ if (address == OF_BAD_ADDR)
+ return ERR_PTR(-ENODEV);
+
+ mem = devm_ioremap(dev->dev, address + offset, size);
+ if (!mem)
+ return ERR_PTR(-ENOMEM);
+
+ return mem;
+}
+
+static void __iomem *ofdrm_mach64_cmap_ioremap(struct ofdrm_device *odev,
+ struct device_node *of_node,
+
