On Fri, Jun 10, 2022 at 01:54:12PM +0300, Jani Nikula wrote:
> On Thu, 09 Jun 2022, Animesh Manna <animesh.manna@xxxxxxxxx> wrote:
> > Each LFP may have different panel type which is stored in LFP data
> > data block. Based on the child device index respective panel-type/
> > panel-type2 field will be used.
> >
> > v1: Initial rfc verion.
> > v2: Based on review comments from Jani,
> > - Used panel-type instead addition panel-index variable.
> > - DEVICE_HANDLE_* name changed and placed before DEVICE_TYPE_*
> > macro.
> >
> > Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/icl_dsi.c | 2 +-
> > drivers/gpu/drm/i915/display/intel_bios.c | 40 +++++++++++++------
> > drivers/gpu/drm/i915/display/intel_bios.h | 3 +-
> > drivers/gpu/drm/i915/display/intel_dp.c | 3 +-
> > drivers/gpu/drm/i915/display/intel_lvds.c | 3 +-
> > drivers/gpu/drm/i915/display/intel_sdvo.c | 2 +-
> > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 4 ++
> > drivers/gpu/drm/i915/display/vlv_dsi.c | 2 +-
> > 8 files changed, 39 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> > index 3b5305c219ba..b3aa430abd03 100644
> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> > @@ -2050,7 +2050,7 @@ void icl_dsi_init(struct drm_i915_private *dev_priv)
> > /* attach connector to encoder */
> > intel_connector_attach_encoder(intel_connector, encoder);
> >
> > - intel_bios_init_panel(dev_priv, &intel_connector->panel, NULL);
> > + intel_bios_init_panel(dev_priv, intel_connector, NULL);
> >
> > mutex_lock(&dev->mode_config.mutex);
> > intel_panel_add_vbt_lfp_fixed_mode(intel_connector);
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> > index aaea27fe5d16..f74e63823c08 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -604,13 +604,15 @@ get_lfp_data_tail(const struct bdb_lvds_lfp_data *data,
> > }
> >
> > static int opregion_get_panel_type(struct drm_i915_private *i915,
> > - const struct edid *edid)
> > + const struct edid *edid,
> > + const struct intel_bios_encoder_data *devdata)
> > {
> > return intel_opregion_get_panel_type(i915);
> > }
> >
> > static int vbt_get_panel_type(struct drm_i915_private *i915,
> > - const struct edid *edid)
> > + const struct edid *edid,
> > + const struct intel_bios_encoder_data *devdata)
>
> This is nitpicking, but semantically feels like the devdata parameter
> should be before edid.
>
> > {
> > const struct bdb_lvds_options *lvds_options;
> >
> > @@ -625,11 +627,17 @@ static int vbt_get_panel_type(struct drm_i915_private *i915,
> > return -1;
> > }
> >
> > - return lvds_options->panel_type;
> > + if (devdata->child.handle == DEVICE_HANDLE_LFP1)
> > + return lvds_options->panel_type;
> > + else if (devdata->child.handle == DEVICE_HANDLE_LFP2)
> > + return lvds_options->panel_type2;
> > + else
> > + return -1;
>
> Not all legacy panels have encoder data (i.e. VBT child device
> config). I'd go for something like this:
>
> if (devdata && devdata->child.handle == DEVICE_HANDLE_LFP2)
> return lvds_options->panel_type2;
>
> drm_WARN_ON(&i915->drm, devdata && devdata->child.handle != DEVICE_HANDLE_LFP1)
>
> return lvds_options->panel_type;
>
> I don't know if that's going to lead to a bunch of warnings, but I'd
> want to know. Or we can demote it to drm_dbg_kms(), now or later.
I went through my VBT stash and looks like handle==LFP1 should
hold for everything (even my ancient i830 has that). So I'd go
with a WARN.
> > }
> >
> > static int pnpid_get_panel_type(struct drm_i915_private *i915,
> > - const struct edid *edid)
> > + const struct edid *edid,
> > + const struct intel_bios_encoder_data *devdata)
> > {
> > const struct bdb_lvds_lfp_data *data;
> > const struct bdb_lvds_lfp_data_ptrs *ptrs;
> > @@ -675,7 +683,8 @@ static int pnpid_get_panel_type(struct drm_i915_private *i915,
> > }
> >
> > static int fallback_get_panel_type(struct drm_i915_private *i915,
> > - const struct edid *edid)
> > + const struct edid *edid,
> > + const struct intel_bios_encoder_data *devdata)
> > {
> > return 0;
> > }
> > @@ -688,12 +697,14 @@ enum panel_type {
> > };
> >
> > static int get_panel_type(struct drm_i915_private *i915,
> > - const struct edid *edid)
> > + const struct edid *edid,
> > + const struct intel_bios_encoder_data *devdata)
> > {
> > struct {
> > const char *name;
> > int (*get_panel_type)(struct drm_i915_private *i915,
> > - const struct edid *edid);
> > + const struct edid *edid,
> > + const struct intel_bios_encoder_data *devdata);
> > int panel_type;
> > } panel_types[] = {
> > [PANEL_TYPE_OPREGION] = {
> > @@ -716,7 +727,7 @@ static int get_panel_type(struct drm_i915_private *i915,
> > int i;
> >
> > for (i = 0; i < ARRAY_SIZE(panel_types); i++) {
> > - panel_types[i].panel_type = panel_types[i].get_panel_type(i915, edid);
> > + panel_types[i].panel_type = panel_types[i].get_panel_type(i915, edid, devdata);
> >
> > drm_WARN_ON(&i915->drm, panel_types[i].panel_type > 0xf &&
> > panel_types[i].panel_type != 0xff);
> > @@ -747,7 +758,8 @@ static int get_panel_type(struct drm_i915_private *i915,
> > static void
> > parse_panel_options(struct drm_i915_private *i915,
> > struct intel_panel *panel,
> > - const struct edid *edid)
> > + const struct edid *edid,
> > + const struct intel_bios_encoder_data *devdata)
> > {
> > const struct bdb_lvds_options *lvds_options;
> > int panel_type;
> > @@ -759,7 +771,7 @@ parse_panel_options(struct drm_i915_private *i915,
> >
> > panel->vbt.lvds_dither = lvds_options->pixel_dither;
> >
> > - panel_type = get_panel_type(i915, edid);
> > + panel_type = get_panel_type(i915, edid, devdata);
> >
> > panel->vbt.panel_type = panel_type;
> >
> > @@ -3103,12 +3115,16 @@ void intel_bios_init(struct drm_i915_private *i915)
> > }
> >
> > void intel_bios_init_panel(struct drm_i915_private *i915,
> > - struct intel_panel *panel,
> > + struct intel_connector *intel_connector,
> > const struct edid *edid)
> > {
> > + struct intel_panel *panel = &intel_connector->panel;
> > + struct intel_encoder *encoder = intel_connector->encoder;
>
> At least vlv_dsi_init() calls intel_bios_init_panel() before setting
> intel_connector->encoder, which would oops on the next line.
The different order of stuff for vlv_dsi_init() vs. icl_dsi_init()
is rather annoying. Some kind of unification effort might be nice.
>
> > + const struct intel_bios_encoder_data *devdata = i915->vbt.ports[encoder->port];
> > +
>
> intel_bios_init_panel() gets called:
>
> * On platforms/outputs where encoder->port does not make sense,
> e.g. intel_lvds_init() sets it to PORT_NONE.
>
> * On platforms where i915->vbt.ports[] is not initialized at all. See
> has_ddi_port_info().
>
> * On platforms/outputs where i915->vbt.ports[] is not
> initialized. Specifically, DSI is not handled by parse_ddi_port()
> because on VLV, at least in theory, the DSI ports can coexist and
> collide with other ports.
>
> I'm wondering if maybe it's best to have the caller figure out const
> struct intel_bios_encoder_data *, and pass that along. If it's not
> possible, just pass NULL. For DP on DDI platforms it's already set in
> encoder->devdata. (We should basically set that on all platforms where
> it's available, but we're not there yet.)
>
> This should work trivially for the immediate goal of enabling multiple
> eDP panels, and be compatible with enabling multiple DSI or combo
> eDP/DSI panels in the future once we've figured out how to fix devdata
> for DSI.
>
> Ville, thoughts?
Sounds reasonable enough to me.
--
Ville Syrjälä
Intel
