From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrea Merello Subject: Re: [PATCH 2/2] drm: simple_kms_helper: add support for bridges Date: Wed, 24 Aug 2016 08:21:55 +0200 Message-ID: References: <1471961284-12439-1-git-send-email-andrea.merello@gmail.com> <20160823152014.GM10980@phenom.ffwll.local> <20160823155420.GO10980@phenom.ffwll.local> <20160823200156.GT10980@phenom.ffwll.local> Reply-To: andrea.merello@gmail.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1083724943==" Return-path: Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8261A6E001 for ; Wed, 24 Aug 2016 06:22:16 +0000 (UTC) Received: by mail-it0-x241.google.com with SMTP id j124so1134534ith.3 for ; Tue, 23 Aug 2016 23:22:16 -0700 (PDT) In-Reply-To: <20160823200156.GT10980@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1083724943== Content-Type: multipart/alternative; boundary=001a113f328474c22a053acb4d7f --001a113f328474c22a053acb4d7f Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Tue, Aug 23, 2016 at 10:01 PM, Daniel Vetter wrote: > On Tue, Aug 23, 2016 at 06:44:18PM +0200, Andrea Merello wrote: > > On Tue, Aug 23, 2016 at 5:54 PM, Daniel Vetter wrote: > > > > > On Tue, Aug 23, 2016 at 05:39:36PM +0200, Andrea Merello wrote: > > > > On Tue, Aug 23, 2016 at 5:20 PM, Daniel Vetter > wrote: > > > > > > > > > On Tue, Aug 23, 2016 at 04:08:04PM +0200, Andrea Merello wrote: > > > > > > Introduce drm_simple_display_pipe_attach_bridge() in order > > > > > > to make it possible to use drm encoders with the simple display > > > > > > pipes managed by simple_kms_helpers > > > > > > > > > > > > Suggested-by: Daniel Vetter > > > > > > Signed-off-by: Andrea Merello > > > > > > Cc: Noralf Tr=C3=B8nnes > > > > > > Cc: Daniel Vetter > > > > > > Cc: David Airlie > > > > > > > > > > Threading of your patch series is somehow broken, usually that > should > > > all > > > > > work nicely if you've set up git send-email. > > > > > > > > > > One question: Should we ahve a drm_simple_display_pipe_ > detach_bridge > > > (for > > > > > cleanup) too? > > > > > > > > > > > > > Unsure if it worths. May be nice to have a balanced pair, but it > would > > > > probably end up in > > > > a quite redundant one-line func, that only calls drm_bridge_detach > with > > > the > > > > very same argument. > > > > > > > > ..But of course if you want I can add it in v2 series. > > > > > > Yes it's just going to be a one-line, but it'll do a typecast and so > > > better encapsulate the internals of the simple pipe helper. > > > > > > I'm unsure about what do you mean here. Why a typecast? > > > > Wouldn't it be simply drm_simple_display_pipe_detach_bridge(struct > > drm_bridge *bridge) calling in turn drm_detach_bridge(struct drm_bridge > > *bridge) with the very same argument? > > > > Or if you want to stay behind the pipe, then it could be > > drm_simple_display_pipe_detach_bridge(struct drm_simple_display_pipe > *pipe), > > but I would say it just does something like > > drm_bridge_detach(pipe->encoder.bridge), so I don't really get your > point > > about the cast, sorry.. > > Yeah, the latter is what I mean. That way drivers don't have to dig aroun= d > in the details of pipe. And it wouldn't be entirely just a wrapper, I > think it'd be good to also clear pipe->encoder.bridge to NULL. > > Yes, since I'm assigning it in drm_simple_display_pipe_attach_bridge, it seems good to clear it in drm_simple_display_pipe_detach. > Or do you think this is entirely pointless? Sometimes I do go overboard > with curating pretty little functions ;-) > I guess it shouldn't hurt anyway :) Andrea > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > --001a113f328474c22a053acb4d7f Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On T= ue, Aug 23, 2016 at 10:01 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
On Tue, Aug 23, 2016 at 06:44:18PM +0200, Andrea Merello wrote:
> On Tue, Aug 23, 2016 at 5:54 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Tue, Aug 23, 2016 at 05:39:36PM +0200, Andrea Merello wrote: > > > On Tue, Aug 23, 2016 at 5:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > > On Tue, Aug 23, 2016 at 04:08:04PM +0200, Andrea Merell= o wrote:
> > > > > Introduce drm_simple_display_pipe_attach_brid= ge() in order
> > > > > to make it possible to use drm encoders with the s= imple display
> > > > > pipes managed by simple_kms_helpers
> > > > >
> > > > > Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> > > > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > > > > Cc: Noralf Tr=C3=B8nnes <noralf@tronnes.org>
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > Cc: David Airlie <airlied@linux.ie>
> > > >
> > > > Threading of your patch series is somehow broken, usual= ly that should
> > all
> > > > work nicely if you've set up git send-email.
> > > >
> > > > One question: Should we ahve a drm_simple_display_pipe_= detach_bridge
> > (for
> > > > cleanup) too?
> > > >
> > >
> > > Unsure if it worths. May be nice to have a balanced pair, bu= t it would
> > > probably end up in
> > > a quite redundant one-line func, that only calls drm_bridge_= detach with
> > the
> > > very same argument.
> > >
> > > ..But of course if you want I can add it in v2 series.
> >
> > Yes it's just going to be a one-line, but it'll do a type= cast and so
> > better encapsulate the internals of the simple pipe helper.
>
>
> I'm unsure about what do you mean here. Why a typecast?
>
> Wouldn't it be simply drm_simple_display_pipe_detach_bridge(s= truct
> drm_bridge *bridge) calling in turn drm_detach_bridge(struct drm_bridg= e
> *bridge) with the very same argument?
>
> Or if you want to stay behind the pipe, then it could be
> drm_simple_display_pipe_detach_bridge(struct drm_simple_display_p= ipe *pipe),
> but I would say it just does something like
> drm_bridge_detach(pipe->encoder.bridge), so I don't really= get your point
> about the cast, sorry..

Yeah, the latter is what I mean. That way drivers don't hav= e to dig around
in the details of pipe. And it wouldn't be entirely just a wrapper, I think it'd be good to also clear pipe->encoder.bridge to NULL.

Yes, since I'm assigning it in drm_simple_display= _pipe_attach_bridge, it seems good to clear it in drm_simple_display_pipe_d= etach.
=C2=A0
Or do you think this is entirely pointless? Sometimes I do go overboard
with curating pretty little functions ;-)

I guess it sh= ouldn't hurt anyway :)
=C2=A0
Andrea
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http:= //blog.ffwll.ch

--001a113f328474c22a053acb4d7f-- --===============1083724943== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1083724943==--