On 17-05-03 14:45:26, Daniel Stone wrote:
Hi Liviu,
On 3 May 2017 at 11:34, Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote:
On Tue, May 02, 2017 at 10:14:26PM -0700, Ben Widawsky wrote:
v2: A minor addition from Daniel
You are *really* pushing your luck by not Cc-ing *any* of the maintainers of
the drivers you touch. You do want reviews, don't you?
Ouch. I'm very sure Ben does want reviews, but he mainly works in Mesa
where you can rely on the list rather than having to CC everyone
individually. It was a mistake, so please be more gentle with him;
your whole mail comes across as quite hostile to me.
It was not a mistake. The whole point of a mailing list is so that people can
participate without needing to Cc every individual. dri-devel has been the
location, and while many hardware vendors have set up their own list, dri-devel
is still the mailing list for patches like this. There are a ridiculous number
of DRM driver maintainers. There isn't even an easy way to script extracting all
the relevant people from MAINTAINERS (happy to be corrected on that) I don't
believe anybody Cc's them all, ever - but normally there isn't such a globally
invasive patch.
I'm sorry Liviu you obviously felt slighted. I did Cc the Intel mailing list
because my implementation for this was on Intel.
Finally, the questions I should've started with: why the change? What are you trying to
achieve? It is not very clear from the commit message at all.
It does deserve a much better commit message than what it has, but as
he is on holiday for the rest of the week, I can answer. Currently, we
advertise which formats each plane is capable of displaying. In order
for userspace to be able to allocate tiled/compressed buffers for
scanout, we want userspace to be able to discover which modifiers each
plane supports as well.
@@ -105,6 +108,28 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
return -ENOMEM;
}
+ /* First driver to need more than 64 formats needs to fix this */
+ if (WARN_ON(format_count > 64))
+ return -EINVAL;
What is the link between format_count and format modifiers? Why are you introducing
this artificial restriction on the format_count? Also, there doesn't seem to be any
check if format_modifier_count == format_count. What happens when they don't match?
Inside the blob, each modifier carries a bitmask of formats (specified
by index) that the modifier applies to, i.e. with:
.formats = { ARGB8888, XRGB8888, RGB565 },
a modifier which applied only to the 32-bit formats would specify a
bitmask of 0x3. The uABI supports this just fine, but the internal
plumbing does not yet, because no-one supports more than 64 formats.
+
+ if (format_modifiers) {
+ const uint64_t *temp_modifiers = format_modifiers;
+ while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
+ format_modifier_count++;
+ }
+
+ plane->modifier_count = format_modifier_count;
Why do you need to remember this? It is not used anywhere else!
It is used to populate the blob in a later patch!
Cheers,
Daniel
I'm back now. Thanks Daniel for summing it up for me.
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
