Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 8 Oct 2019 11:02:57 -0400
From:   Sean Paul <sean@poorly.run>
To:     "dbasehore ." <dbasehore@chromium.org>
Cc:     Sean Paul <sean@poorly.run>,
        "james qian wang (Arm Technology China)" <james.qian.wang@arm.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Maxime Ripard <maxime.ripard@bootlin.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        David Airlie <airlied@linux.ie>,
        Thierry Reding <thierry.reding@gmail.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        "linux-mediatek@lists.infradead.org" 
        <linux-mediatek@lists.infradead.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>, nd <nd@arm.com>
Subject: Re: [v8,2/4] drm/panel: set display info in panel attach
Message-ID: <20191008150257.GB85762@art_vandelay>
References: <20190925225833.7310-3-dbasehore@chromium.org>
 <20190929052307.GA28304@jamwan02-TSP300>
 <CAGAzgspEA0mcEHwgxyWWh3Gn-iC+a21g5GUrhsRJrTHQ_OAYqQ@mail.gmail.com>
 <20191007164441.GB126146@art_vandelay>
 <CAGAzgsoGNNTeYTmRyC5YNGDHL+SBB2oCFaHFubYa=dnPst=f8Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGAzgsoGNNTeYTmRyC5YNGDHL+SBB2oCFaHFubYa=dnPst=f8Q@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

/snip

> > > > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > > > > index d16158deacdc..f3587a54b8ac 100644
> > > > > --- a/include/drm/drm_panel.h
> > > > > +++ b/include/drm/drm_panel.h
> > > > > @@ -141,6 +141,56 @@ struct drm_panel {
> > > > >        */
> > > > >       const struct drm_panel_funcs *funcs;
> > > > >
> > > >
> > > > All these new added members seems dupliated with drm_display_info,
> > > > So I think, can we add a new drm_plane_funcs func:
> > > >
> > > > int (*set_display_info)(struct drm_panel *panel,
> > > >                         struct drm_display_info *info);
> > >
> > > I don't disagree personally, since I originally wrote it this way, but
> > > 2 maintainers, Daniel Vetter and Thierry Reding, requested that it be
> > > changed. Unless that decision is reversed, I won't be changing this.
> > >
> >
> > Reading back the feedback on v1, I don't think anyone said they were against
> > storing a drm_display_info struct in drm_panel (no one really weighed in on
> > it one way or another). IMO duplicating a bunch of fields feels pretty icky.
> 
> Thanks for the review. Should we duplicate the entire struct, or just
> create a sub-struct, say, physical_properties that will be part of
> drm_display_info and drm_panel?

That's a good idea, but I think you can use the entire struct. Everything has
reasonable default values and I think it might be difficult to draw a line in
the sand as to which fields will or won't be useful for panels going forward.
Best for drivers to just treat the info in drm_display_info as best effort and
have good defaults IMO.

Sean

> 
> >
> > I think most fields in drm_display_info have pretty reasonable defaults, so I'd
> > recommend just storing a copy in drm_panel. As Thierry suggests, we can
> > populate the dt parts in probe (orientation) and then copy over all or a subset
> > of the struct to connector on attach.
> >
> > Sean
> >
> > > >
> > > > Then in drm_panel_attach(), via this interface the specific panel
> > > > driver can directly set connector->display_info. like
> > > >
> > > >    ...
> > > >    if (panel->funcs && panel->funcs->set_display_info)
> > > >                 panel->funcs->unprepare(panel, connector->display_info);
> > > >    ...
> > > >
> > > > Thanks
> > > > James
> > > >
> > > > > +     /**
> > > > > +      * @width_mm:
> > > > > +      *
> > > > > +      * Physical width in mm.
> > > > > +      */
> > > > > +     unsigned int width_mm;
> > > > > +
> > > > > +     /**
> > > > > +      * @height_mm:
> > > > > +      *
> > > > > +      * Physical height in mm.
> > > > > +      */
> > > > > +     unsigned int height_mm;
> > > > > +
> > > > > +     /**
> > > > > +      * @bpc:
> > > > > +      *
> > > > > +      * Maximum bits per color channel. Used by HDMI and DP outputs.
> > > > > +      */
> > > > > +     unsigned int bpc;
> > > > > +
> > > > > +     /**
> > > > > +      * @orientation
> > > > > +      *
> > > > > +      * Installation orientation of the panel with respect to the chassis.
> > > > > +      */
> > > > > +     int orientation;
> > > > > +
> > > > > +     /**
> > > > > +      * @bus_formats
> > > > > +      *
> > > > > +      * Pixel data format on the wire.
> > > > > +      */
> > > > > +     const u32 *bus_formats;
> > > > > +
> > > > > +     /**
> > > > > +      * @num_bus_formats:
> > > > > +      *
> > > > > +      * Number of elements pointed to by @bus_formats
> > > > > +      */
> > > > > +     unsigned int num_bus_formats;
> > > > > +
> > > > > +     /**
> > > > > +      * @bus_flags:
> > > > > +      *
> > > > > +      * Additional information (like pixel signal polarity) for the pixel
> > > > > +      * data on the bus.
> > > > > +      */
> > > > > +     u32 bus_flags;
> > > > > +
> > > > >       /**
> > > > >        * @list:
> > > > >        *
> > >
> > > Thanks for the review
> >
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS