Re: [PATCH RESEND v2 1/2] drm/xen-front: Add support for Xen PV display frontend

[Oleksandr Andrushchenko]

Hi, Daniel!
Sorry, if I strip the patch too much below.

On 03/16/2018 10:23 AM, Daniel Vetter wrote:
> S-o-b line went missing here :-)
will restore it back ;)
> I've read through it, 2 actual review comments (around hot-unplug and
> around the error recovery for failed flips), a few bikesheds, but looks
> all reasonable to me. And much easier to read as one big patch (it's just
> 3k).
>
> One more thing I'd do as a follow-up (don't rewrite everything, this is
> close to merge, better to get it in first): You have a lot of indirections
> and function calls across sources files. That's kinda ok if you have a
> huge driver with 100+k lines of code where you have to split things up.
> But for a small driver like yours here it's a bit overkill.
will review and try to rework after the driver is in
> Personally I'd merge at least the xen backend stuff into the corresponding
> kms code, but that's up to you.
I prefer to have it in smaller chunks and all related code at
one place, so it is easier to maintain. That is why I didn't
plumb frontend <-> backend code right into the KMS code.
> And as mentioned, if you decide to do
> that, a follow-up patch (once this has merged) is perfectly fine.
Ok, after the merge
> -Daniel
>
> > +int xen_drm_front_dbuf_create_from_sgt(struct xen_drm_front_info *front_info,
> > +		uint64_t dbuf_cookie, uint32_t width, uint32_t height,
> > +		uint32_t bpp, uint64_t size, struct sg_table *sgt)
> > +{
> > +	return be_dbuf_create_int(front_info, dbuf_cookie, width, height,
> > +			bpp, size, NULL, sgt);
> > +}
> > +
> > +int xen_drm_front_dbuf_create_from_pages(struct xen_drm_front_info *front_info,
> > +		uint64_t dbuf_cookie, uint32_t width, uint32_t height,
> > +		uint32_t bpp, uint64_t size, struct page **pages)
> > +{
> > +	return be_dbuf_create_int(front_info, dbuf_cookie, width, height,
> > +			bpp, size, pages, NULL);
> > +}
> The above two wrappers seem a bit much, just to set sgt = NULL or pages =
> NULL in one of them. I'd drop them, but that's a bikeshed so feel free to
> ignore.
I had that the way you say in some of the previous implementations,
but finally decided to have these dummy wrappers: seems
to be cleaner this way
> > +static void displback_disconnect(struct xen_drm_front_info *front_info)
> > +{
> > +	bool removed = true;
> > +
> > +	if (front_info->drm_pdev) {
> > +		if (xen_drm_front_drv_is_used(front_info->drm_pdev)) {
> > +			DRM_WARN("DRM driver still in use, deferring removal\n");
> > +			removed = false;
> > +		} else
> > +			xen_drv_remove_internal(front_info);
> Ok this logic here is fishy, since you're open-coding the drm unplug
> infrastructure, but slightly differently and slightyl racy. If you have a
> driver where your underlying "hw" (well it's virtual here, but same idea)
> can disappear any time while userspace is still using the drm driver, you
> need to use the drm_dev_unplug() function and related code.
> drm_dev_unplug() works like drm_dev_unregister, except for the hotplug
> case.
>
> Then you also have to guard all the driver entry points where you do
> access the backchannel using drm_dev_is_unplugged() (I've seen a few of
> those already). Then you can rip out all the logic here and the xen_drm_front_drv_is_used() helper.
Will rework it with drm_dev_unplug, thank you
> I thought there's some patches from Noralf in-flight that improved the
> docs on this, I need to check
>
> > +#define XEN_DRM_NUM_VIDEO_MODES		1
> > +#define XEN_DRM_CRTC_VREFRESH_HZ	60
> > +
> > +static int connector_get_modes(struct drm_connector *connector)
> > +{
> > +	struct xen_drm_front_drm_pipeline *pipeline =
> > +			to_xen_drm_pipeline(connector);
> > +	struct drm_display_mode *mode;
> > +	struct videomode videomode;
> > +	int width, height;
> > +
> > +	mode = drm_mode_create(connector->dev);
> > +	if (!mode)
> > +		return 0;
> > +
> > +	memset(&videomode, 0, sizeof(videomode));
> > +	videomode.hactive = pipeline->width;
> > +	videomode.vactive = pipeline->height;
> > +	width = videomode.hactive + videomode.hfront_porch +
> > +			videomode.hback_porch + videomode.hsync_len;
> > +	height = videomode.vactive + videomode.vfront_porch +
> > +			videomode.vback_porch + videomode.vsync_len;
> > +	videomode.pixelclock = width * height * XEN_DRM_CRTC_VREFRESH_HZ;
> > +	mode->type = DRM_MODE_TYPE_PREFERRED | DRM_MODE_TYPE_DRIVER;
> > +
> > +	drm_display_mode_from_videomode(&videomode, mode);
> > +	drm_mode_probed_add(connector, mode);
> > +	return XEN_DRM_NUM_VIDEO_MODES;
> Bikeshed: just hardcode this to 1, the #define is imo more confusing.
ok, will remove #define
> > +
> > +	}
> > +	/*
> > +	 * Send page flip request to the backend *after* we have event cached
> > +	 * above, so on page flip done event from the backend we can
> > +	 * deliver it and there is no race condition between this code and
> > +	 * event from the backend.
> > +	 * If this is not a page flip, e.g. no flip done event from the backend
> > +	 * is expected, then send now.
> > +	 */
> > +	if (!display_send_page_flip(pipe, old_plane_state))
> > +		xen_drm_front_kms_send_pending_event(pipeline);
> The control flow here is a bit confusing. I'd put the call to send out the
> event right away in case of a failure to communicate with the backend into
> display_send_page_flip() itself. Then drop the bool return value and make
> it void, and also push the comment explaining what you do in case of
> errors into that function.
The reason for having bool for page flip here is that we
need to send pending event for display enable/disable, for example.
So, I decided to make it this way:
1. page flip handled - handles pending event internally
(defers sending until frame done event from the backend)
2. page flip failed - handles externally in case of any
page flip related error, e.g. "not handled" cases, either
due to backend communication error or whatever else
3. all other cases, but page flip
> That way the error handling and recovery is all neatly tied together in
> one place instead of spread around.
Well, I tried to keep it all at one place, but as we decided
to implement connector hotplug for error delivery it
became split. Also, I handle frame done event time-outs there.
>
Thank you very much for review and comments,
Oleksandr