lkml.org 
[lkml]   [2018]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH RESEND v2 1/2] drm/xen-front: Add support for Xen PV display frontend
From
Date
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

\
 
 \ /
  Last update: 2018-03-16 11:53    [W:0.073 / U:0.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site