Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion
On 01/11/2022 01.15, Justin Forbes wrote:
> On Thu, Oct 27, 2022 at 8:57 AM Hector Martin wrote:
>>
>> drm_fb_build_fourcc_list() currently returns all emulated formats
>> unconditionally as long as the native format is among them, even though
>> not all combinations have conversion helpers. Although the list is
>> arguably provided to userspace in precedence order, userspace can pick
>> something out-of-order (and thus break when it shouldn't), or simply
>> only support a format that is unsupported (and thus think it can work,
>> which results in the appearance of a hang as FB blits fail later on,
>> instead of the initialization error you'd expect in this case).
>>
>> Add checks to filter the list of emulated formats to only those
>> supported for conversion to the native format. This presumes that there
>> is a single native format (only the first is checked, if there are
>> multiple). Refactoring this API to drop the native list or support it
>> properly (by returning the appropriate emulated->native mapping table)
>> is left for a future patch.
>>
>> The simpledrm driver is left as-is with a full table of emulated
>> formats. This keeps all currently working conversions available and
>> drops all the broken ones (i.e. this a strict bugfix patch, adding no
>> new supported formats nor removing any actually working ones). In order
>> to avoid proliferation of emulated formats, future drivers should
>> advertise only XRGB as the sole emulated format (since some
>> userspace assumes its presence).
>>
>> This fixes a real user regression where the ?RGB2101010 support commit
>> started advertising it unconditionally where not supported, and KWin
>> decided to start to use it over the native format and broke, but also
>> the fixes the spurious RGB565/RGB888 formats which have been wrongly
>> unconditionally advertised since the dawn of simpledrm.
>>
>> Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB210101
>
>
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Hector Martin
>
> There is a CC for stable on here, but this patch does not apply in any
> way on 6.0 or older kernels as the fourcc bits and considerable churn
> came in with the 6.1 merge window. You don't happen to have a
> backport of this to 6.0 do you?
v1 is probably closer to such a backport, and I offered to figure it out
on Matrix but I heard you're already working on it ;)
- Hector
Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion
On 28/10/2022 17.07, Thomas Zimmermann wrote: > In yesterday's discussion on IRC, it was said that several devices > advertise ARGB framebuffers when the hardware actually uses XRGB? Is > there hardware that supports transparent primary planes? ARGB hardware probably exists in the form of embedded systems with preconfigured blending. For example, one could imagine an OSD-type setup where there is a hardware video scaler controlled entirely outside of DRM/KMS (probably by a horrible vendor driver), and the overlay framebuffer is exposed via simpledrm as a dumb memory region, and expects ARGB to work. So ideally, we wouldn't expose XRGB on ARGB systems. But there is this problem: arch/arm64/boot/dts/qcom/msm8998-oneplus-common.dtsi: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sdm630-sony-xperia-nile.dtsi: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sdm660-xiaomi-lavender.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sdm850-samsung-w737.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm6350-sony-xperia-lena-pdx213.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm8150-sony-xperia-kumano.dtsi: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm8250-sony-xperia-edo.dtsi: format = "a8r8g8b8"; arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami.dtsi: format = "a8r8g8b8"; arch/arm64/boot/dts/socionext/uniphier-ld20-akebi96.dts: format = "a8r8g8b8"; I'm pretty sure those phones don't have transparent screens, nor magically put video planes below the firmware framebuffer. If there are 12 device trees for phones in mainline which lie about having alpha support, who knows how many more exist outside? If we stop advertising pretend-XRGB on them, I suspect we're going to break a lot of software... Of course, there is one "correct" solution here: have an actual xrgb->argb conversion helper that just clears the high byte. Then those platforms lying about having alpha and using xrgb from userspace will take a performace hit, but they should arguably just fix their device tree in that case. Maybe this is the way to go in this case? Note that there would be no inverse conversion (no advertising argb on xrgb backends), so that one would be dropped vs. what we have today. This effectively keeps the "xrgb helpers and nothing else" rule while actually supporting it for argb backend framebuffers correctly. Any platforms actually wanting to use argb framebuffers with meaningful alpha should be configuring their userspace to preferentially render directly to argb to avoid the perf hit anyway. - Hector
[PATCH v2] drm/format-helper: Only advertise supported formats for conversion
drm_fb_build_fourcc_list() currently returns all emulated formats
unconditionally as long as the native format is among them, even though
not all combinations have conversion helpers. Although the list is
arguably provided to userspace in precedence order, userspace can pick
something out-of-order (and thus break when it shouldn't), or simply
only support a format that is unsupported (and thus think it can work,
which results in the appearance of a hang as FB blits fail later on,
instead of the initialization error you'd expect in this case).
Add checks to filter the list of emulated formats to only those
supported for conversion to the native format. This presumes that there
is a single native format (only the first is checked, if there are
multiple). Refactoring this API to drop the native list or support it
properly (by returning the appropriate emulated->native mapping table)
is left for a future patch.
The simpledrm driver is left as-is with a full table of emulated
formats. This keeps all currently working conversions available and
drops all the broken ones (i.e. this a strict bugfix patch, adding no
new supported formats nor removing any actually working ones). In order
to avoid proliferation of emulated formats, future drivers should
advertise only XRGB as the sole emulated format (since some
userspace assumes its presence).
This fixes a real user regression where the ?RGB2101010 support commit
started advertising it unconditionally where not supported, and KWin
decided to start to use it over the native format and broke, but also
the fixes the spurious RGB565/RGB888 formats which have been wrongly
unconditionally advertised since the dawn of simpledrm.
Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
Cc: sta...@vger.kernel.org
Signed-off-by: Hector Martin
---
I'm proposing this alternative approach after a heated discussion on
IRC. I'm out of ideas, if y'all don't like this one you can figure it
out for yourseves :-)
Changes since v1:
This v2 moves all the changes to the helper (so they will apply to
the upcoming ofdrm, though ofdrm also needs to be fixed to trim its
format table to only formats that should be emulated, probably only
XRGB, to avoid further proliferating the use of conversions),
and avoids touching more than one file. The API still needs cleanup
as mentioned (supporting more than one native format is fundamentally
broken, since the helper would need to tell the driver *what* native
format to use for *each* emulated format somehow), but all current and
planned users only pass in one native format, so this can (and should)
be fixed later.
Aside: After other IRC discussion, I'm testing nuking the
XRGB2101010 <-> ARGB2101010 advertisement (which does not involve
conversion) by removing those entries from simpledrm in the Asahi Linux
downstream tree. As far as I'm concerned, it can be removed if nobody
complains (by removing those entries from the simpledrm array), if
maintainers are generally okay with removing advertised formats at all.
If so, there might be other opportunities for further trimming the list
non-native formats advertised to userspace.
Tested with KWin-X11, KWin-Wayland, GNOME-X11, GNOME-Wayland, and Weston
on both XRGB2101010 and RGB simpledrm framebuffers.
drivers/gpu/drm/drm_format_helper.c | 66 -
1 file changed, 47 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_format_helper.c
b/drivers/gpu/drm/drm_format_helper.c
index e2f76621453c..3ee59bae9d2f 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -807,6 +807,38 @@ static bool is_listed_fourcc(const uint32_t *fourccs,
size_t nfourccs, uint32_t
return false;
}
+static const uint32_t conv_from_xrgb[] = {
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_ARGB,
+ DRM_FORMAT_XRGB2101010,
+ DRM_FORMAT_ARGB2101010,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_RGB888,
+};
+
+static const uint32_t conv_from_rgb565_888[] = {
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_ARGB,
+};
+
+static bool is_conversion_supported(uint32_t from, uint32_t to)
+{
+ switch (from) {
+ case DRM_FORMAT_XRGB:
+ case DRM_FORMAT_ARGB:
+ return is_listed_fourcc(conv_from_xrgb,
ARRAY_SIZE(conv_from_xrgb), to);
+ case DRM_FORMAT_RGB565:
+ case DRM_FORMAT_RGB888:
+ return is_listed_fourcc(conv_from_rgb565_888,
ARRAY_SIZE(conv_from_rgb565_888), to);
+ case DRM_FORMAT_XRGB2101010:
+ return to == DRM_FORMAT_ARGB2101010;
+ case DRM_FORMAT_ARGB2101010:
+ return to == DRM_FORMAT_XRGB2101010;
+ default:
+ return false;
+ }
+}
+
/**
* drm_fb_build_fourcc_list - Filters a list of supported color formats against
*
Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
On 27/10/2022 20.08, Thomas Zimmermann wrote:
> We currently have two DRM drivers that call drm_fb_build_fourcc_list():
> simpledrm and ofdrm. I've been very careful to keep the format selection
> in sync between them. (That's the reason why the helper exists at all.)
> If the drivers start to use different logic, it will only become more
> chaotic.
>
> The format array of ofdrm is at [1]. At a minimum, ofdrm should get the
> same fix as simpledrm.
Looks like this was merged recently, so I didn't see it on my tree (I
was basing off of 6.1-rc2).
Since this patch is a regression fix, it should be applied to drm-fixes
(and automatically picked up by stable folks) soon to be fixed in 6.1,
and then we can fix whatever is needed in ofdrm separately in drm-tip.
As long as ofdrm is ready for the new behavior prior to the merge of
drm-tip with 6.1, there will be no breakage.
In this case, the change required to ofdrm is probably just to replace
that array with just DRM_FORMAT_XRGB (which should be the only
supported fallback format for new drivers) and then to add a test to
only expose it for formats for which we actually have conversion
helpers, similar to what the the switch() enumerates here. That logic
could later be moved into the helper as a refactor.
>> /* Primary plane */
>> +switch (format->format) {
>
> I trust you when you say that ->XRGB is not enough. But
> although I've read your replies, I still don't understand why this
> switch is necessary.
>
> Why don't we call drm_fb_build_fourcc_list() with the native
> format/formats and let it append a number of formats, such as adding
> XRGB888, adding ARGB if necessary, adding ARGB2101010 if necessary.
> Each with a elaborate comment why and which userspace needs the format. (?)
That would be fine to do, it would just be moving the logic to the
helper. That kind of refactoring is better suited for subsequent
patches. This is a regression fix, it attempts to minimize the amount of
refactoring, which means keeping the logic in simpledrm, to make it
easier to review for correctness.
Also, that would change the API of that function, which would likely
make the merge with the new ofdrm painful. The way things are now, a
small fix to ofdrm will make it compatible with both the state before
and after this patch, which means the merge will go through painlessly,
and then we can just refactor everything once everything is in the same
tree.
- Hector
Re: [PATCH] drm/simpledrm: Only advertise formats that are supported
On 27/10/2022 19.13, Hector Martin wrote:
> Until now, simpledrm unconditionally advertised all formats that can be
> supported natively as conversions. However, we don't actually have a
> full conversion matrix of helpers. Although the list is arguably
> provided to userspace in precedence order, userspace can pick something
> out-of-order (and thus break when it shouldn't), or simply only support
> a format that is unsupported (and thus think it can work, which results
> in the appearance of a hang as FB blits fail later on, instead of the
> initialization error you'd expect in this case).
>
> Split up the format table into separate ones for each required subset,
> and then pick one based on the native format. Also remove the
> native<->conversion overlap check from the helper (which doesn't make
> sense any more, since the native format is advertised anyway and this
> way RGB565/RGB888 can share a format table), and instead print the same
> message in simpledrm when the native format is not one for which we have
> conversions at all.
>
> This fixes a real user regression where the ?RGB2101010 support commit
> started advertising it unconditionally where not supported, and KWin
> decided to start to use it over the native format, but also the fixes
> the spurious RGB565/RGB888 formats which have been wrongly
> unconditionally advertised since the dawn of simpledrm.
>
> Note: this patch is merged because splitting it into two patches, one
> for the helper and one for simpledrm, would regress at the midpoint
> regardless of the order. If simpledrm is changed first, that would break
> working conversions to RGB565/RGB888 (since those share a table that
> does not include the native formats). If the helper is changed first, it
> would start spuriously advertising all conversion formats when the
> native format doesn't have any supported conversions at all.
>
> Acked-by: Pekka Paalanen
> Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
> Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Hector Martin
> ---
> drivers/gpu/drm/drm_format_helper.c | 15 ---
> drivers/gpu/drm/tiny/simpledrm.c| 62 +
> 2 files changed, 55 insertions(+), 22 deletions(-)
>
To answer some issues that came up on IRC:
Q: Why not move this logic / the tables to the helper?
A: Because simpledrm is the only user so far, and this patch is Cc:
stable because we have an actual regression that broke KDE. I'm going
for the minimal patch that keeps everything that worked to this day
working, and stops advertising things that never worked, no more, no
less. Future refactoring can always happen later (and is probably a good
idea when other drivers start using the helper).
Q: XRGB is supposed to be the only canonical format. Why not just
drop everything but conversions to/from XRGB?
A: Because that would regress things that work today, and could break
existing userspace on some platforms. That may be a good idea, but I
think we should fix the bugs first, and leave the discussion of whether
we want to actually remove existing functionality for later.
Q: Why not just add a conversion from XRGB2101010 to XRGB?
A: Because that would only fix KDE, and would make it slower vs. not
advertising XRGB2101010 at all (double conversions, plus kernel
conversion can be slower). Plus, it doesn't make any sense as it only
fills in one entry in the conversion matrix. If we wanted to actually
fill out the conversion matrix, and thus support everything simpledrm
has advertised to day correctly, we would need helpers for:
rgb565->rgb888
rgb888->rgb565
rgb565->xrgb2101010
rgb888->xrgb2101010
xrgb2101010->rgb565
xrgb2101010->rgb888
xrgb2101010->xrgb
That seems like overkill and unlikely to actually help anyone, it'd just
give userspace more options to shoot itself in the foot with a
sub-optimal format choice. And it's a pile of code.
- Hector
[PATCH] drm/simpledrm: Only advertise formats that are supported
Until now, simpledrm unconditionally advertised all formats that can be
supported natively as conversions. However, we don't actually have a
full conversion matrix of helpers. Although the list is arguably
provided to userspace in precedence order, userspace can pick something
out-of-order (and thus break when it shouldn't), or simply only support
a format that is unsupported (and thus think it can work, which results
in the appearance of a hang as FB blits fail later on, instead of the
initialization error you'd expect in this case).
Split up the format table into separate ones for each required subset,
and then pick one based on the native format. Also remove the
native<->conversion overlap check from the helper (which doesn't make
sense any more, since the native format is advertised anyway and this
way RGB565/RGB888 can share a format table), and instead print the same
message in simpledrm when the native format is not one for which we have
conversions at all.
This fixes a real user regression where the ?RGB2101010 support commit
started advertising it unconditionally where not supported, and KWin
decided to start to use it over the native format, but also the fixes
the spurious RGB565/RGB888 formats which have been wrongly
unconditionally advertised since the dawn of simpledrm.
Note: this patch is merged because splitting it into two patches, one
for the helper and one for simpledrm, would regress at the midpoint
regardless of the order. If simpledrm is changed first, that would break
working conversions to RGB565/RGB888 (since those share a table that
does not include the native formats). If the helper is changed first, it
would start spuriously advertising all conversion formats when the
native format doesn't have any supported conversions at all.
Acked-by: Pekka Paalanen
Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
Cc: sta...@vger.kernel.org
Signed-off-by: Hector Martin
---
drivers/gpu/drm/drm_format_helper.c | 15 ---
drivers/gpu/drm/tiny/simpledrm.c| 62 +
2 files changed, 55 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_format_helper.c
b/drivers/gpu/drm/drm_format_helper.c
index e2f76621453c..c60c13f3a872 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -864,20 +864,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
++fourccs;
}
- /*
-* The plane's atomic_update helper converts the framebuffer's color
format
-* to a native format when copying to device memory.
-*
-* If there is not a single format supported by both, device and
-* driver, the native formats are likely not supported by the conversion
-* helpers. Therefore *only* support the native formats and add a
-* conversion helper ASAP.
-*/
- if (!found_native) {
- drm_warn(dev, "Format conversion helpers required to add extra
formats.\n");
- goto out;
- }
-
/*
* The extra formats, emulated by the driver, go second.
*/
@@ -898,7 +884,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
++fourccs;
}
-out:
return fourccs - fourccs_out;
}
EXPORT_SYMBOL(drm_fb_build_fourcc_list);
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 18489779fb8a..1257411f3d44 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -446,22 +446,48 @@ static int simpledrm_device_init_regulators(struct
simpledrm_device *sdev)
*/
/*
- * Support all formats of simplefb and maybe more; in order
- * of preference. The display's update function will do any
+ * Support the subset of formats that we have conversion helpers for,
+ * in order of preference. The display's update function will do any
* conversion necessary.
*
* TODO: Add blit helpers for remaining formats and uncomment
* constants.
*/
-static const uint32_t simpledrm_primary_plane_formats[] = {
+
+/*
+ * Supported conversions to RGB565 and RGB888:
+ * from [AX]RGB
+ */
+static const uint32_t simpledrm_primary_plane_formats_base[] = {
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_ARGB,
+};
+
+/*
+ * Supported conversions to [AX]RGB:
+ * A/X variants (no-op)
+ * from RGB565
+ * from RGB888
+ */
+static const uint32_t simpledrm_primary_plane_formats_xrgb[] = {
DRM_FORMAT_XRGB,
DRM_FORMAT_ARGB,
+ DRM_FORMAT_RGB888,
DRM_FORMAT_RGB565,
//DRM_FORMAT_XRGB1555,
//DRM_FORMAT_ARGB1555,
- DRM_FORMAT_RGB888,
+};
+
+/*
+ * Supported conversions to [AX]RGB2101010:
+ * A/X variants (no-op)
+ * from [AX]RGB
+ */
+static const uint32_t simpledrm_primary_plane_formats
Re: [PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of
On 13/12/2021 20.30, Javier Martinez Canillas wrote: On Mon, Dec 13, 2021 at 11:46 AM Hector Martin wrote: On 13/12/2021 17.44, Javier Martinez Canillas wrote: Hello Hector, On Sun, Dec 12, 2021 at 7:24 AM Hector Martin wrote: This code is required for both simplefb and simpledrm, so let's move it into the OF core instead of having it as an ad-hoc initcall in the drivers. Acked-by: Thomas Zimmermann Signed-off-by: Hector Martin --- drivers/of/platform.c | 4 drivers/video/fbdev/simplefb.c | 21 + 2 files changed, 5 insertions(+), 20 deletions(-) This is indeed a much better approach than what I suggested. I just have one comment. diff --git a/drivers/of/platform.c b/drivers/of/platform.c index b3faf89744aa..793350028906 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void) of_node_put(node); } + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); You have to check if the node variable is NULL here. + of_platform_device_create(node, NULL, NULL); Otherwise this could lead to a NULL pointer dereference if debug output is enabled (the node->full_name is printed). Where is it printed? I thought I might need a NULL check, but this code Sorry, I misread of_amba_device_create() as of_platform_device_create(), which uses the "%pOF" printk format specifier [0] to print the node's full name as a debug output [1]. [0]: https://elixir.bootlin.com/linux/v5.16-rc5/source/Documentation/core-api/printk-formats.rst#L462 [1]: https://elixir.bootlin.com/linux/v5.16-rc5/source/drivers/of/platform.c#L233 was suggested verbatim by Rob in v2 without the NULL check and digging through I found that the NULL codepath is safe. You are right that passing NULL is a safe code path for now due the of_device_is_available(node) check, but that seems fragile to me since just adding a similar debug output to of_platform_device_create() could trigger the NULL pointer dereference. Since Rob is the DT maintainer, I'm going to defer to his opinion on this one :-) -- Hector Martin (mar...@marcan.st) Public Key: https://mrcn.st/pub
Re: [PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of
On 13/12/2021 17.44, Javier Martinez Canillas wrote:
Hello Hector,
On Sun, Dec 12, 2021 at 7:24 AM Hector Martin wrote:
This code is required for both simplefb and simpledrm, so let's move it
into the OF core instead of having it as an ad-hoc initcall in the
drivers.
Acked-by: Thomas Zimmermann
Signed-off-by: Hector Martin
---
drivers/of/platform.c | 4
drivers/video/fbdev/simplefb.c | 21 +
2 files changed, 5 insertions(+), 20 deletions(-)
This is indeed a much better approach than what I suggested. I just
have one comment.
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b3faf89744aa..793350028906 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void)
of_node_put(node);
}
+ node = of_get_compatible_child(of_chosen, "simple-framebuffer");
You have to check if the node variable is NULL here.
+ of_platform_device_create(node, NULL, NULL);
Otherwise this could lead to a NULL pointer dereference if debug
output is enabled (the node->full_name is printed).
Where is it printed? I thought I might need a NULL check, but this code
was suggested verbatim by Rob in v2 without the NULL check and digging
through I found that the NULL codepath is safe.
of_platform_device_create calls of_platform_device_create_pdata
directly, and:
static struct platform_device *of_platform_device_create_pdata(
struct device_node *np,
const char *bus_id,
void *platform_data,
struct device *parent)
{
struct platform_device *dev;
if (!of_device_is_available(np) ||
of_node_test_and_set_flag(np, OF_POPULATED))
return NULL;
of_device_is_available takes a global spinlock and then calls
__of_device_is_available, and that does:
static bool __of_device_is_available(const struct device_node *device)
{
const char *status;
int statlen;
if (!device)
return false;
... so I don't see how this can do anything but immediately return false
if node is NULL.
--
Hector Martin (mar...@marcan.st)
Public Key: https://mrcn.st/pub
[PATCH v3 3/3] drm/simpledrm: Add [AX]RGB2101010 formats
This is the format used by the bootloader framebuffer on Apple ARM64
platforms.
Reviewed-by: Thomas Zimmermann
Signed-off-by: Hector Martin
---
drivers/gpu/drm/tiny/simpledrm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 2f15b9aa..b977f5c94562 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -571,8 +571,8 @@ static const uint32_t simpledrm_default_formats[] = {
//DRM_FORMAT_XRGB1555,
//DRM_FORMAT_ARGB1555,
DRM_FORMAT_RGB888,
- //DRM_FORMAT_XRGB2101010,
- //DRM_FORMAT_ARGB2101010,
+ DRM_FORMAT_XRGB2101010,
+ DRM_FORMAT_ARGB2101010,
};
static const uint64_t simpledrm_format_modifiers[] = {
--
2.33.0
[PATCH v3 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_toio()
Add XRGB emulation support for devices that can only do XRGB2101010.
This is chiefly useful for simpledrm on Apple devices where the
bootloader-provided framebuffer is 10-bit.
Signed-off-by: Hector Martin
---
drivers/gpu/drm/drm_format_helper.c | 64 +
include/drm/drm_format_helper.h | 3 ++
2 files changed, 67 insertions(+)
diff --git a/drivers/gpu/drm/drm_format_helper.c
b/drivers/gpu/drm/drm_format_helper.c
index dbe3e830096e..0f28dd2bdd72 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -409,6 +409,61 @@ void drm_fb_xrgb_to_rgb888_toio(void __iomem *dst,
unsigned int dst_pitch,
}
EXPORT_SYMBOL(drm_fb_xrgb_to_rgb888_toio);
+static void drm_fb_xrgb_to_xrgb2101010_line(u32 *dbuf, const u32 *sbuf,
+ unsigned int pixels)
+{
+ unsigned int x;
+ u32 val32;
+
+ for (x = 0; x < pixels; x++) {
+ val32 = ((sbuf[x] & 0x00FF) << 2) |
+ ((sbuf[x] & 0xFF00) << 4) |
+ ((sbuf[x] & 0x00FF) << 6);
+ *dbuf++ = val32 | ((val32 >> 8) & 0x00300C03);
+ }
+}
+
+/**
+ * drm_fb_xrgb_to_xrgb2101010_toio - Convert XRGB to XRGB2101010 clip
+ * buffer
+ * @dst: XRGB2101010 destination buffer (iomem)
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
+ * @vaddr: XRGB source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ *
+ * Drivers can use this function for XRGB2101010 devices that don't natively
+ * support XRGB.
+ */
+void drm_fb_xrgb_to_xrgb2101010_toio(void __iomem *dst,
+unsigned int dst_pitch, const void
*vaddr,
+const struct drm_framebuffer *fb,
+const struct drm_rect *clip)
+{
+ size_t linepixels = clip->x2 - clip->x1;
+ size_t dst_len = linepixels * sizeof(u32);
+ unsigned int y, lines = clip->y2 - clip->y1;
+ void *dbuf;
+
+ if (!dst_pitch)
+ dst_pitch = dst_len;
+
+ dbuf = kmalloc(dst_len, GFP_KERNEL);
+ if (!dbuf)
+ return;
+
+ vaddr += clip_offset(clip, fb->pitches[0], sizeof(u32));
+ for (y = 0; y < lines; y++) {
+ drm_fb_xrgb_to_xrgb2101010_line(dbuf, vaddr, linepixels);
+ memcpy_toio(dst, dbuf, dst_len);
+ vaddr += fb->pitches[0];
+ dst += dst_pitch;
+ }
+
+ kfree(dbuf);
+}
+EXPORT_SYMBOL(drm_fb_xrgb_to_xrgb2101010_toio);
+
/**
* drm_fb_xrgb_to_gray8 - Convert XRGB to grayscale
* @dst: 8-bit grayscale destination buffer
@@ -500,6 +555,10 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int
dst_pitch, uint32_t dst_for
fb_format = DRM_FORMAT_XRGB;
if (dst_format == DRM_FORMAT_ARGB)
dst_format = DRM_FORMAT_XRGB;
+ if (fb_format == DRM_FORMAT_ARGB2101010)
+ fb_format = DRM_FORMAT_XRGB2101010;
+ if (dst_format == DRM_FORMAT_ARGB2101010)
+ dst_format = DRM_FORMAT_XRGB2101010;
if (dst_format == fb_format) {
drm_fb_memcpy_toio(dst, dst_pitch, vmap, fb, clip);
@@ -515,6 +574,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int
dst_pitch, uint32_t dst_for
drm_fb_xrgb_to_rgb888_toio(dst, dst_pitch, vmap,
fb, clip);
return 0;
}
+ } else if (dst_format == DRM_FORMAT_XRGB2101010) {
+ if (fb_format == DRM_FORMAT_XRGB) {
+ drm_fb_xrgb_to_xrgb2101010_toio(dst, dst_pitch,
vmap, fb, clip);
+ return 0;
+ }
}
return -EINVAL;
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 97e4c3223af3..b30ed5de0a33 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -33,6 +33,9 @@ void drm_fb_xrgb_to_rgb888(void *dst, unsigned int
dst_pitch, const void *sr
void drm_fb_xrgb_to_rgb888_toio(void __iomem *dst, unsigned int dst_pitch,
const void *vaddr, const struct
drm_framebuffer *fb,
const struct drm_rect *clip);
+void drm_fb_xrgb_to_xrgb2101010_toio(void __iomem *dst, unsigned int
dst_pitch,
+const void *vaddr, const struct
drm_framebuffer *fb,
+const struct drm_rect *clip);
void drm_fb_xrgb_to_gray8(void *dst, unsigned int dst_pitch, const void
*vaddr,
const struct drm_framebuffer *fb, const struct
drm_rect *clip);
--
2.33.0
[PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of
This code is required for both simplefb and simpledrm, so let's move it
into the OF core instead of having it as an ad-hoc initcall in the
drivers.
Acked-by: Thomas Zimmermann
Signed-off-by: Hector Martin
---
drivers/of/platform.c | 4
drivers/video/fbdev/simplefb.c | 21 +
2 files changed, 5 insertions(+), 20 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b3faf89744aa..793350028906 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void)
of_node_put(node);
}
+ node = of_get_compatible_child(of_chosen, "simple-framebuffer");
+ of_platform_device_create(node, NULL, NULL);
+ of_node_put(node);
+
/* Populate everything else. */
of_platform_default_populate(NULL, NULL, NULL);
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index b63074fd892e..57541887188b 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -541,26 +541,7 @@ static struct platform_driver simplefb_driver = {
.remove = simplefb_remove,
};
-static int __init simplefb_init(void)
-{
- int ret;
- struct device_node *np;
-
- ret = platform_driver_register(&simplefb_driver);
- if (ret)
- return ret;
-
- if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) {
- for_each_child_of_node(of_chosen, np) {
- if (of_device_is_compatible(np, "simple-framebuffer"))
- of_platform_device_create(np, NULL, NULL);
- }
- }
-
- return 0;
-}
-
-fs_initcall(simplefb_init);
+module_platform_driver(simplefb_driver);
MODULE_AUTHOR("Stephen Warren ");
MODULE_DESCRIPTION("Simple framebuffer driver");
--
2.33.0
[PATCH v3 0/3] drm/simpledrm: Apple M1 / DT platform support fixes
Hi DRM folks, This short series makes simpledrm work on Apple M1 (including Pro/Max) platforms the way simplefb already does, by adding XRGB2101010 support and making it bind to framebuffers in /chosen the same way simplefb does. This avoids breaking the bootloader-provided framebuffer console when simpledrm is selected to replace simplefb, as these FBs always seem to be 10-bit (at least when a real screen is attached). Changes since v2: - Made 10-bit conversion code fill the LSBs - Added ARGB2101010 to supported formats list - Simplified OF core code per review feedback Hector Martin (3): of: Move simple-framebuffer device handling from simplefb to of drm/format-helper: Add drm_fb_xrgb_to_xrgb2101010_toio() drm/simpledrm: Add [AX]RGB2101010 formats drivers/gpu/drm/drm_format_helper.c | 64 + drivers/gpu/drm/tiny/simpledrm.c| 4 +- drivers/of/platform.c | 4 ++ drivers/video/fbdev/simplefb.c | 21 +- include/drm/drm_format_helper.h | 3 ++ 5 files changed, 74 insertions(+), 22 deletions(-) -- 2.33.0
Re: [PATCH v2 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_toio()
Hi, thanks for the review!
On 07/12/2021 18.40, Thomas Zimmermann wrote:
Hi
Am 07.12.21 um 08:29 schrieb Hector Martin:
Add XRGB emulation support for devices that can only do XRGB2101010.
This is chiefly useful for simpledrm on Apple devices where the
bootloader-provided framebuffer is 10-bit.
Signed-off-by: Hector Martin
---
drivers/gpu/drm/drm_format_helper.c | 62 +
include/drm/drm_format_helper.h | 3 ++
2 files changed, 65 insertions(+)
diff --git a/drivers/gpu/drm/drm_format_helper.c
b/drivers/gpu/drm/drm_format_helper.c
index dbe3e830096e..edd611d3ab6a 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -409,6 +409,59 @@ void drm_fb_xrgb_to_rgb888_toio(void __iomem *dst,
unsigned int dst_pitch,
}
EXPORT_SYMBOL(drm_fb_xrgb_to_rgb888_toio);
+static void drm_fb_xrgb_to_xrgb2101010_line(u32 *dbuf, const u32 *sbuf,
+ unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ *dbuf++ = ((sbuf[x] & 0x00FF) << 2) |
+ ((sbuf[x] & 0xFF00) << 4) |
+ ((sbuf[x] & 0x00FF) << 6);
This isn't quite right. The lowest two destination bits in each
component will always be zero. You have to do the shifting as above and
for each component the two highest source bits have to be OR'ed into the
two lowest destination bits. For example the source bits in a component
are numbered 7 to 0
| 7 6 5 4 3 2 1 0 |
then the destination bits should be
| 7 6 5 4 3 2 1 0 7 6 |
I think both approaches have pros and cons. Leaving the two LSBs always
at 0 yields a fully linear transfer curve with no discontinuities, but
means the maximum brightness is slightly less than full. Setting them
fully maps the brightness range, but creates 4 double wide steps in the
transfer curve (also it's potentially slightly slower CPU-wise).
If you prefer the latter I'll do that for v2.
--
Hector Martin (mar...@marcan.st)
Public Key: https://mrcn.st/pub
[PATCH v2 3/3] drm/simpledrm: Add XRGB2101010 format
This is the format used by the bootloader framebuffer on Apple ARM64
platforms.
Reviewed-by: Thomas Zimmermann
Signed-off-by: Hector Martin
---
drivers/gpu/drm/tiny/simpledrm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 2f15b9aa..edadfd9ee882 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -571,7 +571,7 @@ static const uint32_t simpledrm_default_formats[] = {
//DRM_FORMAT_XRGB1555,
//DRM_FORMAT_ARGB1555,
DRM_FORMAT_RGB888,
- //DRM_FORMAT_XRGB2101010,
+ DRM_FORMAT_XRGB2101010,
//DRM_FORMAT_ARGB2101010,
};
--
2.33.0
[PATCH v2 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_toio()
Add XRGB emulation support for devices that can only do XRGB2101010.
This is chiefly useful for simpledrm on Apple devices where the
bootloader-provided framebuffer is 10-bit.
Signed-off-by: Hector Martin
---
drivers/gpu/drm/drm_format_helper.c | 62 +
include/drm/drm_format_helper.h | 3 ++
2 files changed, 65 insertions(+)
diff --git a/drivers/gpu/drm/drm_format_helper.c
b/drivers/gpu/drm/drm_format_helper.c
index dbe3e830096e..edd611d3ab6a 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -409,6 +409,59 @@ void drm_fb_xrgb_to_rgb888_toio(void __iomem *dst,
unsigned int dst_pitch,
}
EXPORT_SYMBOL(drm_fb_xrgb_to_rgb888_toio);
+static void drm_fb_xrgb_to_xrgb2101010_line(u32 *dbuf, const u32 *sbuf,
+ unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ *dbuf++ = ((sbuf[x] & 0x00FF) << 2) |
+ ((sbuf[x] & 0xFF00) << 4) |
+ ((sbuf[x] & 0x00FF) << 6);
+ }
+}
+
+/**
+ * drm_fb_xrgb_to_xrgb2101010_toio - Convert XRGB to XRGB2101010 clip
+ * buffer
+ * @dst: XRGB2101010 destination buffer (iomem)
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
+ * @vaddr: XRGB source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ *
+ * Drivers can use this function for XRGB2101010 devices that don't natively
+ * support XRGB.
+ */
+void drm_fb_xrgb_to_xrgb2101010_toio(void __iomem *dst,
+unsigned int dst_pitch, const void
*vaddr,
+const struct drm_framebuffer *fb,
+const struct drm_rect *clip)
+{
+ size_t linepixels = clip->x2 - clip->x1;
+ size_t dst_len = linepixels * sizeof(u32);
+ unsigned y, lines = clip->y2 - clip->y1;
+ void *dbuf;
+
+ if (!dst_pitch)
+ dst_pitch = dst_len;
+
+ dbuf = kmalloc(dst_len, GFP_KERNEL);
+ if (!dbuf)
+ return;
+
+ vaddr += clip_offset(clip, fb->pitches[0], sizeof(u32));
+ for (y = 0; y < lines; y++) {
+ drm_fb_xrgb_to_xrgb2101010_line(dbuf, vaddr, linepixels);
+ memcpy_toio(dst, dbuf, dst_len);
+ vaddr += fb->pitches[0];
+ dst += dst_pitch;
+ }
+
+ kfree(dbuf);
+}
+EXPORT_SYMBOL(drm_fb_xrgb_to_xrgb2101010_toio);
+
/**
* drm_fb_xrgb_to_gray8 - Convert XRGB to grayscale
* @dst: 8-bit grayscale destination buffer
@@ -500,6 +553,10 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int
dst_pitch, uint32_t dst_for
fb_format = DRM_FORMAT_XRGB;
if (dst_format == DRM_FORMAT_ARGB)
dst_format = DRM_FORMAT_XRGB;
+ if (fb_format == DRM_FORMAT_ARGB2101010)
+ fb_format = DRM_FORMAT_XRGB2101010;
+ if (dst_format == DRM_FORMAT_ARGB2101010)
+ dst_format = DRM_FORMAT_XRGB2101010;
if (dst_format == fb_format) {
drm_fb_memcpy_toio(dst, dst_pitch, vmap, fb, clip);
@@ -515,6 +572,11 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int
dst_pitch, uint32_t dst_for
drm_fb_xrgb_to_rgb888_toio(dst, dst_pitch, vmap,
fb, clip);
return 0;
}
+ } else if (dst_format == DRM_FORMAT_XRGB2101010) {
+ if (fb_format == DRM_FORMAT_XRGB) {
+ drm_fb_xrgb_to_xrgb2101010_toio(dst, dst_pitch,
vmap, fb, clip);
+ return 0;
+ }
}
return -EINVAL;
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 97e4c3223af3..b30ed5de0a33 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -33,6 +33,9 @@ void drm_fb_xrgb_to_rgb888(void *dst, unsigned int
dst_pitch, const void *sr
void drm_fb_xrgb_to_rgb888_toio(void __iomem *dst, unsigned int dst_pitch,
const void *vaddr, const struct
drm_framebuffer *fb,
const struct drm_rect *clip);
+void drm_fb_xrgb_to_xrgb2101010_toio(void __iomem *dst, unsigned int
dst_pitch,
+const void *vaddr, const struct
drm_framebuffer *fb,
+const struct drm_rect *clip);
void drm_fb_xrgb_to_gray8(void *dst, unsigned int dst_pitch, const void
*vaddr,
const struct drm_framebuffer *fb, const struct
drm_rect *clip);
--
2.33.0
[PATCH v2 1/3] of: Move simple-framebuffer device handling from simplefb to of
This code is required for both simplefb and simpledrm, so let's move it
into the OF core instead of having it as an ad-hoc initcall in the
drivers.
Signed-off-by: Hector Martin
---
drivers/of/platform.c | 5 +
drivers/video/fbdev/simplefb.c | 21 +
2 files changed, 6 insertions(+), 20 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b3faf89744aa..e097f40b03c0 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -540,6 +540,11 @@ static int __init of_platform_default_populate_init(void)
of_node_put(node);
}
+ for_each_child_of_node(of_chosen, node) {
+ if (of_device_is_compatible(node, "simple-framebuffer"))
+ of_platform_device_create(node, NULL, NULL);
+ }
+
/* Populate everything else. */
of_platform_default_populate(NULL, NULL, NULL);
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index b63074fd892e..57541887188b 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -541,26 +541,7 @@ static struct platform_driver simplefb_driver = {
.remove = simplefb_remove,
};
-static int __init simplefb_init(void)
-{
- int ret;
- struct device_node *np;
-
- ret = platform_driver_register(&simplefb_driver);
- if (ret)
- return ret;
-
- if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) {
- for_each_child_of_node(of_chosen, np) {
- if (of_device_is_compatible(np, "simple-framebuffer"))
- of_platform_device_create(np, NULL, NULL);
- }
- }
-
- return 0;
-}
-
-fs_initcall(simplefb_init);
+module_platform_driver(simplefb_driver);
MODULE_AUTHOR("Stephen Warren ");
MODULE_DESCRIPTION("Simple framebuffer driver");
--
2.33.0
[PATCH v2 0/3] drm/simpledrm: Apple M1 / DT platform support fixes
Hi DRM folks, This short series makes simpledrm work on Apple M1 (including Pro/Max) platforms the way simplefb already does, by adding XRGB2101010 support and making it bind to framebuffers in /chosen the same way simplefb does. This avoids breaking the bootloader-provided framebuffer console when simpledrm is selected to replace simplefb, as these FBs always seem to be 10-bit (at least when a real screen is attached). Changes since v1: - Moved the OF platform device setup code from simplefb into common code, instead of duplicating it in simpledrm - Rebased on drm-tip Hector Martin (3): of: Move simple-framebuffer device handling from simplefb to of drm/format-helper: Add drm_fb_xrgb_to_xrgb2101010_toio() drm/simpledrm: Add XRGB2101010 format drivers/gpu/drm/drm_format_helper.c | 62 + drivers/gpu/drm/tiny/simpledrm.c| 2 +- drivers/of/platform.c | 5 +++ drivers/video/fbdev/simplefb.c | 21 +- include/drm/drm_format_helper.h | 3 ++ 5 files changed, 72 insertions(+), 21 deletions(-) -- 2.33.0
Re: [PATCH 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_dstclip()
On 22/11/2021 18.52, Pekka Paalanen wrote: On Wed, 17 Nov 2021 23:58:28 +0900 Hector Martin wrote: Add XRGB emulation support for devices that can only do XRGB2101010. This is chiefly useful for simpledrm on Apple devices where the bootloader-provided framebuffer is 10-bit, which already works fine with simplefb. This is required to make simpledrm support this too. Signed-off-by: Hector Martin --- drivers/gpu/drm/drm_format_helper.c | 64 + include/drm/drm_format_helper.h | 4 ++ 2 files changed, 68 insertions(+) Hi Hector, I'm curious, since the bootloader seems to always set up a 10-bit mode, is there a reason for it that you can guess? Is the monitor in WCG or even HDR mode? My guess is that Apple prefer to use 10-bit framebuffers for seamless handover with their graphics stack, which presumably uses 10-bit framebuffers these days. It seems to be unconditional; I've never seen anything but 10 bits across all Apple devices, both with the internal panels on laptops and with bog standard external displays on the Mac Mini via HDMI. HDR is not necessary, even very dumb capture cards and old screens get a 10-bit framebufer in memory. The only time I see an 8-bit framebuffer is with *no* monitor connected on the Mini, in which case you get an 8-bit 640x1136 dummy framebuffer (that's the iPhone 5 screen resolution... :-) ) -- Hector Martin (mar...@marcan.st) Public Key: https://mrcn.st/pub
Re: [PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen
On 18/11/2021 18.19, Thomas Zimmermann wrote:
Hi
Am 17.11.21 um 15:58 schrieb Hector Martin:
@@ -897,5 +898,21 @@ static struct platform_driver simpledrm_platform_driver = {
module_platform_driver(simpledrm_platform_driver);
+static int __init simpledrm_init(void)
+{
+ struct device_node *np;
+
+ if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) {
+ for_each_child_of_node(of_chosen, np) {
+ if (of_device_is_compatible(np, "simple-framebuffer"))
+ of_platform_device_create(np, NULL, NULL);
+ }
+ }
+
+ return 0;
+}
+
+fs_initcall(simpledrm_init);
+
Simpledrm is just a driver, but this is platform setup code. Why is this
code located here and not under arch/ or drivers/firmware/?
I know that other drivers do similar things, it doesn't seem to belong here.
This definitely doesn't belong in either of those, since it is not arch-
or firmware-specific. It is implementing support for the standard
simple-framebuffer OF binding, which specifies that it must be located
within the /chosen node (and thus the default OF setup code won't do the
matching for you); this applies to all OF platforms [1]
Adding Rob; do you think this should move from simplefb/simpledrm to
common OF code? (where?)
[1] Documentation/devicetree/bindings/display/simple-framebuffer.yaml
--
Hector Martin (mar...@marcan.st)
Public Key: https://mrcn.st/pub
Re: [PATCH] drm/format-helper: Fix dst computation in drm_fb_xrgb8888_to_rgb888_dstclip()
On 17/11/2021 23.56, Thomas Zimmermann wrote: Hi Am 17.11.21 um 15:22 schrieb Hector Martin: The dst pointer was being advanced by the clip width, not the full line stride, resulting in corruption. The clip offset was also calculated incorrectly. Cc: sta...@vger.kernel.org Signed-off-by: Hector Martin Thanks for your patch, but you're probably on the wrong branch. We rewrote this code recently and fixed the issue in drm-misc-next. [1][2] Oops. I was on linux-next as of Nov 1. Looks like I missed it by a week! Sounds like I'm going to have to rebase/rewrite the other series I just sent too... -- Hector Martin (mar...@marcan.st) Public Key: https://mrcn.st/pub
[PATCH 3/3] drm/simpledrm: Enable XRGB2101010 format
This is the format used by the bootloader framebuffer on Apple ARM64
platforms, and is already supported by simplefb. This avoids regressing
on these platforms when simpledrm is enabled and replaces simplefb.
Signed-off-by: Hector Martin
---
drivers/gpu/drm/tiny/simpledrm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 2c84f2ea1fa2..b4b69f3a7e79 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -571,7 +571,7 @@ static const uint32_t simpledrm_default_formats[] = {
//DRM_FORMAT_XRGB1555,
//DRM_FORMAT_ARGB1555,
DRM_FORMAT_RGB888,
- //DRM_FORMAT_XRGB2101010,
+ DRM_FORMAT_XRGB2101010,
//DRM_FORMAT_ARGB2101010,
};
--
2.33.0
[PATCH 2/3] drm/format-helper: Add drm_fb_xrgb8888_to_xrgb2101010_dstclip()
Add XRGB emulation support for devices that can only do XRGB2101010.
This is chiefly useful for simpledrm on Apple devices where the
bootloader-provided framebuffer is 10-bit, which already works fine with
simplefb. This is required to make simpledrm support this too.
Signed-off-by: Hector Martin
---
drivers/gpu/drm/drm_format_helper.c | 64 +
include/drm/drm_format_helper.h | 4 ++
2 files changed, 68 insertions(+)
diff --git a/drivers/gpu/drm/drm_format_helper.c
b/drivers/gpu/drm/drm_format_helper.c
index 69fde60e36b3..5998e57d6ff2 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -378,6 +378,60 @@ void drm_fb_xrgb_to_rgb888_dstclip(void __iomem *dst,
unsigned int dst_pitch
}
EXPORT_SYMBOL(drm_fb_xrgb_to_rgb888_dstclip);
+static void drm_fb_xrgb_to_xrgb2101010_line(u32 *dbuf, u32 *sbuf,
+ unsigned int pixels)
+{
+ unsigned int x;
+
+ for (x = 0; x < pixels; x++) {
+ *dbuf++ = ((sbuf[x] & 0x00FF) << 2) |
+ ((sbuf[x] & 0xFF00) << 4) |
+ ((sbuf[x] & 0x00FF) << 6);
+ }
+}
+
+/**
+ * drm_fb_xrgb_to_xrgb2101010_dstclip - Convert XRGB to XRGB2101010
clip
+ * buffer
+ * @dst: XRGB2101010 destination buffer (iomem)
+ * @dst_pitch: destination buffer pitch
+ * @vaddr: XRGB source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ *
+ * Drivers can use this function for XRGB2101010 devices that don't natively
+ * support XRGB.
+ *
+ * This function applies clipping on dst, i.e. the destination is a
+ * full (iomem) framebuffer but only the clip rect content is copied over.
+ */
+void drm_fb_xrgb_to_xrgb2101010_dstclip(void __iomem *dst,
+ unsigned int dst_pitch, void *vaddr,
+ struct drm_framebuffer *fb,
+ struct drm_rect *clip)
+{
+ size_t linepixels = clip->x2 - clip->x1;
+ size_t dst_len = linepixels * 4;
+ unsigned int y, lines = clip->y2 - clip->y1;
+ void *dbuf;
+
+ dbuf = kmalloc(dst_len, GFP_KERNEL);
+ if (!dbuf)
+ return;
+
+ vaddr += clip_offset(clip, fb->pitches[0], sizeof(u32));
+ dst += clip_offset(clip, dst_pitch, sizeof(u32));
+ for (y = 0; y < lines; y++) {
+ drm_fb_xrgb_to_xrgb2101010_line(dbuf, vaddr, linepixels);
+ memcpy_toio(dst, dbuf, dst_len);
+ vaddr += fb->pitches[0];
+ dst += dst_pitch;
+ }
+
+ kfree(dbuf);
+}
+EXPORT_SYMBOL(drm_fb_xrgb_to_xrgb2101010_dstclip);
+
/**
* drm_fb_xrgb_to_gray8 - Convert XRGB to grayscale
* @dst: 8-bit grayscale destination buffer
@@ -464,6 +518,10 @@ int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned
int dst_pitch,
fb_format = DRM_FORMAT_XRGB;
if (dst_format == DRM_FORMAT_ARGB)
dst_format = DRM_FORMAT_XRGB;
+ if (fb_format == DRM_FORMAT_ARGB2101010)
+ fb_format = DRM_FORMAT_XRGB2101010;
+ if (dst_format == DRM_FORMAT_ARGB2101010)
+ dst_format = DRM_FORMAT_XRGB2101010;
if (dst_format == fb_format) {
drm_fb_memcpy_dstclip(dst, dst_pitch, vmap, fb, clip);
@@ -482,6 +540,12 @@ int drm_fb_blit_rect_dstclip(void __iomem *dst, unsigned
int dst_pitch,
vmap, fb, clip);
return 0;
}
+ } else if (dst_format == DRM_FORMAT_XRGB2101010) {
+ if (fb_format == DRM_FORMAT_XRGB) {
+ drm_fb_xrgb_to_xrgb2101010_dstclip(dst, dst_pitch,
+ vmap, fb, clip);
+ return 0;
+ }
}
return -EINVAL;
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index e86925cf07b9..a0faa710878b 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -29,6 +29,10 @@ void drm_fb_xrgb_to_rgb888(void *dst, void *src, struct
drm_framebuffer *fb,
void drm_fb_xrgb_to_rgb888_dstclip(void __iomem *dst, unsigned int
dst_pitch,
void *vaddr, struct drm_framebuffer *fb,
struct drm_rect *clip);
+void drm_fb_xrgb_to_xrgb2101010_dstclip(void __iomem *dst,
+ unsigned int dst_pitch, void *vaddr,
+ struct drm_framebuffer *fb,
+ struct drm_rect *clip);
void drm_fb_xrgb_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
struct drm_rect *clip);
--
2.33.0
[PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen
This matches the simplefb behavior; these nodes are not matched by the
standard OF machinery. This fixes a regression when simpledrm replaces
simeplefb.
Signed-off-by: Hector Martin
---
drivers/gpu/drm/tiny/simpledrm.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 481b48bde047..2c84f2ea1fa2 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -2,6 +2,7 @@
#include
#include
+#include
#include
#include
#include
@@ -897,5 +898,21 @@ static struct platform_driver simpledrm_platform_driver = {
module_platform_driver(simpledrm_platform_driver);
+static int __init simpledrm_init(void)
+{
+ struct device_node *np;
+
+ if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) {
+ for_each_child_of_node(of_chosen, np) {
+ if (of_device_is_compatible(np, "simple-framebuffer"))
+ of_platform_device_create(np, NULL, NULL);
+ }
+ }
+
+ return 0;
+}
+
+fs_initcall(simpledrm_init);
+
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL v2");
--
2.33.0
[PATCH 0/3] drm/simpledrm: Apple M1 / DT platform support fixes
Hi DRM folks, This short series makes simpledrm work on Apple M1 (including Pro/Max) platforms the way simplefb already does, by adding XRGB2101010 support and making it bind to framebuffers in /chosen the same way simplefb does. This avoids breaking the bootloader-provided framebuffer console when simpledrm is selected to replace simplefb, as these FBs always seem to be 10-bit (at least when a real screen is attached). Hector Martin (3): drm/simpledrm: Bind to OF framebuffers in /chosen drm/format-helper: Add drm_fb_xrgb_to_xrgb2101010_dstclip() drm/simpledrm: Enable XRGB2101010 format drivers/gpu/drm/drm_format_helper.c | 64 + drivers/gpu/drm/tiny/simpledrm.c| 19 - include/drm/drm_format_helper.h | 4 ++ 3 files changed, 86 insertions(+), 1 deletion(-) -- 2.33.0
[PATCH] drm/format-helper: Fix dst computation in drm_fb_xrgb8888_to_rgb888_dstclip()
The dst pointer was being advanced by the clip width, not the full line
stride, resulting in corruption. The clip offset was also calculated
incorrectly.
Cc: sta...@vger.kernel.org
Signed-off-by: Hector Martin
---
drivers/gpu/drm/drm_format_helper.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_format_helper.c
b/drivers/gpu/drm/drm_format_helper.c
index e676921422b8..12bc6b45e95b 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -366,12 +366,12 @@ void drm_fb_xrgb_to_rgb888_dstclip(void __iomem *dst,
unsigned int dst_pitch
return;
vaddr += clip_offset(clip, fb->pitches[0], sizeof(u32));
- dst += clip_offset(clip, dst_pitch, sizeof(u16));
+ dst += clip_offset(clip, dst_pitch, 3);
for (y = 0; y < lines; y++) {
drm_fb_xrgb_to_rgb888_line(dbuf, vaddr, linepixels);
memcpy_toio(dst, dbuf, dst_len);
vaddr += fb->pitches[0];
- dst += dst_len;
+ dst += dst_pitch;
}
kfree(dbuf);
--
2.33.0
Re: [PATCH 2/2] maintainers: Update freedesktop.org IRC channels
On 30/05/2021 20.01, Lukas Wunner wrote: On Sat, May 29, 2021 at 10:16:38AM -0400, Alyssa Rosenzweig wrote: --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1651,7 +1651,7 @@ L:linux-arm-ker...@lists.infradead.org (moderated for non-subscribers) S:Maintained W:https://asahilinux.org B:https://github.com/AsahiLinux/linux/issues -C: irc://chat.freenode.net/asahi-dev +C: irc://irc.oftc.net/asahi-dev T:git https://github.com/AsahiLinux/linux.git F:Documentation/devicetree/bindings/arm/apple.yaml F:Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml This isn't a freedesktop.org project, so either needs to be dropped from the patch or the patch needs an ack from Hector Martin (+cc). Heh, I totally forgot we had the IRC info in MAINTAINERS too when I did the move. Thanks :) Acked-By: Hector Martin -- Hector Martin (mar...@marcan.st) Public Key: https://mrcn.st/pub
