Date:   Fri, 16 Mar 2018 12:52:09 +0200
From:   Oleksandr Andrushchenko <andr2000@...il.com>
To:     xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, airlied@...ux.ie,
        daniel.vetter@...el.com, seanpaul@...omium.org,
        gustavo@...ovan.org, jgross@...e.com, boris.ostrovsky@...cle.com,
        konrad.wilk@...cle.com,
        Oleksandr Andrushchenko <oleksandr_andrushchenko@...m.com>
Subject: Re: [PATCH RESEND v2 1/2] drm/xen-front: Add support for Xen PV
 display frontend

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

Powered by blists - more mailing lists