Re: [PATCH v1 2/3] drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table

[Abhinav Kumar]

On 6/24/2022 5:23 PM, Stephen Boyd wrote:
> Quoting Stephen Boyd (2022-06-24 17:11:01)
> > Quoting Abhinav Kumar (2022-06-24 17:03:37)
> > > So let me try to explain this as this is what i understood from the
> > > patch and how kuogee explained me.
> > >
> > > The ordering of the array still matters here and thats what he is trying
> > > to address with the second change.
> >
> > The order of the array should not matter. That's the problem.
>
> It seems like somewhere else the order of the array matters, presumably
> while setting up encoders?
>
> > > So as per him, he tried to swap the order of entries like below and that
> > > did not work and that is incorrect behavior because he still retained
> > > the MSM_DP_CONTROLLER_x field for the table like below:
> > >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index dcd80c8a794c..7816e82452ca 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -140,8 +140,8 @@ static const struct msm_dp_config sc7180_dp_cfg = {
> > >
> > >    static const struct msm_dp_config sc7280_dp_cfg = {
> > >           .descs = (const struct msm_dp_desc[]) {
> > > -               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> > > .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> > >                   [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> > > .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> > > +               [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
> > > .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> > >           },
> > >           .num_descs = 2,
> > >    };
> > >
> > >
> > > The reason order is important is because  in this function below, even
> > > though it matches the address to find which one to use it loops through
> > > the array and so the value of *id will change depending on which one is
> > > located where.
> > >
> > > static const struct msm_dp_desc *dp_display_get_desc(struct
> > > platform_device *pdev,
> > >                                unsigned int *id)
> >
> > Thanks! We should fix this function to not overwrite the id.
>
> Ah nevermind. I mixed up dp->id and h_tile_instance thinking one was
> overwriting the other but that doesn't make any sense.

Yes, I also misunderstood one point.

Even if we re-order like above, we are still retaining the index 
correctly so that will still work.

I checked with kuogee again now, he mentioned he needs this only for 
patch 3.

He is not sure of the root-cause of why turning ON the first display 
fixes the issue . I think that needs to be debugged correctly to answers 
questions posted by you / Dmitry. Lets hold on these patches till we 
have the answers.