Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 4 Jul 2023 18:56:54 +0200
From:   Sam Ravnborg <sam@ravnborg.org>
To:     Mohammad Faiz Abbas Rizvi <faiz.abbas@arm.com>
Cc:     Liviu Dudau <liviu.dudau@arm.com>, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/arm/komeda: Remove component framework and add a
 simple encoder
Message-ID: <20230704165654.GA940443@ravnborg.org>
References: <20230621084116.26882-1-faiz.abbas@arm.com>
 <ZJ1UJaNJese6g2Ia@e110455-lin.cambridge.arm.com>
 <90f386c3-b2bb-b876-80df-c67005e89a66@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <90f386c3-b2bb-b876-80df-c67005e89a66@arm.com>
Precedence: bulk

Hi Mohammed,

On Tue, Jul 04, 2023 at 07:19:04PM +0530, Mohammad Faiz Abbas Rizvi wrote:
> Hi Liviu,
> 
> On 6/29/2023 3:21 PM, Liviu Dudau wrote:
> > Hi Faiz,
> >
> > Thanks for the patch and for addressing what was at some moment on my "nice to
> > improve / cleanup" list. Sorry for the delay in responding, I had to revive
> > the bits of an old setup to be able to test this properly, with 2 encoders
> > attached.
> >
> > On Wed, Jun 21, 2023 at 02:11:16PM +0530, Faiz Abbas wrote:
> >> The Komeda driver always expects the remote connector node to initialize
> >> an encoder. It uses the component aggregator framework which consists
> >> of component->bind() calls used to initialize the remote encoder and attach
> >> it to the crtc. This is different from other drm implementations which expect
> >> the display driver to supply a crtc and connect an encoder to it.
> > I think both approaches are valid in DRM. We don't want to remove the component
> > framework because it is doing the wrong thing, but because we cannot use it
> > with drivers that implement the drm_bridge API. Given that we usually pair with
> > a component encoder that also implements a drm_bridge, dropping support for
> > component framework should not affect the users of the driver.

Glad to see the patch - I think this is moving things in the right
direction.


While at it do you plan to support DRM_BRIDGE_ATTACH_NO_CONNECTOR?

I did not read the patch carefully but noticed this call with no flags:

> drm_bridge_attach(&kcrtc->encoder, bridge, NULL, 0);

To add support for DRM_BRIDGE_ATTACH_NO_CONNECTOR you may need to verify
that all relevant bridge drivers supports the flag.
You will be told at runtime but only if the relevant bridge driver is
used.

It can be done later and is obviously a separate patch.

	Sam