[PATCH 1/2] staging/vboxvideo: don't set dev_priv_size = 0

[PATCH 1/2] staging/vboxvideo: don't set dev_priv_size = 0

[Daniel Vetter]

The compiler already clears this for us.

More important, someone might look what this is actually used for,
and freak out about the dragon staring back at them.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Nicholas Mc Guire <der.herr@hofr.at>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: Fabio Rafael da Rosa <fdr@pid42.net>
---
 drivers/staging/vboxvideo/vbox_drv.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c
index b0d73d5fba5d..43c3d0a4fa1a 100644
--- a/drivers/staging/vboxvideo/vbox_drv.c
+++ b/drivers/staging/vboxvideo/vbox_drv.c
@@ -222,7 +222,6 @@ static void vbox_master_drop(struct drm_device *dev, struct drm_file *file_priv)
 static struct drm_driver driver = {
 	.driver_features =
 	    DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_ATOMIC,
-	.dev_priv_size = 0,
 
 	.lastclose = drm_fb_helper_lastclose,
 	.master_set = vbox_master_set,
-- 
2.20.1

[PATCH 2/2] staging/vboxvideo: Add TODO

[Daniel Vetter]

Noticed why wonder what vboxvideo is using the ->master_set/drop hooks
for.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Fabio Rafael da Rosa <fdr@pid42.net>
Cc: Nicholas Mc Guire <der.herr@hofr.at>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/vboxvideo/TODO | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/vboxvideo/TODO b/drivers/staging/vboxvideo/TODO
index 2e0f99c3f10c..2953e7f794fb 100644
--- a/drivers/staging/vboxvideo/TODO
+++ b/drivers/staging/vboxvideo/TODO
@@ -1,5 +1,7 @@
 TODO:
 -Get a full review from the drm-maintainers on dri-devel done on this driver
+-Drop all the logic around initial_mode_queried/master_set&_drop and everything
+related to this. kms clients can handle hotplugs.
 -Extend this TODO with the results of that review
 
 Please send any patches to Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
-- 
2.20.1

Re: [PATCH 2/2] staging/vboxvideo: Add TODO

[Greg Kroah-Hartman]

On Mon, Feb 04, 2019 at 11:31:14AM +0100, Daniel Vetter wrote:
> Noticed why wonder what vboxvideo is using the ->master_set/drop hooks
> for.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Fabio Rafael da Rosa <fdr@pid42.net>
> Cc: Nicholas Mc Guire <der.herr@hofr.at>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Hans de Goede <hdegoede@redhat.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/2] staging/vboxvideo: Add TODO

[Sam Ravnborg]

Hi Daniel

On Mon, Feb 04, 2019 at 11:31:14AM +0100, Daniel Vetter wrote:
> Noticed why wonder what vboxvideo is using the ->master_set/drop hooks
> for.
Can you improve the gammar a little, I find it hard to read.

> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Fabio Rafael da Rosa <fdr@pid42.net>
> Cc: Nicholas Mc Guire <der.herr@hofr.at>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/staging/vboxvideo/TODO | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/vboxvideo/TODO b/drivers/staging/vboxvideo/TODO
> index 2e0f99c3f10c..2953e7f794fb 100644
> --- a/drivers/staging/vboxvideo/TODO
> +++ b/drivers/staging/vboxvideo/TODO
> @@ -1,5 +1,7 @@
>  TODO:
>  -Get a full review from the drm-maintainers on dri-devel done on this driver
> +-Drop all the logic around initial_mode_queried/master_set&_drop and everything
> +related to this. kms clients can handle hotplugs.
>  -Extend this TODO with the results of that review
>  
>  Please send any patches to Greg Kroah-Hartman <gregkh@linuxfoundation.org>,

The syntax around "master_set&_drop" could be better.
I wondered if the "&" was some rst syntax.

But anyway, despite grammar nit and syntax nit:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Which bring me back to a question asked a week ago or so.
What is missing before we can move vboxvideo out of staging/

Could we carry a few TODO items over in drm/ ar do we need a clean TODO?

	Sam

Re: [PATCH 2/2] staging/vboxvideo: Add TODO

[Daniel Vetter]

On Mon, Feb 04, 2019 at 07:54:16PM +0100, Sam Ravnborg wrote:
> Hi Daniel
> 
> On Mon, Feb 04, 2019 at 11:31:14AM +0100, Daniel Vetter wrote:
> > Noticed why wonder what vboxvideo is using the ->master_set/drop hooks
> > for.
> Can you improve the gammar a little, I find it hard to read.
> 
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Fabio Rafael da Rosa <fdr@pid42.net>
> > Cc: Nicholas Mc Guire <der.herr@hofr.at>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/staging/vboxvideo/TODO | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/staging/vboxvideo/TODO b/drivers/staging/vboxvideo/TODO
> > index 2e0f99c3f10c..2953e7f794fb 100644
> > --- a/drivers/staging/vboxvideo/TODO
> > +++ b/drivers/staging/vboxvideo/TODO
> > @@ -1,5 +1,7 @@
> >  TODO:
> >  -Get a full review from the drm-maintainers on dri-devel done on this driver
> > +-Drop all the logic around initial_mode_queried/master_set&_drop and everything
> > +related to this. kms clients can handle hotplugs.
> >  -Extend this TODO with the results of that review
> >  
> >  Please send any patches to Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
> 
> The syntax around "master_set&_drop" could be better.
> I wondered if the "&" was some rst syntax.
> 
> But anyway, despite grammar nit and syntax nit:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Both patches queued for 5.1 in drm-misc, thanks for taking a look.

> Which bring me back to a question asked a week ago or so.
> What is missing before we can move vboxvideo out of staging/

I think it boils down to someone needs to submit it and we'll take a look.

> Could we carry a few TODO items over in drm/ ar do we need a clean TODO?

We carry todos in drm too, it's going to be a judgement call.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] staging/vboxvideo: Add TODO

[Hans de Goede]

Hi,

On 06-02-19 16:58, Daniel Vetter wrote:
> On Mon, Feb 04, 2019 at 07:54:16PM +0100, Sam Ravnborg wrote:
>> Hi Daniel
>>
>> On Mon, Feb 04, 2019 at 11:31:14AM +0100, Daniel Vetter wrote:
>>> Noticed why wonder what vboxvideo is using the ->master_set/drop hooks
>>> for.
>> Can you improve the gammar a little, I find it hard to read.
>>
>>>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Fabio Rafael da Rosa <fdr@pid42.net>
>>> Cc: Nicholas Mc Guire <der.herr@hofr.at>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   drivers/staging/vboxvideo/TODO | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/staging/vboxvideo/TODO b/drivers/staging/vboxvideo/TODO
>>> index 2e0f99c3f10c..2953e7f794fb 100644
>>> --- a/drivers/staging/vboxvideo/TODO
>>> +++ b/drivers/staging/vboxvideo/TODO
>>> @@ -1,5 +1,7 @@
>>>   TODO:
>>>   -Get a full review from the drm-maintainers on dri-devel done on this driver
>>> +-Drop all the logic around initial_mode_queried/master_set&_drop and everything
>>> +related to this. kms clients can handle hotplugs.
>>>   -Extend this TODO with the results of that review
>>>   
>>>   Please send any patches to Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
>>
>> The syntax around "master_set&_drop" could be better.
>> I wondered if the "&" was some rst syntax.
>>
>> But anyway, despite grammar nit and syntax nit:
>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> Both patches queued for 5.1 in drm-misc, thanks for taking a look.
> 
>> Which bring me back to a question asked a week ago or so.
>> What is missing before we can move vboxvideo out of staging/
> 
> I think it boils down to someone needs to submit it and we'll take a look.

Right, I have this on my TODO, but I did not manage to find time for this
the past few weeks. I expect to have time for this in a couple of weeks
from now, which means the move will miss the 5.1 merge window.

Note you (Sam) are certainly welcome to submit a patch doing the move
yourself, I can certainly use some help with maintaining vboxvideo.

Note I will be available to help answer questions resulting from a
review for moving the driver out of staging. And more in general, if you
want to do this I will try to help where I can.

Regards,

Hans

Re: [PATCH 2/2] staging/vboxvideo: Add TODO

[Sam Ravnborg]

Hi Hans
> >
> >>Which bring me back to a question asked a week ago or so.
> >>What is missing before we can move vboxvideo out of staging/
> >
> >I think it boils down to someone needs to submit it and we'll take a look.
> 
> Right, I have this on my TODO, but I did not manage to find time for this
> the past few weeks. I expect to have time for this in a couple of weeks
> from now, which means the move will miss the 5.1 merge window.
> 
> Note you (Sam) are certainly welcome to submit a patch doing the move
> yourself, I can certainly use some help with maintaining vboxvideo.

I have not tried to submit patches to move vboxvideo out of staging.
Partially due to lack of time, mostly lack of competence to
follow-up on the review comments I hope we will see.
I am perfectly happy to do trivial stuff, but when someone ask
to replace the use of ttm with shmem+gem (example - I do not
know if this is a relevant comment), then it is above
me for the moment.
And on top of this then I lack a good testing environment.

I could do it but then anything difficult would just be added
to a TODO file, and then I dunno how useful it is.
Something to consider when we are past the merge window anyway.

	Sam

Re: [PATCH 2/2] staging/vboxvideo: Add TODO

[Hans de Goede]

Hi,

On 13-02-19 19:46, Sam Ravnborg wrote:
> Hi Hans
>>>
>>>> Which bring me back to a question asked a week ago or so.
>>>> What is missing before we can move vboxvideo out of staging/
>>>
>>> I think it boils down to someone needs to submit it and we'll take a look.
>>
>> Right, I have this on my TODO, but I did not manage to find time for this
>> the past few weeks. I expect to have time for this in a couple of weeks
>> from now, which means the move will miss the 5.1 merge window.
>>
>> Note you (Sam) are certainly welcome to submit a patch doing the move
>> yourself, I can certainly use some help with maintaining vboxvideo.
> 
> I have not tried to submit patches to move vboxvideo out of staging.
> Partially due to lack of time, mostly lack of competence to
> follow-up on the review comments I hope we will see.
> I am perfectly happy to do trivial stuff, but when someone ask
> to replace the use of ttm with shmem+gem (example - I do not
> know if this is a relevant comment), then it is above
> me for the moment.
> And on top of this then I lack a good testing environment.
> 
> I could do it but then anything difficult would just be added
> to a TODO file, and then I dunno how useful it is.
> Something to consider when we are past the merge window anyway.

Ok, I will try to submit a patch doing the move myself soon.

Regards,

Hans

Re: [PATCH 1/2] staging/vboxvideo: don't set dev_priv_size = 0

[Greg Kroah-Hartman]

On Mon, Feb 04, 2019 at 11:31:13AM +0100, Daniel Vetter wrote:
> The compiler already clears this for us.
> 
> More important, someone might look what this is actually used for,
> and freak out about the dragon staring back at them.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Nicholas Mc Guire <der.herr@hofr.at>
> Cc: Emil Velikov <emil.velikov@collabora.com>
> Cc: Fabio Rafael da Rosa <fdr@pid42.net>
> ---
>  drivers/staging/vboxvideo/vbox_drv.c | 1 -
>  1 file changed, 1 deletion(-)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 1/2] staging/vboxvideo: don't set dev_priv_size = 0

[Sam Ravnborg]

Hi Daniel

On Mon, Feb 04, 2019 at 11:31:13AM +0100, Daniel Vetter wrote:
> The compiler already clears this for us.
> 
> More important, someone might look what this is actually used for,
> and freak out about the dragon staring back at them.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Nicholas Mc Guire <der.herr@hofr.at>
> Cc: Emil Velikov <emil.velikov@collabora.com>
> Cc: Fabio Rafael da Rosa <fdr@pid42.net>
> ---
>  drivers/staging/vboxvideo/vbox_drv.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c
> index b0d73d5fba5d..43c3d0a4fa1a 100644
> --- a/drivers/staging/vboxvideo/vbox_drv.c
> +++ b/drivers/staging/vboxvideo/vbox_drv.c
> @@ -222,7 +222,6 @@ static void vbox_master_drop(struct drm_device *dev, struct drm_file *file_priv)
>  static struct drm_driver driver = {
>  	.driver_features =
>  	    DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_ATOMIC,
> -	.dev_priv_size = 0,
>  
>  	.lastclose = drm_fb_helper_lastclose,
>  	.master_set = vbox_master_set,

I have stared at the file for a long time and so far no dragon
was staring back at me. There was a few "#ifdef" that screamed
at me, and a drm_fb_helper_fbdev_setup() that looked
suspicious alas no dragon :-(

As for the change above, dragon or no dragon:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

Re: [PATCH 1/2] staging/vboxvideo: don't set dev_priv_size = 0

[Daniel Vetter]

On Mon, Feb 4, 2019 at 7:49 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Daniel
>
> On Mon, Feb 04, 2019 at 11:31:13AM +0100, Daniel Vetter wrote:
> > The compiler already clears this for us.
> >
> > More important, someone might look what this is actually used for,
> > and freak out about the dragon staring back at them.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Nicholas Mc Guire <der.herr@hofr.at>
> > Cc: Emil Velikov <emil.velikov@collabora.com>
> > Cc: Fabio Rafael da Rosa <fdr@pid42.net>
> > ---
> >  drivers/staging/vboxvideo/vbox_drv.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/staging/vboxvideo/vbox_drv.c b/drivers/staging/vboxvideo/vbox_drv.c
> > index b0d73d5fba5d..43c3d0a4fa1a 100644
> > --- a/drivers/staging/vboxvideo/vbox_drv.c
> > +++ b/drivers/staging/vboxvideo/vbox_drv.c
> > @@ -222,7 +222,6 @@ static void vbox_master_drop(struct drm_device *dev, struct drm_file *file_priv)
> >  static struct drm_driver driver = {
> >       .driver_features =
> >           DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_ATOMIC,
> > -     .dev_priv_size = 0,
> >
> >       .lastclose = drm_fb_helper_lastclose,
> >       .master_set = vbox_master_set,
>
> I have stared at the file for a long time and so far no dragon
> was staring back at me. There was a few "#ifdef" that screamed
> at me, and a drm_fb_helper_fbdev_setup() that looked
> suspicious alas no dragon :-(

dev_priv_size is used by drm_bufs.c aka "you want a root-hole? have
it!" code from dri1 days. It's not running on any modern driver, at
least trinity/syzcaller stopped complaining (and I reviewed all the
entry points and made sure they go nowhere else than an immediate
return -errno). Except nouveau, for reasons (we accidentally made it
uapi there, but it's fixed, just need to wait for all those installs
to die so we can nuke it for good). The dragon was right there
breathing down your neck, and wouldn't have seen it coming  if it
decided to have a snack :-)

btw if you're bored, we should probably have a
CONFIG_FEWER_EXPLOITS_IN_DRM_NOUVEAU or so, default n, for the next
unaware traveller wandering into this dragon den.
-Daniel

> As for the change above, dragon or no dragon:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>
>         Sam



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch