Re: [Freedreno] [PATCH v4 03/20] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

Hi Laurent

On 4/24/2022 2:12 PM, Abhinav Kumar wrote:
> Hi Laurent
>
> On 4/24/2022 12:59 PM, Laurent Pinchart wrote:
> > Hi Abhinav,
> >
> > On Sun, Apr 24, 2022 at 11:23:20AM -0700, Abhinav Kumar wrote:
> > > On 4/24/2022 11:12 AM, Abhinav Kumar wrote:
> > > > On 4/24/2022 7:50 AM, Laurent Pinchart wrote:
> > > > > On Fri, Apr 22, 2022 at 04:06:38PM -0700, Abhinav Kumar wrote:
> > > > > > For some vendor driver implementations, display hardware can
> > > > > > be shared between the encoder used for writeback and the physical
> > > > > > display.
> > > > > >
> > > > > > In addition resources such as clocks and interrupts can
> > > > > > also be shared between writeback and the real encoder.
> > > > > >
> > > > > > To accommodate such vendor drivers and hardware, allow
> > > > > > real encoder to be passed for drm_writeback_connector.
> > > > > >
> > > > > > For existing clients, drm_writeback_connector_init() will use
> > > > > > an internal_encoder under the hood and hence no changes will
> > > > > > be needed.
> > > > > >
> > > > > > changes in v7:
> > > > > >      - move this change before the vc4 change in the series
> > > > > >        to minimize the changes to vendor drivers in drm core
> > > > > >        changes
> > > > >
> > > > > Why is this needed ? The drm_writeback_connector functions don't need
> > > > > the drm_encoder after drm_writeback_connector_init() (or
> > > > > drm_writeback_connector_init_with_encoder()) returns.
> > > >
> > > > Sorry I didnt follow this comment. This change log is incorrect, so
> > > > after changing the previous change in the series and modifying this, no
> > > > further changes are needed to vc4, so I decided to drop the next 
> > > > change.
> > > > So this change log is incorrect. I can remove this.
> > > >
> > > > Is that what you meant?
> > >
> > > So till the previous change, the only user of
> > > drm_writeback_connector_init_with_encoder() was
> > > drm_writeback_connector_init() which was still passing its own
> > > internal_encoder.
> > >
> > > Only if the wb_connector->encoder is changed to a pointer, other vendor
> > > drivers can pass their own encoder to
> > > drm_writeback_connector_init_with_encoder().
> > >
> > > Hence you are right that drm_writeback_connector functions do not need
> > > drm_encoder after init() returns, but till this change is done, other
> > > vendor drivers cannot directly call
> > > drm_writeback_connector_init_with_encoder() because the encoder will not
> > > be valid till then.
> >
> > Users of drm_writeback_connector_init_with_encoder() handle the encoder
> > themselves, they can simply ignore drm_writeback_connector.encoder. The
> > documentation of the encoder field needs to be updated though (I'd do so
> > in the previous patch), to clearly mention that the field is valid only
> > when using drm_writeback_connector_init(), not when calling
> > drm_writeback_connector_init_with_encoder().
>
> So you are suggesting to drop this change and just update the doc in 
> patch 2?
>
> If so, I dont think it would be correct to assume that even though 
> drm_writeback_connector_init_with_encoder() is used 
> drm_writeback_connector.encoder should not be used.
>
> The reason is that, lets consider, prepare_wb_job() and 
> cleanup_writeback_job().
>
> These pass only the connector to the vendor hooks.
>
> 1126      *
> 1127      * This callback is used by the atomic modeset helpers.
> 1128      */
> 1129     int (*prepare_writeback_job)(struct drm_writeback_connector 
> *connector,
> 1130                      struct drm_writeback_job *job);
>
> Now, if you assume connector->encoder is not valid, this adds more 
> confusion because now we are putting an extra condition that 
> connector->encoder is not valid anymore.
>
> Thats why I think we should leave it as-it-is to not allow room for such 
> discrepancies.

I was able to abandon this change and still make writeback work with 
just trivial changes to MSM driver.

So we are okay to drop this change as you requested and let the vendor 
driver manage the encoder completely.

If you do think that this change is worth re-visiting sometime, let me 
know I can absorb it as a separate change.

Thanks

Abhinav
> > > Hope this clarifies it.
> > >
> > > > > > Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > > > ---
> > > > > >    drivers/gpu/drm/drm_writeback.c | 18 ++++++++++++------
> > > > > >    drivers/gpu/drm/vc4/vc4_txp.c   |  4 ++--
> > > > > >    include/drm/drm_writeback.h     | 22 ++++++++++++++++++++--
> > > > > >    3 files changed, 34 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > > > > b/drivers/gpu/drm/drm_writeback.c
> > > > > > index 92658ad..0538674 100644
> > > > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > > > @@ -180,21 +180,21 @@ int drm_writeback_connector_init(struct
> > > > > > drm_device *dev,
> > > > > >    {
> > > > > >        int ret = 0;
> > > > > > -    drm_encoder_helper_add(&wb_connector->encoder, 
> > > > > > enc_helper_funcs);
> > > > > > +    drm_encoder_helper_add(&wb_connector->internal_encoder, 
> > > > > > enc_helper_funcs);
> > > > > > -    wb_connector->encoder.possible_crtcs = possible_crtcs;
> > > > > > +    wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
> > > > > > -    ret = drm_encoder_init(dev, &wb_connector->encoder,
> > > > > > +    ret = drm_encoder_init(dev, &wb_connector->internal_encoder,
> > > > > >                       &drm_writeback_encoder_funcs,
> > > > > >                       DRM_MODE_ENCODER_VIRTUAL, NULL);
> > > > > >        if (ret)
> > > > > >            return ret;
> > > > > > -    ret = drm_writeback_connector_init_with_encoder(dev, 
> > > > > > wb_connector, &wb_connector->encoder,
> > > > > > -            con_funcs, formats, n_formats);
> > > > > > +    ret = drm_writeback_connector_init_with_encoder(dev, 
> > > > > > wb_connector,
> > > > > > +            &wb_connector->internal_encoder, con_funcs, formats, 
> > > > > > n_formats);
> > > > > >        if (ret)
> > > > > > -        drm_encoder_cleanup(&wb_connector->encoder);
> > > > > > +        drm_encoder_cleanup(&wb_connector->internal_encoder);
> > > > > >        return ret;
> > > > > >    }
> > > > > > @@ -239,6 +239,12 @@ int 
> > > > > > drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> > > > > >        struct drm_mode_config *config = &dev->mode_config;
> > > > > >        int ret = create_writeback_properties(dev);
> > > > > > +    /*
> > > > > > +     * Assign the encoder passed to this API to the 
> > > > > > wb_connector's encoder.
> > > > > > +     * For drm_writeback_connector_init(), this shall be the 
> > > > > > internal_encoder
> > > > > > +     */
> > > > > > +    wb_connector->encoder = enc;
> > > > > > +
> > > > > >        if (ret != 0)
> > > > > >            return ret;
> > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c 
> > > > > > b/drivers/gpu/drm/vc4/vc4_txp.c
> > > > > > index 3447eb6..7e063a9 100644
> > > > > > --- a/drivers/gpu/drm/vc4/vc4_txp.c
> > > > > > +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> > > > > > @@ -159,7 +159,7 @@ struct vc4_txp {
> > > > > >    static inline struct vc4_txp *encoder_to_vc4_txp(struct 
> > > > > > drm_encoder *encoder)
> > > > > >    {
> > > > > > -    return container_of(encoder, struct vc4_txp, connector.encoder);
> > > > > > +    return container_of(encoder, struct vc4_txp, 
> > > > > > connector.internal_encoder);
> > > > > >    }
> > > > > >    static inline struct vc4_txp *connector_to_vc4_txp(struct  
> > > > > > drm_connector *conn)
> > > > > > @@ -507,7 +507,7 @@ static int vc4_txp_bind(struct device *dev, 
> > > > > > struct device *master, void *data)
> > > > > >        if (ret)
> > > > > >            return ret;
> > > > > > -    encoder = &txp->connector.encoder;
> > > > > > +    encoder = txp->connector.encoder;
> > > > > >        encoder->possible_crtcs = drm_crtc_mask(crtc);
> > > > > >        ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
> > > > > > diff --git a/include/drm/drm_writeback.h 
> > > > > > b/include/drm/drm_writeback.h
> > > > > > index bb306fa..3fbae9d 100644
> > > > > > --- a/include/drm/drm_writeback.h
> > > > > > +++ b/include/drm/drm_writeback.h
> > > > > > @@ -25,13 +25,31 @@ struct drm_writeback_connector {
> > > > > >        struct drm_connector base;
> > > > > >        /**
> > > > > > -     * @encoder: Internal encoder used by the connector to fulfill
> > > > > > +     * @encoder: handle to drm_encoder used by the connector to 
> > > > > > fulfill
> > > > > >         * the DRM framework requirements. The users of the
> > > > > >         * @drm_writeback_connector control the behaviour of the 
> > > > > > @encoder
> > > > > >         * by passing the @enc_funcs parameter to 
> > > > > > drm_writeback_connector_init()
> > > > > >         * function.
> > > > > > +     *
> > > > > > +     * For some vendor drivers, the hardware resources are shared 
> > > > > > between
> > > > > > +     * writeback encoder and rest of the display pipeline.
> > > > > > +     * To accommodate such cases, encoder is a handle to the real 
> > > > > > encoder
> > > > > > +     * hardware.
> > > > > > +     *
> > > > > > +     * For current existing writeback users, this shall continue 
> > > > > > to be the
> > > > > > +     * embedded encoder for the writeback connector.
> > > > > > +     */
> > > > > > +    struct drm_encoder *encoder;
> > > > > > +
> > > > > > +    /**
> > > > > > +     * @internal_encoder: internal encoder used by writeback when
> > > > > > +     * drm_writeback_connector_init() is used.
> > > > > > +     * @encoder will be assigned to this for those cases
> > > > > > +     *
> > > > > > +     * This will be unused when 
> > > > > > drm_writeback_connector_init_with_encoder()
> > > > > > +     * is used.
> > > > > >         */
> > > > > > -    struct drm_encoder encoder;
> > > > > > +    struct drm_encoder internal_encoder;
> > > > > >        /**
> > > > > >         * @pixel_formats_blob_ptr: