Re: [PATCH v3 09/13] drm: Add encoder_type field to the drm_bridge structure

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

On Tuesday 29 Nov 2016 21:25:27 Daniel Vetter wrote:
> On Tue, Nov 29, 2016 at 07:49:22PM +0200, Laurent Pinchart wrote:
> > On Tuesday 29 Nov 2016 11:27:20 Daniel Vetter wrote:
> >> On Tue, Nov 29, 2016 at 11:58:44AM +0200, Laurent Pinchart wrote:
> >>> On Tuesday 29 Nov 2016 10:56:53 Daniel Vetter wrote:
> >>>> On Tue, Nov 29, 2016 at 11:04:39AM +0200, Laurent Pinchart wrote:
> >>>>> The drm_bridge object models on- or off-chip hardware encoders and
> >>>>> provide an abstract control API to display drivers. In order to help
> >>>>> display drivers creating the right kind of drm_encoder object,
> >>>>> expose the type of the hardware encoder associated with each bridge.
> >>>>> 
> >>>>> Signed-off-by: Laurent Pinchart
> >>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>> 
> >>>> DRM_MODE_ENCODER_BRIDGE. Problem solved, because in reality no one
> >>>> cares one iota about the encoder type.
> >>> 
> >>> It's exposed to userspace though, are you 100% sure we won't break
> >>> anything ?
> >> 
> >> We've added DP, DSI, DPMST and DPI encoder types thus far, no one
> >> screamed.
> > 
> > In that case why don't we go one step further and remove the encoder type
> > completely ? We can't remove the field from the API, but we can hardcode
> > it to a single value.
> > 
> > There are however drivers that rely on the encoder type (radeon, nouveau,
> > sti, amdgpu, msm and rcar-du, but I'll fix the last one) so we'd need to
> > address that first. If we don't want to remove the encoder_type field
> > from in-kernel structures and let drivers use it, then I don't think
> > DRM_MODE_ENCODER_BRIDGE would be a good option, we should report the real
> > type instead.
> 
> If you strongly believe that I will not stop you. This was just a
> suggestion to get all your stuff landed with minimal amounts of effort and
> across-the-subsystem cleanup needed. I'd do it that ;-)
> 
> And if you don't like DRM_MODE_ENCODER_BRIDGE you could also pick
> DRM_MODE_ENCODER_NONE, which is what most seem to do today. In the end it
> doesn't matter no matter which option you pick. The only difference is in
> the amount of effort you need to spend to get it merged ...

I've tried hardcoding the encoder type to DRM_MODE_ENCODER_NONE and basic 
tests still pass (I haven't tried more complex userspace stacks such as X or 
Weston though). I'll thus drop the addition of encoder_type to the bridge for 
now. As a result we should start deprecating the drm_encoder::encoder_type 
field (unless a compelling reason is found of course, in which case I'd have 
to revive drm_bridge::encoder_type).

-- 
Regards,

Laurent Pinchart