Re: [PATCH v2 14/17] drm/msm/dpu: add the writeback connector layer

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 4/20/2022 12:52 AM, Dmitry Baryshkov wrote:
> On 20/04/2022 04:46, Abhinav Kumar wrote:
> > Introduce the dpu_writeback module which serves as the
> > interface between dpu operations and the drm_writeback.
> >
> > This module manages the connector related operations for
> > dpu writeback.
> >
> > changes in v2:
> >     - start using drm_writeback_connector_init_with_encoder()
> >     - drop unnecessary arguments from dpu_writeback_init()
> >     - rebase on msm-next tip and remove usage of priv->connectors
> >
> > Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/Makefile                  |  1 +
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 68 
> > +++++++++++++++++++++++++++
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h | 25 ++++++++++
> >   3 files changed, 94 insertions(+)
> >   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> >   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
> >
> > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> > index 0387f22..66395ee 100644
> > --- a/drivers/gpu/drm/msm/Makefile
> > +++ b/drivers/gpu/drm/msm/Makefile
> > @@ -80,6 +80,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
> >       disp/dpu1/dpu_plane.o \
> >       disp/dpu1/dpu_rm.o \
> >       disp/dpu1/dpu_vbif.o \
> > +    disp/dpu1/dpu_writeback.o
> >   msm-$(CONFIG_DRM_MSM_MDSS) += \
> >       msm_mdss.o \
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > new file mode 100644
> > index 0000000..526d884
> > --- /dev/null
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c
> > @@ -0,0 +1,68 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights 
> > reserved.
> > + */
> > +
> > +#include "dpu_writeback.h"
> > +
> > +static int dpu_wb_conn_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 const struct drm_connector_funcs dpu_wb_conn_funcs = {
> > +    .reset = drm_atomic_helper_connector_reset,
> > +    .fill_modes = drm_helper_probe_single_connector_modes,
> > +    .destroy = drm_connector_cleanup,
> > +    .atomic_duplicate_state = 
> > drm_atomic_helper_connector_duplicate_state,
> > +    .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +static int dpu_wb_conn_prepare_job(struct drm_writeback_connector 
> > *connector,
> > +        struct drm_writeback_job *job)
> > +{
> > +    if (!job->fb)
> > +        return 0;
> > +
> > +    dpu_encoder_prepare_wb_job(connector->encoder, job);
> > +
> > +    return 0;
> > +}
> > +
> > +static void dpu_wb_conn_cleanup_job(struct drm_writeback_connector 
> > *connector,
> > +        struct drm_writeback_job *job)
> > +{
> > +    if (!job->fb)
> > +        return;
> > +
> > +    dpu_encoder_cleanup_wb_job(connector->encoder, job);
> > +}
> > +
> > +static const struct drm_connector_helper_funcs 
> > dpu_wb_conn_helper_funcs = {
> > +    .get_modes = dpu_wb_conn_get_modes,
> > +    .prepare_writeback_job = dpu_wb_conn_prepare_job,
> > +    .cleanup_writeback_job = dpu_wb_conn_cleanup_job,
> > +};
> > +
> > +int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
> > +        const u32 *format_list, u32 num_formats)
> > +{
> > +    struct dpu_wb_connector *dpu_wb_conn;
> > +    int rc = 0;
> > +
> > +    dpu_wb_conn = devm_kzalloc(dev->dev, sizeof(*dpu_wb_conn), 
> > GFP_KERNEL);
> > +
> > +    drm_connector_helper_add(&dpu_wb_conn->base.base, 
> > &dpu_wb_conn_helper_funcs);
> > +
> > +    /* DPU initializes the encoder and sets it up completely for 
> > writeback
> > +     * cases and hence should use the new API 
> > drm_writeback_connector_init_with_encoder
> > +     * to initialize the writeback connector
> > +     */
> > +    rc = drm_writeback_connector_init_with_encoder(dev, 
> > &dpu_wb_conn->base, enc,
> > +            &dpu_wb_conn_funcs, format_list, num_formats);
> > +
> > +    return rc;
> > +}
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
> > new file mode 100644
> > index 0000000..05aff05
> > --- /dev/null
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights 
> > reserved.
> > + */
> > +
> > +#ifndef _DPU_WRITEBACK_H
> > +#define _DPU_WRITEBACK_H
> > +
> > +#include <drm/drm_crtc.h>
> > +#include <drm/drm_file.h>
> > +#include <drm/drm_probe_helper.h>
> > +#include <drm/drm_writeback.h>
> > +
> > +#include "msm_drv.h"
> > +#include "dpu_kms.h"
> > +#include "dpu_encoder_phys.h"
> > +
> > +struct dpu_wb_connector {
> > +    struct drm_writeback_connector base;
> > +};
>
> Do you plan to add more fields to this struct? If not, we can probably 
> drop it and use struct drm_writeback_connector directly.

Glad you asked about it. I was expecting this question because it looks 
like a very "light" struct.

Yes, we do plan to expand this as we will keep adding writeback features 
sequentially now to make it ready for this to be absorbed downstream 
completely.

> > +
> > +int dpu_writeback_init(struct drm_device *dev, struct drm_encoder *enc,
> > +        const u32 *format_list, u32 num_formats);
> > +
> > +#endif /*_DPU_WRITEBACK_H */