Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Liviu Dudau]

On Wed, May 10, 2017 at 12:33:15PM -0700, Ben Widawsky wrote:
> On 17-05-10 18:24:52, Liviu Dudau wrote:
> >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  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 ...).
> >>>
> >>> 
> >>> Don't know about open _mail_ drivers but there are plenty of open mail 
> >>> apps that one can use
> >>> 
> >>>
> >>> 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,
> >
> 
> Thanks. Reordered your comments a bit...
> 
> >- documentation needs to use the actual macro name: DRM_FORMAT_MOD_INVALID
> 
> Okay. It originally wasn't this way because it forces the sentence to be two
> lines, but I've changed it.
> 
> >- some extraneous empty lines could be trimmed
> 
> I think I got them all..
> 
> >- 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.
> 
> I can wrap this, that might be the best way and then I wouldn't need to touch
> the other drivers. Here is the relevant part of the diff:
> 
> 

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Ben Widawsky]

On 17-05-10 18:24:52, Liviu Dudau wrote:

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  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 ...).
>
> 
> Don't know about open _mail_ drivers but there are plenty of open mail apps 
that one can use
> 
>
> 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,



Thanks. Reordered your comments a bit...


- documentation needs to use the actual macro name: DRM_FORMAT_MOD_INVALID


Okay. It originally wasn't this way because it forces the sentence to be two
lines, but I've changed it.


- some extraneous empty lines could be trimmed


I think I got them all..


- 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.


I can wrap this, that might be the best way and then I wouldn't need to touch
the other drivers. Here is the relevant part of the diff:

I'm fine with that if other people are. It only is the way it is because
Kristian originally did this for GET_PLANE2.

+__printf(9, 10)
+int drm_universal_plane_init_with_modifiers(struct drm_device *dev,
+   struct drm_plane *plane,
+   uint32_t possible_crtcs,
+   const struct drm_plane_funcs *funcs,
+   const uint32_t *formats,
+   unsigned int format_count,
+   const 

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Liviu Dudau]

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  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 ...).
> > 
> > 
> > Don't know about open _mail_ drivers but there are plenty of open mail apps 
> > that one can use
> > 
> > 
> > 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-de...@lists.freedesktop.org
> 

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Ben Widawsky]

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  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 ...).



Don't know about open _mail_ drivers but there are plenty of open mail apps 
that one can use


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.

--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Ben Widawsky]

On 17-05-03 14:45:26, Daniel Stone wrote:

Hi Liviu,

On 3 May 2017 at 11:34, Liviu Dudau  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 = { ARGB, XRGB, 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[kbuild test robot]

Hi Ben,

[auto build test WARNING on drm/drm-next]
[also build test WARNING on next-20170503]
[cannot apply to v4.11]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Ben-Widawsky/drm-Plumb-modifiers-through-plane-init/20170504-004955
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c: In function 
'tinydrm_display_pipe_init':
>> drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c:227:8: warning: passing argument 
>> 6 of 'drm_simple_display_pipe_init' from incompatible pointer type
 ret = drm_simple_display_pipe_init(drm, >pipe, funcs, formats,
   ^
   In file included from include/drm/tinydrm/tinydrm.h:15:0,
from drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c:13:
   include/drm/drm_simple_kms_helper.h:121:5: note: expected 'const uint64_t *' 
but argument is of type 'struct drm_connector *'
int drm_simple_display_pipe_init(struct drm_device *dev,
^
   drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c:227:8: error: too few arguments 
to function 'drm_simple_display_pipe_init'
 ret = drm_simple_display_pipe_init(drm, >pipe, funcs, formats,
   ^
   In file included from include/drm/tinydrm/tinydrm.h:15:0,
from drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c:13:
   include/drm/drm_simple_kms_helper.h:121:5: note: declared here
int drm_simple_display_pipe_init(struct drm_device *dev,
^

vim +/drm_simple_display_pipe_init +227 
drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c

fa201ac2 Noralf Trønnes 2017-01-22  211 *mode_copy = *mode;
fa201ac2 Noralf Trønnes 2017-01-22  212 ret = 
tinydrm_rotate_mode(mode_copy, rotation);
fa201ac2 Noralf Trønnes 2017-01-22  213 if (ret) {
fa201ac2 Noralf Trønnes 2017-01-22  214 DRM_ERROR("Illegal 
rotation value %u\n", rotation);
fa201ac2 Noralf Trønnes 2017-01-22  215 return -EINVAL;
fa201ac2 Noralf Trønnes 2017-01-22  216 }
fa201ac2 Noralf Trønnes 2017-01-22  217  
fa201ac2 Noralf Trønnes 2017-01-22  218 drm->mode_config.min_width = 
mode_copy->hdisplay;
fa201ac2 Noralf Trønnes 2017-01-22  219 drm->mode_config.max_width = 
mode_copy->hdisplay;
fa201ac2 Noralf Trønnes 2017-01-22  220 drm->mode_config.min_height = 
mode_copy->vdisplay;
fa201ac2 Noralf Trønnes 2017-01-22  221 drm->mode_config.max_height = 
mode_copy->vdisplay;
fa201ac2 Noralf Trønnes 2017-01-22  222  
fa201ac2 Noralf Trønnes 2017-01-22  223 connector = 
tinydrm_connector_create(drm, mode_copy, connector_type);
fa201ac2 Noralf Trønnes 2017-01-22  224 if (IS_ERR(connector))
fa201ac2 Noralf Trønnes 2017-01-22  225 return 
PTR_ERR(connector);
fa201ac2 Noralf Trønnes 2017-01-22  226  
fa201ac2 Noralf Trønnes 2017-01-22 @227 ret = 
drm_simple_display_pipe_init(drm, >pipe, funcs, formats,
fa201ac2 Noralf Trønnes 2017-01-22  228 
   format_count, connector);
fa201ac2 Noralf Trønnes 2017-01-22  229 if (ret)
fa201ac2 Noralf Trønnes 2017-01-22  230 return ret;
fa201ac2 Noralf Trønnes 2017-01-22  231  
fa201ac2 Noralf Trønnes 2017-01-22  232 return 0;
fa201ac2 Noralf Trønnes 2017-01-22  233  }
fa201ac2 Noralf Trønnes 2017-01-22  234  
EXPORT_SYMBOL(tinydrm_display_pipe_init);

:: The code at line 227 was first introduced by commit
:: fa201ac2c61f51d9abdaffdf994d5780dcb51703 drm: Add DRM support for tiny 
LCD displays

:: TO: Noralf Trønnes 
:: CC: Noralf Trønnes 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[kbuild test robot]

Hi Ben,

[auto build test WARNING on drm/drm-next]
[also build test WARNING on next-20170503]
[cannot apply to v4.11]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Ben-Widawsky/drm-Plumb-modifiers-through-plane-init/20170504-004955
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick 
(https://www.imagemagick.org)
   include/linux/init.h:1: warning: no structured comments found
   kernel/sched/core.c:2085: warning: No description found for parameter 'rf'
   kernel/sched/core.c:2085: warning: Excess function parameter 'cookie' 
description in 'try_to_wake_up_local'
   include/linux/kthread.h:26: warning: Excess function parameter '...' 
description in 'kthread_create'
   kernel/sys.c:1: warning: no structured comments found
   include/linux/device.h:969: warning: No description found for parameter 
'dma_ops'
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   include/linux/iio/iio.h:597: warning: No description found for parameter 
'trig_readonly'
   include/linux/iio/trigger.h:151: warning: No description found for parameter 
'indio_dev'
   include/linux/iio/trigger.h:151: warning: No description found for parameter 
'trig'
   include/linux/device.h:970: warning: No description found for parameter 
'dma_ops'
   drivers/regulator/core.c:1467: warning: Excess function parameter 'ret' 
description in 'regulator_dev_lookup'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'set_busid'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'irq_handler'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'irq_preinstall'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'irq_postinstall'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'irq_uninstall'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'debugfs_init'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_open_object'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_close_object'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'prime_handle_to_fd'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'prime_fd_to_handle'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_prime_export'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_prime_import'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_prime_pin'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_prime_unpin'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_prime_res_obj'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_prime_get_sg_table'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_prime_import_sg_table'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_prime_vmap'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_prime_vunmap'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_prime_mmap'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'gem_vm_ops'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'major'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'minor'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'patchlevel'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'name'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'desc'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'date'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'driver_features'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'ioctls'
   include/drm/drm_drv.h:524: warning: No description found for parameter 
'num_ioctls'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'fops'
>> include/drm/drm_plane.h:545: warning: No description found for parameter 
>> 'formats'
>> include/drm/drm_plane.h:545: warning: No description found for parameter 
>> 'modifiers'
>> include/drm/drm_plane.h:545: warning: No description found for parameter 
>> 'modifier_count'
   include/drm/drm_color_mgmt.h:1: warning: no structured comments found
   drivers/gpu/drm/drm_plane_helper.c:403: warning: No description found for 
parameter 'ctx'
   drivers/gpu/drm/drm_plane_helper.c:404: warning: No description found for 
parameter 'ctx'
   

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Liviu Dudau]

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  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 ...).


Don't know about open _mail_ drivers but there are plenty of open mail apps 
that one can use


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

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Daniel Vetter]

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  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 ...). But besides that, it works
perfectly fine for arm render compression format too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Liviu Dudau]

On Wed, May 03, 2017 at 03:14:56PM +0100, Daniel Stone wrote:
> On 3 May 2017 at 15:07, Liviu Dudau  wrote:
> > On Wed, May 03, 2017 at 02:45:26PM +0100, Daniel Stone wrote:
> >> On 3 May 2017 at 11:34, Liviu Dudau  wrote:
> >> > 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.
> >
> > Sorry, I'm not trying to be hostile. Try to read the bolded words with a 
> > long
> > (southern USA) intonation as if to draw attention to them. You will see that
> > it is just pointing to two facts: he does not warn anyone about the changes 
> > and
> > he is not making the patchset that obviously critical by having a commit 
> > message
> > that implies everyone should pay attention to it. So he is really hoping to 
> > be
> > lucky to get reviews (or doesn't actively seek them).
> 
> You could achieve the same thing with absolutely no room for
> misinterpretation: 'Hi Ben. Not sure if you weren't aware or forgot,
> but when posting patches here, please use get_maintainers.pl to build
> a CC list. I only really saw this by luck, and other maintainers have
> probably missed this too.'

Agree, probably a better tone. Apologies to Ben for the initial form. I'm sure
he will do the correct thing next time.

> 
> >> 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.

Best regards,
Liviu

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Daniel Stone]

On 3 May 2017 at 15:07, Liviu Dudau  wrote:
> On Wed, May 03, 2017 at 02:45:26PM +0100, Daniel Stone wrote:
>> On 3 May 2017 at 11:34, Liviu Dudau  wrote:
>> > 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.
>
> Sorry, I'm not trying to be hostile. Try to read the bolded words with a long
> (southern USA) intonation as if to draw attention to them. You will see that
> it is just pointing to two facts: he does not warn anyone about the changes 
> and
> he is not making the patchset that obviously critical by having a commit 
> message
> that implies everyone should pay attention to it. So he is really hoping to be
> lucky to get reviews (or doesn't actively seek them).

You could achieve the same thing with absolutely no room for
misinterpretation: 'Hi Ben. Not sure if you weren't aware or forgot,
but when posting patches here, please use get_maintainers.pl to build
a CC list. I only really saw this by luck, and other maintainers have
probably missed this too.'

>> 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.)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Liviu Dudau]

On Wed, May 03, 2017 at 02:45:26PM +0100, Daniel Stone wrote:
> Hi Liviu,
> 
> On 3 May 2017 at 11:34, Liviu Dudau  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.

Sorry, I'm not trying to be hostile. Try to read the bolded words with a long
(southern USA) intonation as if to draw attention to them. You will see that
it is just pointing to two facts: he does not warn anyone about the changes and
he is not making the patchset that obviously critical by having a commit message
that implies everyone should pay attention to it. So he is really hoping to be
lucky to get reviews (or doesn't actively seek them).

> 
> > 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.

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.

> 
> >> @@ -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 = { ARGB, XRGB, 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.

Yeah, still doesn't answer the question. Maybe if the comment said something 
like:
"When building format modifiers bitmap we only have space to map up to 64 
formats.
Check that the driver is not trying to exceed that." then it would've been more
obvious why the check is needed and also why one doesn't care about 
format_modifier_count.

To explain my mindset: the way the patch series was formatted and the way I've 
read it
initially I did not realised that the idea is to build a bitmask. Nothing in 
this patch
alludes to that so I assumed there was a 1:1 relationship between format 
modifiers and
format.

> 
> >> +
> >> + 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!

I cannot find that patch. Nothing in the 3 patches uses that. Have a look!

Best regards,
Liviu

> 
> Cheers,
> Daniel

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Daniel Stone]

Hi Liviu,

On 3 May 2017 at 11:34, Liviu Dudau  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.

> 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 = { ARGB, XRGB, 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Liviu Dudau]

On Tue, May 02, 2017 at 10:14:26PM -0700, Ben Widawsky wrote:
> v2: A minor addition from Daniel
> 
> Cc: Daniel Stone 

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?

> Signed-off-by: Ben Widawsky 
> ---
>  drivers/gpu/drm/arc/arcpgu_crtc.c   |  1 +
>  drivers/gpu/drm/arm/hdlcd_crtc.c|  1 +
>  drivers/gpu/drm/arm/malidp_planes.c |  2 +-
>  drivers/gpu/drm/armada/armada_crtc.c|  1 +
>  drivers/gpu/drm/armada/armada_overlay.c |  1 +
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  4 +++-
>  drivers/gpu/drm/drm_modeset_helper.c|  1 +
>  drivers/gpu/drm/drm_plane.c | 32 
> -
>  drivers/gpu/drm/drm_simple_kms_helper.c |  3 +++
>  drivers/gpu/drm/exynos/exynos_drm_plane.c   |  2 +-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c |  2 +-
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  |  1 +
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  2 +-
>  drivers/gpu/drm/i915/intel_display.c|  7 +-
>  drivers/gpu/drm/i915/intel_sprite.c |  4 ++--
>  drivers/gpu/drm/imx/ipuv3-plane.c   |  4 ++--
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c|  2 +-
>  drivers/gpu/drm/meson/meson_plane.c |  1 +
>  drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c   |  2 +-
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c   |  4 ++--
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c   |  2 +-
>  drivers/gpu/drm/nouveau/nv50_display.c  |  5 ++--
>  drivers/gpu/drm/omapdrm/omap_plane.c|  3 ++-
>  drivers/gpu/drm/qxl/qxl_display.c   |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c |  4 ++--
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  4 ++--
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  4 ++--
>  drivers/gpu/drm/sti/sti_cursor.c|  2 +-
>  drivers/gpu/drm/sti/sti_gdp.c   |  2 +-
>  drivers/gpu/drm/sti/sti_hqvdp.c |  2 +-
>  drivers/gpu/drm/sun4i/sun4i_layer.c |  2 +-
>  drivers/gpu/drm/tegra/dc.c  | 12 +-
>  drivers/gpu/drm/vc4/vc4_plane.c |  2 +-
>  drivers/gpu/drm/virtio/virtgpu_plane.c  |  2 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c |  4 ++--
>  drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c|  4 ++--
>  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c|  4 ++--
>  drivers/gpu/drm/zte/zx_plane.c  |  2 +-
>  include/drm/drm_plane.h | 21 +++-
>  include/drm/drm_simple_kms_helper.h |  1 +
>  include/uapi/drm/drm_fourcc.h   | 11 +
>  41 files changed, 126 insertions(+), 46 deletions(-)

I wish there was a way to sort the non-driver-specific changes out of this 
patch and
put the at the beginning or at the end of the patchset. That way one does not 
have to
hunt through the file for the relevant changes in the API.

How about introducing a new function called drm_universal_plane_mod_init() that 
has
the new parameter, a second patch that moves the relevant/all drivers to the 
new function
and (if all drivers were converted) a third patch to rename 
drm_universal_plane_mod_init()
back to drm_universal_plane_init()? I know it is more churn, but I'm struggling 
to figure
out why all the drivers have to pass a NULL here. Either they care about 
modifiers or
they don't, in which case a separate function would make things more obvious.

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.

> 
> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c 
> b/drivers/gpu/drm/arc/arcpgu_crtc.c
> index ad9a95916f1f..cd8a24c7c67d 100644
> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> @@ -218,6 +218,7 @@ static struct drm_plane *arc_pgu_plane_init(struct 
> drm_device *drm)
>  
>   ret = drm_universal_plane_init(drm, plane, 0xff, _pgu_plane_funcs,
>  formats, ARRAY_SIZE(formats),
> +NULL,
>  DRM_PLANE_TYPE_PRIMARY, NULL);
>   if (ret)
>   return ERR_PTR(ret);
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c 
> b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index 798a3cc480a2..0caa03ae8708 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -303,6 +303,7 @@ static struct drm_plane *hdlcd_plane_init(struct 
> drm_device *drm)
>  
>   ret = drm_universal_plane_init(drm, plane, 0xff, _plane_funcs,
>  formats, ARRAY_SIZE(formats),
> +NULL,
>  DRM_PLANE_TYPE_PRIMARY, NULL);
>   if (ret) 

[Intel-gfx] [PATCH 1/3] drm: Plumb modifiers through plane init

[Ben Widawsky]

v2: A minor addition from Daniel

Cc: Daniel Stone 
Signed-off-by: Ben Widawsky 
---
 drivers/gpu/drm/arc/arcpgu_crtc.c   |  1 +
 drivers/gpu/drm/arm/hdlcd_crtc.c|  1 +
 drivers/gpu/drm/arm/malidp_planes.c |  2 +-
 drivers/gpu/drm/armada/armada_crtc.c|  1 +
 drivers/gpu/drm/armada/armada_overlay.c |  1 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |  4 +++-
 drivers/gpu/drm/drm_modeset_helper.c|  1 +
 drivers/gpu/drm/drm_plane.c | 32 -
 drivers/gpu/drm/drm_simple_kms_helper.c |  3 +++
 drivers/gpu/drm/exynos/exynos_drm_plane.c   |  2 +-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c |  2 +-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  |  1 +
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c |  2 +-
 drivers/gpu/drm/i915/intel_display.c|  7 +-
 drivers/gpu/drm/i915/intel_sprite.c |  4 ++--
 drivers/gpu/drm/imx/ipuv3-plane.c   |  4 ++--
 drivers/gpu/drm/mediatek/mtk_drm_plane.c|  2 +-
 drivers/gpu/drm/meson/meson_plane.c |  1 +
 drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c   |  2 +-
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c   |  4 ++--
 drivers/gpu/drm/mxsfb/mxsfb_drv.c   |  2 +-
 drivers/gpu/drm/nouveau/nv50_display.c  |  5 ++--
 drivers/gpu/drm/omapdrm/omap_plane.c|  3 ++-
 drivers/gpu/drm/qxl/qxl_display.c   |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_plane.c |  4 ++--
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  4 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  4 ++--
 drivers/gpu/drm/sti/sti_cursor.c|  2 +-
 drivers/gpu/drm/sti/sti_gdp.c   |  2 +-
 drivers/gpu/drm/sti/sti_hqvdp.c |  2 +-
 drivers/gpu/drm/sun4i/sun4i_layer.c |  2 +-
 drivers/gpu/drm/tegra/dc.c  | 12 +-
 drivers/gpu/drm/vc4/vc4_plane.c |  2 +-
 drivers/gpu/drm/virtio/virtgpu_plane.c  |  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c |  4 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c|  4 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c|  4 ++--
 drivers/gpu/drm/zte/zx_plane.c  |  2 +-
 include/drm/drm_plane.h | 21 +++-
 include/drm/drm_simple_kms_helper.h |  1 +
 include/uapi/drm/drm_fourcc.h   | 11 +
 41 files changed, 126 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c 
b/drivers/gpu/drm/arc/arcpgu_crtc.c
index ad9a95916f1f..cd8a24c7c67d 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -218,6 +218,7 @@ static struct drm_plane *arc_pgu_plane_init(struct 
drm_device *drm)
 
ret = drm_universal_plane_init(drm, plane, 0xff, _pgu_plane_funcs,
   formats, ARRAY_SIZE(formats),
+  NULL,
   DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 798a3cc480a2..0caa03ae8708 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -303,6 +303,7 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device 
*drm)
 
ret = drm_universal_plane_init(drm, plane, 0xff, _plane_funcs,
   formats, ARRAY_SIZE(formats),
+  NULL,
   DRM_PLANE_TYPE_PRIMARY, NULL);
if (ret) {
devm_kfree(drm->dev, plane);
diff --git a/drivers/gpu/drm/arm/malidp_planes.c 
b/drivers/gpu/drm/arm/malidp_planes.c
index 814fda23cead..b156610c68a5 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -400,7 +400,7 @@ int malidp_de_planes_init(struct drm_device *drm)
DRM_PLANE_TYPE_OVERLAY;
ret = drm_universal_plane_init(drm, >base, crtcs,
   _de_plane_funcs, formats,
-  n, plane_type, NULL);
+  n, NULL, plane_type, NULL);
if (ret < 0)
goto cleanup;
 
diff --git a/drivers/gpu/drm/armada/armada_crtc.c 
b/drivers/gpu/drm/armada/armada_crtc.c
index 4fe19fde84f9..ea48ef88f0e4 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -1269,6 +1269,7 @@ static int armada_drm_crtc_create(struct drm_device *drm, 
struct device *dev,
   _primary_plane_funcs,
   armada_primary_formats,