| Message ID | 1656090912-18074-1-git-send-email-quic_khsieh@quicinc.com |
|---|---|
| Headers | show |
| Series | fix primary corruption issue | expand |
Hi, On Fri, Jun 24, 2022 at 10:15 AM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > With current implementation, communication between interface driver and > upper mdss encoder layer are implemented through function calls. This > increase code complexity. Since struct msm_display_info contains msm > generic display information, it can be expended to contains more useful > information, such as widebus and dcs, in future to serve as communication > channel purpose between interface driver and upper mdss encoder layer so > that existing function calls can be eliminated. > This patch more struct msm_display_info to msm_drv.h to be visible by > whole msm scope. > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 -------------------- > drivers/gpu/drm/msm/msm_drv.h | 19 +++++++++++++++++++ > 2 files changed, 19 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > index 781d41c..6b604c5 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > @@ -19,26 +19,6 @@ > #define IDLE_TIMEOUT (66 - 16/2) > > /** > - * struct msm_display_info - defines display properties > - * @intf_type: DRM_MODE_ENCODER_ type > - * @capabilities: Bitmask of display flags > - * @num_of_h_tiles: Number of horizontal tiles in case of split interface > - * @h_tile_instance: Controller instance used per tile. Number of elements is > - * based on num_of_h_tiles > - * @is_te_using_watchdog_timer: Boolean to indicate watchdog TE is > - * used instead of panel TE in cmd mode panels > - * @dsc: DSC configuration data for DSC-enabled displays > - */ > -struct msm_display_info { > - int intf_type; > - uint32_t capabilities; > - uint32_t num_of_h_tiles; > - uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY]; > - bool is_te_using_watchdog_timer; > - struct msm_display_dsc_config *dsc; So in the structure you're "moving" there's this member called "dsc". > -}; > - > -/** > * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to > * @encoder: encoder pointer > * @crtc: crtc pointer > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index fdbaad5..f9c263b 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -106,11 +106,30 @@ struct msm_drm_thread { > struct kthread_worker *worker; > }; > > +<<<<<<< HEAD The above doesn't look like valid C to me. > /* DSC config */ > struct msm_display_dsc_config { > struct drm_dsc_config *drm; > }; > > +/** > + * struct msm_display_info - defines display properties > + * @intf_type: DRM_MODE_ENCODER_ type > + * @capabilities: Bitmask of display flags > + * @num_of_h_tiles: Number of horizontal tiles in case of split interface > + * @h_tile_instance: Controller instance used per tile. Number of elements is > + * based on num_of_h_tiles > + * @is_te_using_watchdog_timer: Boolean to indicate watchdog TE is > + * used instead of panel TE in cmd mode panels > + */ > +struct msm_display_info { > + int intf_type; > + uint32_t capabilities; > + uint32_t num_of_h_tiles; > + uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY]; > + bool is_te_using_watchdog_timer; ...but then when you "move" the structure to its new location, which should be a noop, then <poof> the "dsc" variable vanishes (along with the kernel doc description of it before the structure). -Doug
On 6/24/2022 2:40 PM, Doug Anderson wrote: > Hi, > > On Fri, Jun 24, 2022 at 10:15 AM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >> With current implementation, communication between interface driver and >> upper mdss encoder layer are implemented through function calls. This >> increase code complexity. Since struct msm_display_info contains msm >> generic display information, it can be expended to contains more useful >> information, such as widebus and dcs, in future to serve as communication >> channel purpose between interface driver and upper mdss encoder layer so >> that existing function calls can be eliminated. >> This patch more struct msm_display_info to msm_drv.h to be visible by >> whole msm scope. >> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 -------------------- >> drivers/gpu/drm/msm/msm_drv.h | 19 +++++++++++++++++++ >> 2 files changed, 19 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> index 781d41c..6b604c5 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> @@ -19,26 +19,6 @@ >> #define IDLE_TIMEOUT (66 - 16/2) >> >> /** >> - * struct msm_display_info - defines display properties >> - * @intf_type: DRM_MODE_ENCODER_ type >> - * @capabilities: Bitmask of display flags >> - * @num_of_h_tiles: Number of horizontal tiles in case of split interface >> - * @h_tile_instance: Controller instance used per tile. Number of elements is >> - * based on num_of_h_tiles >> - * @is_te_using_watchdog_timer: Boolean to indicate watchdog TE is >> - * used instead of panel TE in cmd mode panels >> - * @dsc: DSC configuration data for DSC-enabled displays >> - */ >> -struct msm_display_info { >> - int intf_type; >> - uint32_t capabilities; >> - uint32_t num_of_h_tiles; >> - uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY]; >> - bool is_te_using_watchdog_timer; >> - struct msm_display_dsc_config *dsc; > So in the structure you're "moving" there's this member called "dsc". > > >> -}; >> - >> -/** >> * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to >> * @encoder: encoder pointer >> * @crtc: crtc pointer >> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h >> index fdbaad5..f9c263b 100644 >> --- a/drivers/gpu/drm/msm/msm_drv.h >> +++ b/drivers/gpu/drm/msm/msm_drv.h >> @@ -106,11 +106,30 @@ struct msm_drm_thread { >> struct kthread_worker *worker; >> }; >> >> +<<<<<<< HEAD > The above doesn't look like valid C to me. > > >> /* DSC config */ >> struct msm_display_dsc_config { >> struct drm_dsc_config *drm; >> }; >> >> +/** >> + * struct msm_display_info - defines display properties >> + * @intf_type: DRM_MODE_ENCODER_ type >> + * @capabilities: Bitmask of display flags >> + * @num_of_h_tiles: Number of horizontal tiles in case of split interface >> + * @h_tile_instance: Controller instance used per tile. Number of elements is >> + * based on num_of_h_tiles >> + * @is_te_using_watchdog_timer: Boolean to indicate watchdog TE is >> + * used instead of panel TE in cmd mode panels >> + */ >> +struct msm_display_info { >> + int intf_type; >> + uint32_t capabilities; >> + uint32_t num_of_h_tiles; >> + uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY]; >> + bool is_te_using_watchdog_timer; > ...but then when you "move" the structure to its new location, which > should be a noop, then <poof> the "dsc" variable vanishes (along with > the kernel doc description of it before the structure). Sorry, i did not resolve the conflicts correctly when i cherry-pick them to msm-next tree. Will fix them. > -Doug
On Fri, 24 Jun 2022 at 20:15, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > With current implementation, communication between interface driver and > upper mdss encoder layer are implemented through function calls. This > increase code complexity. Since struct msm_display_info contains msm > generic display information, it can be expended to contains more useful > information, such as widebus and dcs, in future to serve as communication > channel purpose between interface driver and upper mdss encoder layer so > that existing function calls can be eliminated. > This patch more struct msm_display_info to msm_drv.h to be visible by > whole msm scope. NAK. The msm_display_info contains information used by (and useful to) DPU only, it is not 'msm generic' info. For this reason it has been moved from msm_drv.h to dpu_encoder.h inIn commit b7420739f112 ("drm/msm: move struct msm_display_info to dpu driver") . Neither mdp5 nor mdp4 are going to use this structure. At some point I thought too that we might be able to create a set of data and functions to describe encoder backends (dsi, hdmi, dp). This has failed for me. After musing over the msm_drv.h part containing functions published by the backends, I could not end up with a set of them being good enough. The only common part seems to be the modeset_init, snapshot and (once DP gets the DSC interface) get_dsc_config. The rest is backend-specific. I would suggest returning to this topic if/once DSI gets wide bus support or DP starts using bonded interfaces. Before that I don't foresee that common data structure would simplify things rather than complicating them. > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 -------------------- > drivers/gpu/drm/msm/msm_drv.h | 19 +++++++++++++++++++ > 2 files changed, 19 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > index 781d41c..6b604c5 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > @@ -19,26 +19,6 @@ > #define IDLE_TIMEOUT (66 - 16/2) > > /** > - * struct msm_display_info - defines display properties > - * @intf_type: DRM_MODE_ENCODER_ type > - * @capabilities: Bitmask of display flags > - * @num_of_h_tiles: Number of horizontal tiles in case of split interface > - * @h_tile_instance: Controller instance used per tile. Number of elements is > - * based on num_of_h_tiles > - * @is_te_using_watchdog_timer: Boolean to indicate watchdog TE is > - * used instead of panel TE in cmd mode panels > - * @dsc: DSC configuration data for DSC-enabled displays > - */ > -struct msm_display_info { > - int intf_type; > - uint32_t capabilities; > - uint32_t num_of_h_tiles; > - uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY]; > - bool is_te_using_watchdog_timer; > - struct msm_display_dsc_config *dsc; > -}; > - > -/** > * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to > * @encoder: encoder pointer > * @crtc: crtc pointer > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index fdbaad5..f9c263b 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -106,11 +106,30 @@ struct msm_drm_thread { > struct kthread_worker *worker; > }; > > +<<<<<<< HEAD Not to mention that this patch is broken per se. > /* DSC config */ > struct msm_display_dsc_config { > struct drm_dsc_config *drm; > }; > > +/** > + * struct msm_display_info - defines display properties > + * @intf_type: DRM_MODE_ENCODER_ type > + * @capabilities: Bitmask of display flags > + * @num_of_h_tiles: Number of horizontal tiles in case of split interface > + * @h_tile_instance: Controller instance used per tile. Number of elements is > + * based on num_of_h_tiles > + * @is_te_using_watchdog_timer: Boolean to indicate watchdog TE is > + * used instead of panel TE in cmd mode panels > + */ > +struct msm_display_info { > + int intf_type; > + uint32_t capabilities; > + uint32_t num_of_h_tiles; > + uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY]; > + bool is_te_using_watchdog_timer; > +}; > + > struct msm_drm_private { > > struct drm_device *dev; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- With best wishes Dmitry
On Sat, 25 Jun 2022 at 00:51, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > On 6/24/2022 2:40 PM, Doug Anderson wrote: > > On Fri, Jun 24, 2022 at 10:15 AM Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > >> +struct msm_display_info { > >> + int intf_type; > >> + uint32_t capabilities; > >> + uint32_t num_of_h_tiles; > >> + uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY]; > >> + bool is_te_using_watchdog_timer; > > ...but then when you "move" the structure to its new location, which > > should be a noop, then <poof> the "dsc" variable vanishes (along with > > the kernel doc description of it before the structure). > > Sorry, i did not resolve the conflicts correctly when i cherry-pick > them to msm-next tree. > > Will fix them. I would strongly suggest doing development on top of msm/next or linux-next. Using any other tree results in lots of problems starting from the lame Fixes tags that we have been constantly seeing for the last few months, conflicts when the patch is being rebased or cherry-picked and ending up with the patches not being tested with the tree that they are being applied to.
Quoting Kuogee Hsieh (2022-06-24 18:02:50) > > On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote: > > On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > >> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > >>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote: > >>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > >>>>> How can I have eDP call dpu_encoder_init() before DP calls with > >>>>> _dpu_kms_initialize_displayport()? > >>>> Why do you want to do it? They are two different encoders. > >>> eDP is primary display which in normal case should be bring up first if > >>> DP is also presented. > >> I do not like the concept of primary display. It is the user, who must > >> decide which display is primary to him. I have seen people using just > >> external monitors and ignoring built-in eDP completely.from > > >> Also, why does the bring up order matters here? What do you gain by > >> bringing up eDP before the DP? > > I should probably rephrase my question to be more clear. How does > > changing the order of DP vs eDP bringup help you 'to fix screen > > corruption'. > > it did fix the primary display correction issue if edp go first and i do > not know the root cause yet. > > We are still investigating it. > > However I do think currently msm_dp_config sc7280_dp_cfg has issues need > be addressed. > What issues exist with sc7280_dp_cfg? It looks correct to me.
On 6/24/2022 6:15 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2022-06-24 18:02:50) >> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote: >>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov >>> <dmitry.baryshkov@linaro.org> wrote: >>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote: >>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >>>>>>> How can I have eDP call dpu_encoder_init() before DP calls with >>>>>>> _dpu_kms_initialize_displayport()? >>>>>> Why do you want to do it? They are two different encoders. >>>>> eDP is primary display which in normal case should be bring up first if >>>>> DP is also presented. >>>> I do not like the concept of primary display. It is the user, who must >>>> decide which display is primary to him. I have seen people using just >>>> external monitors and ignoring built-in eDP completely.from >>>> Also, why does the bring up order matters here? What do you gain by >>>> bringing up eDP before the DP? >>> I should probably rephrase my question to be more clear. How does >>> changing the order of DP vs eDP bringup help you 'to fix screen >>> corruption'. >> it did fix the primary display correction issue if edp go first and i do >> not know the root cause yet. >> >> We are still investigating it. >> >> However I do think currently msm_dp_config sc7280_dp_cfg has issues need >> be addressed. >> > What issues exist with sc7280_dp_cfg? It looks correct to me. If we are going to bring up a new chipset with edp as primary only, i am not sure the below configuration will work? > static const struct msm_dp_config sc7280_dp_cfg = { > .descs = (const struct msm_dp_desc[]) { > [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true }, > }, > .num_descs = 1, > };
On Mon, 27 Jun 2022 at 18:33, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > > On 6/24/2022 6:15 PM, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2022-06-24 18:02:50) > >> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote: > >>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov > >>> <dmitry.baryshkov@linaro.org> wrote: > >>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > >>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote: > >>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > >>>>>>> How can I have eDP call dpu_encoder_init() before DP calls with > >>>>>>> _dpu_kms_initialize_displayport()? > >>>>>> Why do you want to do it? They are two different encoders. > >>>>> eDP is primary display which in normal case should be bring up first if > >>>>> DP is also presented. > >>>> I do not like the concept of primary display. It is the user, who must > >>>> decide which display is primary to him. I have seen people using just > >>>> external monitors and ignoring built-in eDP completely.from > >>>> Also, why does the bring up order matters here? What do you gain by > >>>> bringing up eDP before the DP? > >>> I should probably rephrase my question to be more clear. How does > >>> changing the order of DP vs eDP bringup help you 'to fix screen > >>> corruption'. > >> it did fix the primary display correction issue if edp go first and i do > >> not know the root cause yet. > >> > >> We are still investigating it. > >> > >> However I do think currently msm_dp_config sc7280_dp_cfg has issues need > >> be addressed. > >> > > What issues exist with sc7280_dp_cfg? It looks correct to me. > > > If we are going to bring up a new chipset with edp as primary only, i am > not sure the below configuration will work? > > > static const struct msm_dp_config sc7280_dp_cfg = { > > .descs = (const struct msm_dp_desc[]) { > > [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true }, > > }, > > .num_descs = 1, > > }; As I wrote in one of the comments, there is an issue with num_descs being not obvious (in your example it should be 2, not 1). I thought about dropping it and looping until the MSM_DP_CONTROLLER_COUNT, but this would result in other kinds of hard-to-catch issues. Let me muse about it.
On 6/27/2022 8:38 AM, Dmitry Baryshkov wrote: > On Mon, 27 Jun 2022 at 18:33, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >> >> On 6/24/2022 6:15 PM, Stephen Boyd wrote: >>> Quoting Kuogee Hsieh (2022-06-24 18:02:50) >>>> On 6/24/2022 5:46 PM, Dmitry Baryshkov wrote: >>>>> On Sat, 25 Jun 2022 at 03:28, Dmitry Baryshkov >>>>> <dmitry.baryshkov@linaro.org> wrote: >>>>>> On Sat, 25 Jun 2022 at 03:23, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >>>>>>> On 6/24/2022 5:21 PM, Dmitry Baryshkov wrote: >>>>>>>> On Sat, 25 Jun 2022 at 03:19, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >>>>>>>>> How can I have eDP call dpu_encoder_init() before DP calls with >>>>>>>>> _dpu_kms_initialize_displayport()? >>>>>>>> Why do you want to do it? They are two different encoders. >>>>>>> eDP is primary display which in normal case should be bring up first if >>>>>>> DP is also presented. >>>>>> I do not like the concept of primary display. It is the user, who must >>>>>> decide which display is primary to him. I have seen people using just >>>>>> external monitors and ignoring built-in eDP completely.from >>>>>> Also, why does the bring up order matters here? What do you gain by >>>>>> bringing up eDP before the DP? >>>>> I should probably rephrase my question to be more clear. How does >>>>> changing the order of DP vs eDP bringup help you 'to fix screen >>>>> corruption'. >>>> it did fix the primary display correction issue if edp go first and i do >>>> not know the root cause yet. >>>> >>>> We are still investigating it. >>>> >>>> However I do think currently msm_dp_config sc7280_dp_cfg has issues need >>>> be addressed. >>>> >>> What issues exist with sc7280_dp_cfg? It looks correct to me. >> >> If we are going to bring up a new chipset with edp as primary only, i am >> not sure the below configuration will work? >> >>> static const struct msm_dp_config sc7280_dp_cfg = { >>> .descs = (const struct msm_dp_desc[]) { >>> [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true }, >>> }, >>> .num_descs = 1, >>> }; > As I wrote in one of the comments, there is an issue with num_descs > being not obvious (in your example it should be 2, not 1). I thought > about dropping it and looping until the MSM_DP_CONTROLLER_COUNT, but > this would result in other kinds of hard-to-catch issues. Let me muse > about it. Thanks for consideration.
fix primary corruption : 1) move struc of msm_display_info to msm_drv.h 2) decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table 3) place edp at head of drm bridge chain to fix screen corruption Kuogee Hsieh (3): drm/msm/dp: move struc of msm_display_info to msm_drv.h drm/msm/dp: decoupling dp->id out of dp controller_id at scxxxx_dp_cfg table drm/msm/dp: place edp at head of drm bridge chain to fix screen corruption drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 20 ------------------- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 +++++++++------ drivers/gpu/drm/msm/dp/dp_display.c | 30 ++++++++++++++++++++++------- drivers/gpu/drm/msm/msm_drv.h | 21 ++++++++++++++++++++ 4 files changed, 54 insertions(+), 33 deletions(-)