Re: [PATCH v3] drm/xen-front: Add support for Xen PV display frontend

[Oleksandr Andrushchenko]

> 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 where
> you hotunplug the drm_device but want to keep it around still doesn't work
> from a livetime pov.
I'll try to play with this:

on backend disconnect I will do the following:
ÂÂÂ drm_dev_unplug(dev)
ÂÂÂ xen_drm_front_evtchnl_free_all(front_info);
ÂÂÂ dbuf_free_all(&front_info->dbuf_list);
ÂÂÂ devm_kfree(&front_info->xb_dev->dev, front_info->drm_info);
ÂÂÂ front_info->drm_info = NULL;
ÂÂÂ xenbus_switch_state(front_info->xb_dev, XenbusStateInitialising);

on drm_driver.release callback:

ÂÂÂ drm_atomic_helper_shutdown(dev);
ÂÂÂ drm_mode_config_cleanup(dev);

ÂÂÂ drm_dev_fini(dev);
ÂÂÂ kfree(dev);

Does the above make sense?

> > +static struct xenbus_driver xen_driver = {
> > +	.ids = xen_driver_ids,
> > +	.probe = xen_drv_probe,
> > +	.remove = xen_drv_remove,
> I still don't understand why you have both the remove and fini versions of
> this. See other comments, I think the xenbus vs. drm_device lifetime stuff
> still needs to be cleaned up some more. This shouldn't be that hard
> really.
>
> Or maybe I'm just totally misunderstanding this frontend vs. backend split
> in xen, so if you have a nice gentle intro text for why that exists, it
> might help.
Probably misunderstanding comes from the fact that it is possible if backend
dies it may still have its XenBus state set to connected, thus
displback_disconnect callback will never be called. For that reason on rmmod
I call fini for the DRM driver to destroy it.

> > +	/*
> > +	 * pflip_timeout is set to current jiffies once we send a page flip and
> > +	 * reset to 0 when we receive frame done event from the backed.
> > +	 * It is checked during drm_connector_helper_funcs.detect_ctx to detect
> > +	 * time-outs for frame done event, e.g. due to backend errors.
> > +	 *
> > +	 * This must be protected with front_info->io_lock, so races between
> > +	 * interrupt handler and rest of the code are properly handled.
> > +	 */
> > +	unsigned long pflip_timeout;
> > +
> > +	bool conn_connected;
> I'm pretty sure this doesn't work. Especially the check in display_check
> confuses me, if there's ever an error then you'll never ever be able to
> display anything again, except when someone disables the display.
That was the idea to allow dummy user-space to get an error in
display_check and close, going through display_disable.
Yes, compositors will die in this case.

> If you want to signal errors with the output then this must be done
> through the new link-status property and
> drm_mode_connector_set_link_status_property. Rejecting kms updates in
> display_check with -EINVAL because the hw has a temporary issue is kinda
> not cool (because many compositors just die when this happens). I thought
> we agreed already to remove that, sorry for not spotting that in the
> previous version.
Unfortunatelly, there is little software available which will benefit
from this out of the box. I am specifically interested in embedded
use-cases, e.g. Android (DRM HWC2 - doesn't support hotplug, HWC1.4 doesn't
support link status), Weston (no device hotplug, but connectors and 
outputs).
Other software, like kmscube, modetest will not handle that as well.
So, such software will hang forever until killed.

> Some of the conn_connected checks also look a bit like they should be
> replaced by drm_dev_is_unplugged instead, but I'm not sure.
I believe you are talking about drm_simple_display_pipe_funcs?
Do you mean I have to put drm_dev_is_unplugged in display_enable,
display_disable and display_update callbacks?

> > +static int connector_detect(struct drm_connector *connector,
> > +		struct drm_modeset_acquire_ctx *ctx,
> > +		bool force)
> > +{
> > +	struct xen_drm_front_drm_pipeline *pipeline =
> > +			to_xen_drm_pipeline(connector);
> > +	struct xen_drm_front_info *front_info = pipeline->drm_info->front_info;
> > +	unsigned long flags;
> > +
> > +	/* check if there is a frame done event time-out */
> > +	spin_lock_irqsave(&front_info->io_lock, flags);
> > +	if (pipeline->pflip_timeout &&
> > +			time_after_eq(jiffies, pipeline->pflip_timeout)) {
> > +		DRM_ERROR("Frame done event timed-out\n");
> > +
> > +		pipeline->pflip_timeout = 0;
> > +		pipeline->conn_connected = false;
> > +		xen_drm_front_kms_send_pending_event(pipeline);
> > +	}
> > +	spin_unlock_irqrestore(&front_info->io_lock, flags);
> If you want to check for timeouts please use a worker, don't piggy-pack on
> top of the detect callback.
Ok, will have a dedicated work for that. The reasons why I put this into the
detect callback were:
- the periodic worker is already there, and I do nothing heavy
 in this callback
- if frame done has timed out it most probably means that
 backend has gone, so 10 sec period of detect timeout is also ok: thus 
I don't
 need to schedule a work each page flip which could be a bit costly
So, probably I will also need a periodic work (or kthread/timer) for 
frame done time-outs

> > +static int connector_mode_valid(struct drm_connector *connector,
> > +		struct drm_display_mode *mode)
> > +{
> > +	struct xen_drm_front_drm_pipeline *pipeline =
> > +			to_xen_drm_pipeline(connector);
> > +
> > +	if (mode->hdisplay != pipeline->width)
> > +		return MODE_ERROR;
> > +
> > +	if (mode->vdisplay != pipeline->height)
> > +		return MODE_ERROR;
> > +
> > +	return MODE_OK;
> > +}
> mode_valid on the connector only checks probe modes. Since that is
> hardcoded this doesn't do much, which means userspace can give you a wrong
> mode, and you fall over.
Agree, I will remove this callback completely: I have
drm_connector_funcs.fill_modes == drm_helper_probe_single_connector_modes,
so it will only pick my single hardcoded mode from 
drm_connector_helper_funcs.get_modes
callback (connector_get_modes).

> You need to use one of the other mode_valid callbacks instead,
> drm_simple_display_pipe_funcs has the one you should use.
Not sure I understand why do I need to provide a callback here?
For simple KMS the drm_simple_kms_crtc_mode_valid callback is used,
which always returns MODE_OK if there is no .mode_valid set for the pipe.
As per my understanding drm_simple_kms_crtc_mode_valid is only called for
modes, which were collected by drm_helper_probe_single_connector_modes,
so I assume each time .validate_mode is called it can only have my hardcoded
mode to validate?

> > +
> > +static int display_check(struct drm_simple_display_pipe *pipe,
> > +		struct drm_plane_state *plane_state,
> > +		struct drm_crtc_state *crtc_state)
> > +{
> > +	struct xen_drm_front_drm_pipeline *pipeline =
> > +			to_xen_drm_pipeline(pipe);
> > +
> > +	return pipeline->conn_connected ? 0 : -EINVAL;
> As mentioned, this -EINVAL here needs to go. Since you already have a
> mode_valid callback you can (should) drop this one here entirely.
Not sure how mode_valid is relevant to this code [1]: This function is 
called
in the check phase of an atomic update, specifically when the underlying
plane is checked. But, anyways: the reason for this callback and it 
returning
-EINVAL is primarialy for a dumb user-space which cannot handle hotplug 
events.
But, as you mentioned before, it will make most compositors die, so I 
will remove this

Thank you for reviewing,
Oleksandr

[1] 
https://elixir.bootlin.com/linux/v4.16-rc6/source/include/drm/drm_simple_kms_helper.h#L59