lkml.org 
[lkml]   [2019]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 2/2] drm/vkms: Add support for writeback
    Date
    Hi Rodrigo,

    On Thu, Jun 06, 2019 at 07:41:01PM -0300, Rodrigo Siqueira wrote:
    > This patch implements the necessary functions to add writeback support
    > for vkms. This feature is useful for testing compositors if you don’t
    > have hardware with writeback support.
    >
    > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
    > ---
    > drivers/gpu/drm/vkms/Makefile | 9 +-
    > drivers/gpu/drm/vkms/vkms_crtc.c | 5 +
    > drivers/gpu/drm/vkms/vkms_drv.c | 10 ++
    > drivers/gpu/drm/vkms/vkms_drv.h | 12 ++
    > drivers/gpu/drm/vkms/vkms_output.c | 6 +
    > drivers/gpu/drm/vkms/vkms_writeback.c | 165 ++++++++++++++++++++++++++
    > 6 files changed, 206 insertions(+), 1 deletion(-)
    > create mode 100644 drivers/gpu/drm/vkms/vkms_writeback.c
    >
    > diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
    > index 89f09bec7b23..90eb7acd618d 100644
    > --- a/drivers/gpu/drm/vkms/Makefile
    > +++ b/drivers/gpu/drm/vkms/Makefile
    > @@ -1,4 +1,11 @@
    > # SPDX-License-Identifier: GPL-2.0-only
    > -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
    > +vkms-y := \
    > + vkms_drv.o \
    > + vkms_plane.o \
    > + vkms_output.o \
    > + vkms_crtc.o \
    > + vkms_gem.o \
    > + vkms_crc.o \
    > + vkms_writeback.o
    >
    > obj-$(CONFIG_DRM_VKMS) += vkms.o
    > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
    > index 1bbe099b7db8..ce797e265b1b 100644
    > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
    > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
    > @@ -23,6 +23,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
    > if (!ret)
    > DRM_ERROR("vkms failure on handling vblank");
    >
    > + if (output->writeback_status == WB_START) {
    > + drm_writeback_signal_completion(&output->wb_connector, 0);
    > + output->writeback_status = WB_STOP;
    > + }
    > +
    > if (state && output->crc_enabled) {
    > u64 frame = drm_crtc_accurate_vblank_count(crtc);
    >
    > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
    > index 92296bd8f623..d5917d5a45e3 100644
    > --- a/drivers/gpu/drm/vkms/vkms_drv.c
    > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
    > @@ -29,6 +29,10 @@ bool enable_cursor;
    > module_param_named(enable_cursor, enable_cursor, bool, 0444);
    > MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
    >
    > +int enable_writeback;
    > +module_param_named(enable_writeback, enable_writeback, int, 0444);
    > +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");
    > +
    > static const struct file_operations vkms_driver_fops = {
    > .owner = THIS_MODULE,
    > .open = drm_open,
    > @@ -123,6 +127,12 @@ static int __init vkms_init(void)
    > goto out_fini;
    > }
    >
    > + vkms_device->output.writeback_status = WB_DISABLED;
    > + if (enable_writeback) {
    > + vkms_device->output.writeback_status = WB_STOP;
    > + DRM_INFO("Writeback connector enabled");
    > + }
    > +
    > ret = vkms_modeset_init(vkms_device);
    > if (ret)
    > goto out_fini;
    > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
    > index e81073dea154..ca1f9ee63ec8 100644
    > --- a/drivers/gpu/drm/vkms/vkms_drv.h
    > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
    > @@ -7,6 +7,7 @@
    > #include <drm/drm.h>
    > #include <drm/drm_gem.h>
    > #include <drm/drm_encoder.h>
    > +#include <drm/drm_writeback.h>
    > #include <linux/hrtimer.h>
    >
    > #define XRES_MIN 20
    > @@ -60,14 +61,22 @@ struct vkms_crtc_state {
    > u64 frame_end;
    > };
    >
    > +enum wb_status {
    > + WB_DISABLED,
    > + WB_START,
    > + WB_STOP,
    > +};
    > +
    > struct vkms_output {
    > struct drm_crtc crtc;
    > struct drm_encoder encoder;
    > struct drm_connector connector;
    > + struct drm_writeback_connector wb_connector;
    > struct hrtimer vblank_hrtimer;
    > ktime_t period_ns;
    > struct drm_pending_vblank_event *event;
    > bool crc_enabled;
    > + enum wb_status writeback_status;
    > /* ordered wq for crc_work */
    > struct workqueue_struct *crc_workq;
    > /* protects concurrent access to crc_data */
    > @@ -141,4 +150,7 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
    > size_t *values_cnt);
    > void vkms_crc_work_handle(struct work_struct *work);
    >
    > +/* Writeback */
    > +int enable_writeback_connector(struct vkms_device *vkmsdev);
    > +
    > #endif /* _VKMS_DRV_H_ */
    > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
    > index 1442b447c707..1fc1d4e9585c 100644
    > --- a/drivers/gpu/drm/vkms/vkms_output.c
    > +++ b/drivers/gpu/drm/vkms/vkms_output.c
    > @@ -91,6 +91,12 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
    > goto err_attach;
    > }
    >
    > + if (vkmsdev->output.writeback_status != WB_DISABLED) {
    > + ret = enable_writeback_connector(vkmsdev);
    > + if (ret)
    > + DRM_ERROR("Failed to init writeback connector\n");
    > + }
    > +
    > drm_mode_config_reset(dev);
    >
    > return 0;
    > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
    > new file mode 100644
    > index 000000000000..f7b962ae5646
    > --- /dev/null
    > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
    > @@ -0,0 +1,165 @@
    > +// SPDX-License-Identifier: GPL-2.0+
    > +
    > +#include "vkms_drv.h"
    > +#include <drm/drm_writeback.h>
    > +#include <drm/drm_probe_helper.h>
    > +#include <drm/drm_atomic_helper.h>
    > +#include <drm/drm_gem_framebuffer_helper.h>
    > +
    > +static const struct drm_connector_funcs vkms_wb_connector_funcs = {
    > + .fill_modes = drm_helper_probe_single_connector_modes,
    > + .destroy = drm_connector_cleanup,
    > + .reset = drm_atomic_helper_connector_reset,
    > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
    > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
    > +};
    > +
    > +static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,
    > + struct drm_crtc_state *crtc_state,
    > + struct drm_connector_state *conn_state)
    > +{
    > + struct drm_framebuffer *fb;
    > + const struct drm_display_mode *mode = &crtc_state->mode;
    > +
    > + if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
    > + return 0;
    > +
    > + fb = conn_state->writeback_job->fb;
    > + if (fb->width != mode->hdisplay || fb->height != mode->vdisplay) {
    > + DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
    > + fb->width, fb->height);
    > + return -EINVAL;
    > + }
    > +
    > + if (fb->format->format != DRM_FORMAT_XRGB8888) {
    > + struct drm_format_name_buf format_name;
    > +
    > + DRM_DEBUG_KMS("Invalid pixel format %s\n",
    > + drm_get_format_name(fb->format->format,
    > + &format_name));
    > + return -EINVAL;
    > + }
    > +
    > + return 0;
    > +}
    > +
    > +static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {
    > + .atomic_check = vkms_wb_encoder_atomic_check,
    > +};
    > +
    > +static int vkms_wb_connector_get_modes(struct drm_connector *connector)
    > +{
    > + struct drm_device *dev = connector->dev;
    > +
    > + return drm_add_modes_noedid(connector, dev->mode_config.max_width,
    > + dev->mode_config.max_height);
    > +}
    > +
    > +static int vkms_wb_prepare_job(struct drm_writeback_connector *wb_connector,
    > + struct drm_writeback_job *job)
    > +{
    > + struct vkms_gem_object *vkms_obj;
    > + struct drm_gem_object *gem_obj;
    > + int ret;
    > +
    > + if (!job->fb)
    > + return 0;
    > +
    > + gem_obj = drm_gem_fb_get_obj(job->fb, 0);
    > + ret = vkms_gem_vmap(gem_obj);
    > + if (ret) {
    > + DRM_ERROR("vmap failed: %d\n", ret);
    > + return ret;
    > + }
    > +
    > + vkms_obj = drm_gem_to_vkms_gem(gem_obj);
    > + job->priv = vkms_obj->vaddr;
    > +
    > + return 0;
    > +}
    > +
    > +static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
    > + struct drm_writeback_job *job)
    > +{
    > + struct drm_gem_object *gem_obj;
    > +
    > + if (!job->fb)
    > + return;
    > +
    > + gem_obj = drm_gem_fb_get_obj(job->fb, 0);
    > + vkms_gem_vunmap(gem_obj);
    > +}
    > +
    > +static void vkms_wb_atomic_commit(struct drm_connector *conn,
    > + struct drm_connector_state *state)
    > +{
    > + struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
    > + struct vkms_output *output = &vkmsdev->output;
    > + struct drm_writeback_connector *wb_conn = &output->wb_connector;
    > + struct drm_connector_state *conn_state = wb_conn->base.state;
    > + void *priv_data = conn_state->writeback_job->priv;
    > + struct vkms_crc_data *primary_data = NULL;
    > + struct drm_framebuffer *fb = NULL;
    > + struct vkms_gem_object *vkms_obj;
    > + struct drm_gem_object *gem_obj;
    > + struct drm_plane *plane;
    > +
    > + if (!conn_state)
    > + return;
    > +
    > + if (!conn_state->writeback_job || !conn_state->writeback_job->fb) {
    > + output->writeback_status = WB_STOP;
    > + DRM_DEBUG_DRIVER("Disable writeback\n");
    > + return;
    > + }
    > +
    > + drm_for_each_plane(plane, &vkmsdev->drm) {
    > + struct vkms_plane_state *vplane_state;
    > + struct vkms_crc_data *plane_data;
    > +
    > + vplane_state = to_vkms_plane_state(plane->state);
    > + plane_data = vplane_state->crc_data;
    > +
    > + if (drm_framebuffer_read_refcount(&plane_data->fb) == 0)
    > + continue;

    It's not really specific to this patch, but this looks kinda
    weird to me. You've got a copy of the fb struct in plane_data?

    It seems like that would break a bunch of refcounting if nothing else.

    Can't you use plane->state->fb directly, and just check if it's NULL?

    > +
    > + if (plane->type == DRM_PLANE_TYPE_PRIMARY)
    > + primary_data = plane_data;
    > + }
    > +
    > + if (!primary_data)
    > + return;
    > +
    > + fb = &primary_data->fb;
    > + gem_obj = drm_gem_fb_get_obj(fb, 0);
    > + vkms_obj = drm_gem_to_vkms_gem(gem_obj);
    > +
    > + if (!vkms_obj->vaddr || !priv_data)
    > + return;
    > +
    > + memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
    > + drm_writeback_queue_job(wb_conn, state);
    > + output->writeback_status = WB_START;

    You probably don't need the deferred completion signaling. I'd expect
    this to work OK (and it matches the API semantics):

    drm_writeback_queue_job(wb_conn, state);
    memcpy(priv_data, vkms_obj->vaddr, vkms_obj->gem.size);
    drm_writeback_signal_completion(wb_conn, 0);


    Cheers,
    -Brian

    > +}
    > +
    > +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
    > + .get_modes = vkms_wb_connector_get_modes,
    > + .prepare_writeback_job = vkms_wb_prepare_job,
    > + .cleanup_writeback_job = vkms_wb_cleanup_job,
    > + .atomic_commit = vkms_wb_atomic_commit,
    > +};
    > +
    > +int enable_writeback_connector(struct vkms_device *vkmsdev)
    > +{
    > + struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
    > +
    > + vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
    > + drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
    > +
    > + return drm_writeback_connector_init(&vkmsdev->drm, wb,
    > + &vkms_wb_connector_funcs,
    > + &vkms_wb_encoder_helper_funcs,
    > + vkms_formats,
    > + ARRAY_SIZE(vkms_formats));
    > +}
    > +
    > --
    > 2.21.0


    \
     
     \ /
      Last update: 2019-06-07 16:18    [W:3.067 / U:0.100 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site