Re: [PATCH v2 0/2] drm/nouveau: remove drm_encoder_slave interface

[Danilo Krummrich <dakr@kernel.org>]

On Mon, Dec 16, 2024 at 02:58:59PM +0200, Dmitry Baryshkov wrote:
> On Mon, Dec 16, 2024 at 01:41:56PM +0100, Danilo Krummrich wrote:
> > On Mon, Dec 16, 2024 at 02:16:51PM +0200, Laurent Pinchart wrote:
> > > On Mon, Dec 16, 2024 at 02:11:41PM +0200, Dmitry Baryshkov wrote:
> > > > On Mon, Dec 16, 2024 at 12:45:15PM +0100, Danilo Krummrich wrote:
> > > > > On Sun, Dec 15, 2024 at 12:19:22PM +0200, Dmitry Baryshkov wrote:
> > > > > > The nouveau driver is the only user of the drm_encoder_slave interface.
> > > > > > Demote it from KMS helpers module to the nouveau driver itself, moving
> > > > > > corresponding I2C encoders to be handled by nouveau driver too.
> > > > > 
> > > > > I understand nouveau is the only driver using this interface (and the
> > > > > corresponding i2c encoders).
> > > > > 
> > > > > However, I'm not quite seeing the advantage of folding the interface (including
> > > > > the two i2c drivers) into nouveau. I don't think this legacy interface does harm
> > > > > the subsystem in any way / does prevent the subsystem from moving forward.
> > > > > 
> > > > > Can't we just keep it as it is?
> > > > 
> > > > Well, drm_encoder_slave is a part of the DRM KMS helpers module, so it
> > > > take (a little bit) of space on every system. The nouveau situation
> > > > isn't unique, other drivers (i915, ast) also incorporate the code for
> > > > I2C backends. For the further discussion see the thread starting from
> > > > Laurent's email ([1]).
> > > > 
> > > > [1] https://lore.kernel.org/all/20241117205426.GE12409@pendragon.ideasonboard.com/
> > 
> > The drm_encoder_slave code it's rather small, but I guess this can be used as
> > argument for both, keeping it where it is and moving it.
> > 
> > If you want to move it to nouveau, I'm not going to object. But please fold the
> > helper code, such that we aren't left with unused functions and unnecessary
> > function pointer indirections through struct drm_encoder_slave_funcs.
> 
> This is more or less what I've done. Or would you prefer to keep the
> wrapping functions that just execute the callback? I can change the
> patchset accordingly.

No, I think it's good indeed -- st a first glance it looked like there's more to
get rid of.

There are just a few more nits, I'll go ahead and add comments in the
corresponding patches.

> 
> > 
> > > 
> > > It's also a question of whether maintenance of this code based used by
> > > the nouveau driver only should be the responsibility of the drm-misc
> > > community or the nouveau driver maintainers.
> > 
> > Good question. It's common infrastructure; do we expect / require the last user
> > of such infrastructure to take ownership?
> 
> Unfortunately it's more like 'the only one' :-( In other words, if we

I can't see a major difference between "last one" and "only one" in this
context.

> were expecting other users, there would not be such a move. But
> hopefully all new drivers will use bridges infrastructure.

Agreed, but I don't think it answers my question.

> 
> > 
> > > 
> > > > > > Ideally those two drivers should be converted to the drm_bridge
> > > > > > interface, but it's unclear if it's worth spending time on that.
> > > > > 
> > > > > Probably not.
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > - Renamed symbols in defconfig (Laurent)
> > > > > > - Added missing Kbuild file (Laurent, LKP)
> > > > > > - Renamed guarding defines in include files.
> > > > > > - Dropped mentions of two removed functions.
> > > > > > - Link to v1: https://lore.kernel.org/r/20241214-nouveau-encoder-slave-v1-0-beda767472e3@linaro.org
> > > > > > 
> > > > > > ---
> > > > > > Dmitry Baryshkov (2):
> > > > > >       drm/nouveau: incorporate I2C TV encoder drivers
> > > > > >       drm/nouveau: vendor in drm_encoder_slave API
> > > > > > 
> > > > > >  arch/arm/configs/multi_v7_defconfig                |   4 +-
> > > > > >  arch/parisc/configs/generic-32bit_defconfig        |   4 +-
> > > > > >  arch/parisc/configs/generic-64bit_defconfig        |   4 +-
> > > > > >  drivers/gpu/drm/Makefile                           |   1 -
> > > > > >  drivers/gpu/drm/i2c/Kconfig                        |  18 ----
> > > > > >  drivers/gpu/drm/i2c/Makefile                       |   6 --
> > > > > >  drivers/gpu/drm/nouveau/Kconfig                    |  20 ++++
> > > > > >  drivers/gpu/drm/nouveau/dispnv04/Kbuild            |   3 +
> > > > > >  drivers/gpu/drm/nouveau/dispnv04/dfp.c             |  12 +--
> > > > > >  drivers/gpu/drm/nouveau/dispnv04/i2c/Kbuild        |   5 +
> > > > > >  .../drm/{ => nouveau/dispnv04}/i2c/ch7006_drv.c    |  30 +++---
> > > > > >  .../drm/{ => nouveau/dispnv04}/i2c/ch7006_mode.c   |   8 +-
> > > > > >  .../drm/{ => nouveau/dispnv04}/i2c/ch7006_priv.h   |  11 ++-
> > > > > >  .../drm/{ => nouveau/dispnv04}/i2c/sil164_drv.c    |  33 ++++---
> > > > > >  .../dispnv04/nouveau_i2c_encoder.c}                |  85 +++++-----------
> > > > > >  drivers/gpu/drm/nouveau/dispnv04/tvnv04.c          |  20 ++--
> > > > > >  drivers/gpu/drm/nouveau/dispnv04/tvnv17.c          |   4 +-
> > > > > >  .../gpu/drm/nouveau/include}/i2c/ch7006.h          |   4 +-
> > > > > >  .../gpu/drm/nouveau/include/i2c/encoder_i2c.h      | 109 ++++++++-------------
> > > > > >  .../gpu/drm/nouveau/include}/i2c/sil164.h          |   4 +-
> > > > > >  drivers/gpu/drm/nouveau/nouveau_connector.c        |   6 +-
> > > > > >  drivers/gpu/drm/nouveau/nouveau_encoder.h          |  13 +--
> > > > > >  22 files changed, 172 insertions(+), 232 deletions(-)
> > > > > > ---
> > > > > > base-commit: 4176cf5c5651c33769de83bb61b0287f4ec7719f
> > > > > > change-id: 20241214-nouveau-encoder-slave-a6dd422fa4a9
> > > 
> > > -- 
> > > Regards,
> > > 
> > > Laurent Pinchart
> 
> -- 
> With best wishes
> Dmitry