On Wed, May 10, 2017 at 09:34:40AM -0700, Ben Widawsky wrote: > On 17-05-03 18:30:07, Liviu Dudau wrote: > > On Wed, May 03, 2017 at 06:45:05PM +0200, Daniel Vetter wrote: > > > On Wed, May 03, 2017 at 03:52:23PM +0100, Liviu Dudau wrote: > > > > On Wed, May 03, 2017 at 03:14:56PM +0100, Daniel Stone wrote: > > > > > On 3 May 2017 at 15:07, Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote: > > > > > > On Wed, May 03, 2017 at 02:45:26PM +0100, Daniel Stone wrote: > > > > > >> 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. > > > > > > > > > > > > I get the overall goal. We need/want something similar for Mali DP and AFBC buffers. > > > > > > What I don't understand is the current aproach (but I've found from Brian that somehow > > > > > > I've missed the long discussion(s) around the subject). I was hoping to learn > > > > > > from the commit message why he thinks the introduction of this code is the right > > > > > > way of doing it. And the IRC logs seem to imply that he is mostly doing something > > > > > > that others have agreed upon and he doesn't really care about the approach as long > > > > > > as it ticks the "supported by intel driver" box. > > > > > > > > > > Or, with another interpretation, he thinks the various approaches of > > > > > doing it are all equally good. He took guidance from a couple of > > > > > userspace developers (Weston, ChromeOS), a Freedreno developer and > > > > > also I believe an AFBC developer, to end up where he is now, which he > > > > > still thinks is fine. In doing so, he's been through several > > > > > iterations, always modifying the driver to suit. I think that's a > > > > > pretty good way to do development of new uABI, if you ask me. (And > > > > > again, I have trouble reading your last sentence as anything other > > > > > than hostile.) > > > > > > > > I am getting the message. You think I am too strong here, so I'm going to back off. > > > > Also look forward to see the next version where he takes into account the technical > > > > comments that I have sent. > > > > > > Just to make it really clear: Google has an implementation for AFBC using > > > exactly this scheme for cros. The only reasons it's not floating around > > > here in upstream is that one of the critical pieces is missing (*hint*, > > > *hint* you really want an open mail driver ...). > > > > <tongue_in_cheek> > > Don't know about open _mail_ drivers but there are plenty of open mail apps that one can use > > </tongue_in_cheek> > > > > Joke aside, the Mali GPU *kernel* driver *is* open source. Just not in the mainline and > > not enough for what you want. But I'm not the right person to fix all that. > > > > And GPU is not the only IP capable of producing AFBC data, so there might be another driver > > in the making that will be open source. > > > > Best regards, > > Liviu > > > > > But besides that, it works > > > perfectly fine for arm render compression format too. > > > -Daniel > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > -- > > So are we good with patch 1, sorry if I missed any real valid changes I need to > make, but can we please capture that here. Sure, - documentation needs to use the actual macro name: DRM_FORMAT_MOD_INVALID - drm_universal_plane_init comment about first driver with > 64 formats could do with some verbose explanation on why ("because we encode each format as a bit in the format_modifiers field and we have only reserved 64 bits for that") - most (all?) drivers are passing NULL as the format_modifier pointer to drm_universal_plane_init() which makes me wonder if it is not better to have a different function for drivers that want to pass a non-NULL format_modifier. - plane->modifier_count is never used - memcpy()-ing the format_modifiers in drm_universal_plane_init doesn't check for NULL even if that is what most drivers pass on when they call the function. - some extraneous empty lines could be trimmed - format_mod_supported function is not used in 1/3 patch, you can introduce it in the patch that actually uses it - drm_plane's formats field doesn't look used either. You can look in the first reply to your 1/3 patch if you want the details. Best regards, Liviu > > -- > Ben Widawsky, Intel Open Source Technology Center > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯ _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
