Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On 03/21/2018 10:58 AM, Oleksandr Andrushchenko wrote: From: Oleksandr Andrushchenko Add support for Xen para-virtualized frontend display driver. Accompanying backend [1] is implemented as a user-space application and its helper library [2], capable of running as a Weston client or DRM master. Configuration of both backend and frontend is done via Xen guest domain configuration options [3]. I won't claim that I really understand what's going on here as far as DRM stuff is concerned but I didn't see any obvious issues with Xen bits. So for that you can tack on my Reviewed-by: Boris Ostrovsky ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On 03/22/2018 03:14 AM, Boris Ostrovsky wrote: On 03/21/2018 10:58 AM, Oleksandr Andrushchenko wrote: From: Oleksandr Andrushchenko Add support for Xen para-virtualized frontend display driver. Accompanying backend [1] is implemented as a user-space application and its helper library [2], capable of running as a Weston client or DRM master. Configuration of both backend and frontend is done via Xen guest domain configuration options [3]. I won't claim that I really understand what's going on here as far as DRM stuff is concerned but I didn't see any obvious issues with Xen bits. So for that you can tack on my Reviewed-by: Boris Ostrovsky Thank you ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
My apologies, but I found a few more things that look strange and should
be cleaned up. Sorry for this iterative review approach, but I think we're
slowly getting there.
Thank you for reviewing!
Cheers, Daniel
---
+static int xen_drm_drv_dumb_create(struct drm_file *filp,
+ struct drm_device *dev, struct drm_mode_create_dumb *args)
+{
+ struct xen_drm_front_drm_info *drm_info = dev->dev_private;
+ struct drm_gem_object *obj;
+ int ret;
+
+ ret = xen_drm_front_gem_dumb_create(filp, dev, args);
+ if (ret)
+ goto fail;
+
+ obj = drm_gem_object_lookup(filp, args->handle);
+ if (!obj) {
+ ret = -ENOENT;
+ goto fail_destroy;
+ }
+
+ drm_gem_object_unreference_unlocked(obj);
You can't drop the reference while you keep using the object, someone else
might sneak in and destroy your object. The unreference always must be
last.
Will fix, thank you
+
+ /*
+* In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed
+* via DRM CMA helpers and doesn't have ->pages allocated
+* (xendrm_gem_get_pages will return NULL), but instead can provide
+* sg table
+*/
+ if (xen_drm_front_gem_get_pages(obj))
+ ret = xen_drm_front_dbuf_create_from_pages(
+ drm_info->front_info,
+ xen_drm_front_dbuf_to_cookie(obj),
+ args->width, args->height, args->bpp,
+ args->size,
+ xen_drm_front_gem_get_pages(obj));
+ else
+ ret = xen_drm_front_dbuf_create_from_sgt(
+ drm_info->front_info,
+ xen_drm_front_dbuf_to_cookie(obj),
+ args->width, args->height, args->bpp,
+ args->size,
+ xen_drm_front_gem_get_sg_table(obj));
+ if (ret)
+ goto fail_destroy;
+
The above also has another race: If you construct an object, then it must
be fully constructed by the time you publish it to the wider world. In gem
this is done by calling drm_gem_handle_create() - after that userspace can
get at your object and do nasty things with it in a separate thread,
forcing your driver to Oops if the object isn't fully constructed yet.
That means you need to redo this code here to make sure that the gem
object is fully set up (including pages and sg tables) _before_ anything
calls drm_gem_handle_create().
You are correct, I need to rework this code
This probably means you also need to open-code the cma side, by first
calling drm_gem_cma_create(), then doing any additional setup, and finally
doing the registration to userspace with drm_gem_handle_create as the very
last thing.
Although I tend to avoid open-coding, but this seems the necessary
measure here
Alternativet is to do the pages/sg setup only when you create an fb (and
drop the pages again when the fb is destroyed), but that requires some
refcounting/locking in the driver.
Not sure this will work: nothing prevents you from attaching multiple
FBs to a single dumb handle
So, not only ref-counting should be done here, but I also need to check
if the dumb buffer,
we are attaching to, has been created already
So, I will rework with open-coding some stuff from CMA helpers
Aside: There's still a lot of indirection and jumping around which makes
the code a bit hard to follow.
Probably I am not sure of which indirection we are talking about, could
you please
specifically mark those annoying you?
+
+static void xen_drm_drv_release(struct drm_device *dev)
+{
+ struct xen_drm_front_drm_info *drm_info = dev->dev_private;
+ struct xen_drm_front_info *front_info = drm_info->front_info;
+
+ drm_atomic_helper_shutdown(dev);
+ drm_mode_config_cleanup(dev);
+
+ xen_drm_front_evtchnl_free_all(front_info);
+ dbuf_free_all(&front_info->dbuf_list);
+
+ drm_dev_fini(dev);
+ kfree(dev);
+
+ /*
+* Free now, as this release could be not due to rmmod, but
+* due to the backend disconnect, making drm_info hang in
+* memory until rmmod
+*/
+ devm_kfree(&front_info->xb_dev->dev, front_info->drm_info);
+ front_info->drm_info = NULL;
+
+ /* Tell the backend we are ready to (re)initialize */
+ xenbus_switch_state(front_info->xb_dev, XenbusStateInitialising);
This needs to be in the unplug code. Yes that means you'll have multiple
drm_devices floating around, but that's how hotplug works. That would also
mean that you need to drop the front_info pointer from the backend at
unplug time.
If you don't like those semantics then the only other option is to never
destroy the drm_device, but only mark the drm_connector as disconnected
when the xenbus backend is gone. But this half-half solution here wher
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On Fri, Mar 23, 2018 at 05:54:49PM +0200, Oleksandr Andrushchenko wrote:
>
> > My apologies, but I found a few more things that look strange and should
> > be cleaned up. Sorry for this iterative review approach, but I think we're
> > slowly getting there.
> Thank you for reviewing!
> > Cheers, Daniel
> >
> > > ---
> > > +static int xen_drm_drv_dumb_create(struct drm_file *filp,
> > > + struct drm_device *dev, struct drm_mode_create_dumb *args)
> > > +{
> > > + struct xen_drm_front_drm_info *drm_info = dev->dev_private;
> > > + struct drm_gem_object *obj;
> > > + int ret;
> > > +
> > > + ret = xen_drm_front_gem_dumb_create(filp, dev, args);
> > > + if (ret)
> > > + goto fail;
> > > +
> > > + obj = drm_gem_object_lookup(filp, args->handle);
> > > + if (!obj) {
> > > + ret = -ENOENT;
> > > + goto fail_destroy;
> > > + }
> > > +
> > > + drm_gem_object_unreference_unlocked(obj);
> > You can't drop the reference while you keep using the object, someone else
> > might sneak in and destroy your object. The unreference always must be
> > last.
> Will fix, thank you
> > > +
> > > + /*
> > > + * In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed
> > > + * via DRM CMA helpers and doesn't have ->pages allocated
> > > + * (xendrm_gem_get_pages will return NULL), but instead can provide
> > > + * sg table
> > > + */
> > > + if (xen_drm_front_gem_get_pages(obj))
> > > + ret = xen_drm_front_dbuf_create_from_pages(
> > > + drm_info->front_info,
> > > + xen_drm_front_dbuf_to_cookie(obj),
> > > + args->width, args->height, args->bpp,
> > > + args->size,
> > > + xen_drm_front_gem_get_pages(obj));
> > > + else
> > > + ret = xen_drm_front_dbuf_create_from_sgt(
> > > + drm_info->front_info,
> > > + xen_drm_front_dbuf_to_cookie(obj),
> > > + args->width, args->height, args->bpp,
> > > + args->size,
> > > + xen_drm_front_gem_get_sg_table(obj));
> > > + if (ret)
> > > + goto fail_destroy;
> > > +
> > The above also has another race: If you construct an object, then it must
> > be fully constructed by the time you publish it to the wider world. In gem
> > this is done by calling drm_gem_handle_create() - after that userspace can
> > get at your object and do nasty things with it in a separate thread,
> > forcing your driver to Oops if the object isn't fully constructed yet.
> >
> > That means you need to redo this code here to make sure that the gem
> > object is fully set up (including pages and sg tables) _before_ anything
> > calls drm_gem_handle_create().
> You are correct, I need to rework this code
> >
> > This probably means you also need to open-code the cma side, by first
> > calling drm_gem_cma_create(), then doing any additional setup, and finally
> > doing the registration to userspace with drm_gem_handle_create as the very
> > last thing.
> Although I tend to avoid open-coding, but this seems the necessary measure
> here
> >
> > Alternativet is to do the pages/sg setup only when you create an fb (and
> > drop the pages again when the fb is destroyed), but that requires some
> > refcounting/locking in the driver.
> Not sure this will work: nothing prevents you from attaching multiple FBs to
> a single dumb handle
> So, not only ref-counting should be done here, but I also need to check if
> the dumb buffer,
> we are attaching to, has been created already
No, you must make sure that no dumb buffer can be seen by anyone else
before it's fully created. If you don't register it in the file_priv idr
using drm_gem_handle_create, no one else can get at your buffer. Trying to
paper over this race from all the other places breaks the gem core code
design, and is also much more fragile.
> So, I will rework with open-coding some stuff from CMA helpers
>
> >
> > Aside: There's still a lot of indirection and jumping around which makes
> > the code a bit hard to follow.
> Probably I am not sure of which indirection we are talking about, could you
> please
> specifically mark those annoying you?
I think it's the same indirection we talked about last time, it still
annoys me. But it's still ok if you prefer this way I think :-)
>
> >
> > > +
> > > +static void xen_drm_drv_release(struct drm_device *dev)
> > > +{
> > > + struct xen_drm_front_drm_info *drm_info = dev->dev_private;
> > > + struct xen_drm_front_info *front_info = drm_info->front_info;
> > > +
> > > + drm_atomic_helper_shutdown(dev);
> > > + drm_mode_config_cleanup(dev);
> > > +
> > > + xen_drm_front_evtchnl_free_all(front_info);
> > > + dbuf_free_all(&front_info->dbuf_list);
> > > +
> > > + drm_dev_fini(dev);
> > > + kfree(dev);
> > > +
> > > + /*
> > > + * Free now, as this release could be not due to rmmod, but
> > > + * due to the backend disconnect,
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On 03/26/2018 11:18 AM, Daniel Vetter wrote:
On Fri, Mar 23, 2018 at 05:54:49PM +0200, Oleksandr Andrushchenko wrote:
My apologies, but I found a few more things that look strange and should
be cleaned up. Sorry for this iterative review approach, but I think we're
slowly getting there.
Thank you for reviewing!
Cheers, Daniel
---
+static int xen_drm_drv_dumb_create(struct drm_file *filp,
+ struct drm_device *dev, struct drm_mode_create_dumb *args)
+{
+ struct xen_drm_front_drm_info *drm_info = dev->dev_private;
+ struct drm_gem_object *obj;
+ int ret;
+
+ ret = xen_drm_front_gem_dumb_create(filp, dev, args);
+ if (ret)
+ goto fail;
+
+ obj = drm_gem_object_lookup(filp, args->handle);
+ if (!obj) {
+ ret = -ENOENT;
+ goto fail_destroy;
+ }
+
+ drm_gem_object_unreference_unlocked(obj);
You can't drop the reference while you keep using the object, someone else
might sneak in and destroy your object. The unreference always must be
last.
Will fix, thank you
+
+ /*
+* In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed
+* via DRM CMA helpers and doesn't have ->pages allocated
+* (xendrm_gem_get_pages will return NULL), but instead can provide
+* sg table
+*/
+ if (xen_drm_front_gem_get_pages(obj))
+ ret = xen_drm_front_dbuf_create_from_pages(
+ drm_info->front_info,
+ xen_drm_front_dbuf_to_cookie(obj),
+ args->width, args->height, args->bpp,
+ args->size,
+ xen_drm_front_gem_get_pages(obj));
+ else
+ ret = xen_drm_front_dbuf_create_from_sgt(
+ drm_info->front_info,
+ xen_drm_front_dbuf_to_cookie(obj),
+ args->width, args->height, args->bpp,
+ args->size,
+ xen_drm_front_gem_get_sg_table(obj));
+ if (ret)
+ goto fail_destroy;
+
The above also has another race: If you construct an object, then it must
be fully constructed by the time you publish it to the wider world. In gem
this is done by calling drm_gem_handle_create() - after that userspace can
get at your object and do nasty things with it in a separate thread,
forcing your driver to Oops if the object isn't fully constructed yet.
That means you need to redo this code here to make sure that the gem
object is fully set up (including pages and sg tables) _before_ anything
calls drm_gem_handle_create().
You are correct, I need to rework this code
This probably means you also need to open-code the cma side, by first
calling drm_gem_cma_create(), then doing any additional setup, and finally
doing the registration to userspace with drm_gem_handle_create as the very
last thing.
Although I tend to avoid open-coding, but this seems the necessary measure
here
Alternativet is to do the pages/sg setup only when you create an fb (and
drop the pages again when the fb is destroyed), but that requires some
refcounting/locking in the driver.
Not sure this will work: nothing prevents you from attaching multiple FBs to
a single dumb handle
So, not only ref-counting should be done here, but I also need to check if
the dumb buffer,
we are attaching to, has been created already
No, you must make sure that no dumb buffer can be seen by anyone else
before it's fully created. If you don't register it in the file_priv idr
using drm_gem_handle_create, no one else can get at your buffer. Trying to
paper over this race from all the other places breaks the gem core code
design, and is also much more fragile.
Yes, this is what I implement now, e.g. I do not create
any dumb handle until GEM is fully created. I was just
saying that alternative way when we do pages/sgt on FB
attach will not work in my case
So, I will rework with open-coding some stuff from CMA helpers
Aside: There's still a lot of indirection and jumping around which makes
the code a bit hard to follow.
Probably I am not sure of which indirection we are talking about, could you
please
specifically mark those annoying you?
I think it's the same indirection we talked about last time, it still
annoys me. But it's still ok if you prefer this way I think :-)
Ok, probably this is because I'm looking at the driver
from an editor, but you are from your mail client ;)
+
+static void xen_drm_drv_release(struct drm_device *dev)
+{
+ struct xen_drm_front_drm_info *drm_info = dev->dev_private;
+ struct xen_drm_front_info *front_info = drm_info->front_info;
+
+ drm_atomic_helper_shutdown(dev);
+ drm_mode_config_cleanup(dev);
+
+ xen_drm_front_evtchnl_free_all(front_info);
+ dbuf_free_all(&front_info->dbuf_list);
+
+ drm_dev_fini(dev);
+ kfree(dev);
+
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
Hi, Daniel!
On 03/26/2018 03:46 PM, Oleksandr Andrushchenko wrote:
On 03/26/2018 11:18 AM, Daniel Vetter wrote:
On Fri, Mar 23, 2018 at 05:54:49PM +0200, Oleksandr Andrushchenko wrote:
My apologies, but I found a few more things that look strange and
should
be cleaned up. Sorry for this iterative review approach, but I
think we're
slowly getting there.
Thank you for reviewing!
Cheers, Daniel
---
+static int xen_drm_drv_dumb_create(struct drm_file *filp,
+ struct drm_device *dev, struct drm_mode_create_dumb *args)
+{
+ struct xen_drm_front_drm_info *drm_info = dev->dev_private;
+ struct drm_gem_object *obj;
+ int ret;
+
+ ret = xen_drm_front_gem_dumb_create(filp, dev, args);
+ if (ret)
+ goto fail;
+
+ obj = drm_gem_object_lookup(filp, args->handle);
+ if (!obj) {
+ ret = -ENOENT;
+ goto fail_destroy;
+ }
+
+ drm_gem_object_unreference_unlocked(obj);
You can't drop the reference while you keep using the object,
someone else
might sneak in and destroy your object. The unreference always must be
last.
Will fix, thank you
+
+ /*
+ * In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed
+ * via DRM CMA helpers and doesn't have ->pages allocated
+ * (xendrm_gem_get_pages will return NULL), but instead can
provide
+ * sg table
+ */
+ if (xen_drm_front_gem_get_pages(obj))
+ ret = xen_drm_front_dbuf_create_from_pages(
+ drm_info->front_info,
+ xen_drm_front_dbuf_to_cookie(obj),
+ args->width, args->height, args->bpp,
+ args->size,
+ xen_drm_front_gem_get_pages(obj));
+ else
+ ret = xen_drm_front_dbuf_create_from_sgt(
+ drm_info->front_info,
+ xen_drm_front_dbuf_to_cookie(obj),
+ args->width, args->height, args->bpp,
+ args->size,
+ xen_drm_front_gem_get_sg_table(obj));
+ if (ret)
+ goto fail_destroy;
+
The above also has another race: If you construct an object, then
it must
be fully constructed by the time you publish it to the wider world.
In gem
this is done by calling drm_gem_handle_create() - after that
userspace can
get at your object and do nasty things with it in a separate thread,
forcing your driver to Oops if the object isn't fully constructed yet.
That means you need to redo this code here to make sure that the gem
object is fully set up (including pages and sg tables) _before_
anything
calls drm_gem_handle_create().
You are correct, I need to rework this code
This probably means you also need to open-code the cma side, by first
calling drm_gem_cma_create(), then doing any additional setup, and
finally
doing the registration to userspace with drm_gem_handle_create as
the very
last thing.
Although I tend to avoid open-coding, but this seems the necessary
measure
here
Alternativet is to do the pages/sg setup only when you create an fb
(and
drop the pages again when the fb is destroyed), but that requires some
refcounting/locking in the driver.
Not sure this will work: nothing prevents you from attaching
multiple FBs to
a single dumb handle
So, not only ref-counting should be done here, but I also need to
check if
the dumb buffer,
we are attaching to, has been created already
No, you must make sure that no dumb buffer can be seen by anyone else
before it's fully created. If you don't register it in the file_priv idr
using drm_gem_handle_create, no one else can get at your buffer.
Trying to
paper over this race from all the other places breaks the gem core code
design, and is also much more fragile.
Yes, this is what I implement now, e.g. I do not create
any dumb handle until GEM is fully created. I was just
saying that alternative way when we do pages/sgt on FB
attach will not work in my case
So, I will rework with open-coding some stuff from CMA helpers
Aside: There's still a lot of indirection and jumping around which
makes
the code a bit hard to follow.
Probably I am not sure of which indirection we are talking about,
could you
please
specifically mark those annoying you?
I think it's the same indirection we talked about last time, it still
annoys me. But it's still ok if you prefer this way I think :-)
Ok, probably this is because I'm looking at the driver
from an editor, but you are from your mail client ;)
+
+static void xen_drm_drv_release(struct drm_device *dev)
+{
+ struct xen_drm_front_drm_info *drm_info = dev->dev_private;
+ struct xen_drm_front_info *front_info = drm_info->front_info;
+
+ drm_atomic_helper_shutdown(dev);
+ drm_mode_config_cleanup(dev);
+
+ xen_drm_front_evtchnl_free_all(front_info);
+ dbuf_free_all(&front_info->dbuf_list);
+
+ drm_dev_fini(dev);
+ kfree(dev);
+
+ /*
+ * Free now, as this release could be not due to rmmod, but
+ * due to the backend disconnect, making drm_info hang in
+ * memory until rmmod
+ */
+ devm_kfre
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On Tue, Mar 27, 2018 at 11:34 AM, Oleksandr Andrushchenko
wrote:
> Hi, Daniel!
>
>
> On 03/26/2018 03:46 PM, Oleksandr Andrushchenko wrote:
>>
>> On 03/26/2018 11:18 AM, Daniel Vetter wrote:
>>>
>>> On Fri, Mar 23, 2018 at 05:54:49PM +0200, Oleksandr Andrushchenko wrote:
>
> My apologies, but I found a few more things that look strange and
> should
> be cleaned up. Sorry for this iterative review approach, but I think
> we're
> slowly getting there.
Thank you for reviewing!
>
> Cheers, Daniel
>
>> ---
>> +static int xen_drm_drv_dumb_create(struct drm_file *filp,
>> +struct drm_device *dev, struct drm_mode_create_dumb *args)
>> +{
>> +struct xen_drm_front_drm_info *drm_info = dev->dev_private;
>> +struct drm_gem_object *obj;
>> +int ret;
>> +
>> +ret = xen_drm_front_gem_dumb_create(filp, dev, args);
>> +if (ret)
>> +goto fail;
>> +
>> +obj = drm_gem_object_lookup(filp, args->handle);
>> +if (!obj) {
>> +ret = -ENOENT;
>> +goto fail_destroy;
>> +}
>> +
>> +drm_gem_object_unreference_unlocked(obj);
>
> You can't drop the reference while you keep using the object, someone
> else
> might sneak in and destroy your object. The unreference always must be
> last.
Will fix, thank you
>>
>> +
>> +/*
>> + * In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed
>> + * via DRM CMA helpers and doesn't have ->pages allocated
>> + * (xendrm_gem_get_pages will return NULL), but instead can
>> provide
>> + * sg table
>> + */
>> +if (xen_drm_front_gem_get_pages(obj))
>> +ret = xen_drm_front_dbuf_create_from_pages(
>> +drm_info->front_info,
>> +xen_drm_front_dbuf_to_cookie(obj),
>> +args->width, args->height, args->bpp,
>> +args->size,
>> +xen_drm_front_gem_get_pages(obj));
>> +else
>> +ret = xen_drm_front_dbuf_create_from_sgt(
>> +drm_info->front_info,
>> +xen_drm_front_dbuf_to_cookie(obj),
>> +args->width, args->height, args->bpp,
>> +args->size,
>> +xen_drm_front_gem_get_sg_table(obj));
>> +if (ret)
>> +goto fail_destroy;
>> +
>
> The above also has another race: If you construct an object, then it
> must
> be fully constructed by the time you publish it to the wider world. In
> gem
> this is done by calling drm_gem_handle_create() - after that userspace
> can
> get at your object and do nasty things with it in a separate thread,
> forcing your driver to Oops if the object isn't fully constructed yet.
>
> That means you need to redo this code here to make sure that the gem
> object is fully set up (including pages and sg tables) _before_
> anything
> calls drm_gem_handle_create().
You are correct, I need to rework this code
>
> This probably means you also need to open-code the cma side, by first
> calling drm_gem_cma_create(), then doing any additional setup, and
> finally
> doing the registration to userspace with drm_gem_handle_create as the
> very
> last thing.
Although I tend to avoid open-coding, but this seems the necessary
measure
here
>
> Alternativet is to do the pages/sg setup only when you create an fb
> (and
> drop the pages again when the fb is destroyed), but that requires some
> refcounting/locking in the driver.
Not sure this will work: nothing prevents you from attaching multiple
FBs to
a single dumb handle
So, not only ref-counting should be done here, but I also need to check
if
the dumb buffer,
we are attaching to, has been created already
>>>
>>> No, you must make sure that no dumb buffer can be seen by anyone else
>>> before it's fully created. If you don't register it in the file_priv idr
>>> using drm_gem_handle_create, no one else can get at your buffer. Trying
>>> to
>>> paper over this race from all the other places breaks the gem core code
>>> design, and is also much more fragile.
>>
>> Yes, this is what I implement now, e.g. I do not create
>> any dumb handle until GEM is fully created. I was just
>> saying that alternative way when we do pages/sgt on FB
>> attach will not work in my case
So, I will rework with open-coding some stuff from CMA helpers
> Aside: There's still a lot of indirection and jumping around which
> makes
> the code a bit hard to follow.
Probably I am not sure of which indirection we are talking about, could
you
please
specifically mark those annoying you?
>>>
>>> I think it's the same indirection we talked a
Re: [Xen-devel] [PATCH v3] drm/xen-front: Add support for Xen PV display frontend
On 03/27/2018 12:50 PM, Daniel Vetter wrote:
On Tue, Mar 27, 2018 at 11:34 AM, Oleksandr Andrushchenko
wrote:
Hi, Daniel!
On 03/26/2018 03:46 PM, Oleksandr Andrushchenko wrote:
On 03/26/2018 11:18 AM, Daniel Vetter wrote:
On Fri, Mar 23, 2018 at 05:54:49PM +0200, Oleksandr Andrushchenko wrote:
My apologies, but I found a few more things that look strange and
should
be cleaned up. Sorry for this iterative review approach, but I think
we're
slowly getting there.
Thank you for reviewing!
Cheers, Daniel
---
+static int xen_drm_drv_dumb_create(struct drm_file *filp,
+struct drm_device *dev, struct drm_mode_create_dumb *args)
+{
+struct xen_drm_front_drm_info *drm_info = dev->dev_private;
+struct drm_gem_object *obj;
+int ret;
+
+ret = xen_drm_front_gem_dumb_create(filp, dev, args);
+if (ret)
+goto fail;
+
+obj = drm_gem_object_lookup(filp, args->handle);
+if (!obj) {
+ret = -ENOENT;
+goto fail_destroy;
+}
+
+drm_gem_object_unreference_unlocked(obj);
You can't drop the reference while you keep using the object, someone
else
might sneak in and destroy your object. The unreference always must be
last.
Will fix, thank you
+
+/*
+ * In case of CONFIG_DRM_XEN_FRONTEND_CMA gem_obj is constructed
+ * via DRM CMA helpers and doesn't have ->pages allocated
+ * (xendrm_gem_get_pages will return NULL), but instead can
provide
+ * sg table
+ */
+if (xen_drm_front_gem_get_pages(obj))
+ret = xen_drm_front_dbuf_create_from_pages(
+drm_info->front_info,
+xen_drm_front_dbuf_to_cookie(obj),
+args->width, args->height, args->bpp,
+args->size,
+xen_drm_front_gem_get_pages(obj));
+else
+ret = xen_drm_front_dbuf_create_from_sgt(
+drm_info->front_info,
+xen_drm_front_dbuf_to_cookie(obj),
+args->width, args->height, args->bpp,
+args->size,
+xen_drm_front_gem_get_sg_table(obj));
+if (ret)
+goto fail_destroy;
+
The above also has another race: If you construct an object, then it
must
be fully constructed by the time you publish it to the wider world. In
gem
this is done by calling drm_gem_handle_create() - after that userspace
can
get at your object and do nasty things with it in a separate thread,
forcing your driver to Oops if the object isn't fully constructed yet.
That means you need to redo this code here to make sure that the gem
object is fully set up (including pages and sg tables) _before_
anything
calls drm_gem_handle_create().
You are correct, I need to rework this code
This probably means you also need to open-code the cma side, by first
calling drm_gem_cma_create(), then doing any additional setup, and
finally
doing the registration to userspace with drm_gem_handle_create as the
very
last thing.
Although I tend to avoid open-coding, but this seems the necessary
measure
here
Alternativet is to do the pages/sg setup only when you create an fb
(and
drop the pages again when the fb is destroyed), but that requires some
refcounting/locking in the driver.
Not sure this will work: nothing prevents you from attaching multiple
FBs to
a single dumb handle
So, not only ref-counting should be done here, but I also need to check
if
the dumb buffer,
we are attaching to, has been created already
No, you must make sure that no dumb buffer can be seen by anyone else
before it's fully created. If you don't register it in the file_priv idr
using drm_gem_handle_create, no one else can get at your buffer. Trying
to
paper over this race from all the other places breaks the gem core code
design, and is also much more fragile.
Yes, this is what I implement now, e.g. I do not create
any dumb handle until GEM is fully created. I was just
saying that alternative way when we do pages/sgt on FB
attach will not work in my case
So, I will rework with open-coding some stuff from CMA helpers
Aside: There's still a lot of indirection and jumping around which
makes
the code a bit hard to follow.
Probably I am not sure of which indirection we are talking about, could
you
please
specifically mark those annoying you?
I think it's the same indirection we talked about last time, it still
annoys me. But it's still ok if you prefer this way I think :-)
Ok, probably this is because I'm looking at the driver
from an editor, but you are from your mail client ;)
+
+static void xen_drm_drv_release(struct drm_device *dev)
+{
+struct xen_drm_front_drm_info *drm_info = dev->dev_private;
+struct xen_drm_front_info *front_info = drm_info->front_info;
+
+drm_atomic_helper_shutdown(dev);
+drm_mode_config_cleanup(dev);
+
+xen_drm_front_evtchnl_free_all(front_info);
+dbuf_free_all(&front_info->dbuf_list);
+
+drm_dev_fini(dev);
+kfree(dev);
+
+/*
+ * Free now, as this release could be not due to rmmod, but
+ * due to the ba
