| Message ID | 20220919-v1-17-4844816c9808@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add MT8195 HDMI support | expand |
On 19/09/2022 18:56, Guillaume Ranquet wrote: > Add dpi support to enable the HDMI path. > > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > index 72049a530ae1..27f029ca760b 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > @@ -820,6 +820,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = { > .data = (void *)MTK_DPI }, > { .compatible = "mediatek,mt8192-dpi", > .data = (void *)MTK_DPI }, > + { .compatible = "mediatek,mt8195-dpi", > + .data = (void *)MTK_DPI }, It's compatible with the others. You don't need more compatibles. Best regards, Krzysztof
On Thu, 22 Sep 2022 09:20, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >On 19/09/2022 18:56, Guillaume Ranquet wrote: >> Add dpi support to enable the HDMI path. >> >> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> >> >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> index 72049a530ae1..27f029ca760b 100644 >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> @@ -820,6 +820,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = { >> .data = (void *)MTK_DPI }, >> { .compatible = "mediatek,mt8192-dpi", >> .data = (void *)MTK_DPI }, >> + { .compatible = "mediatek,mt8195-dpi", >> + .data = (void *)MTK_DPI }, > >It's compatible with the others. You don't need more compatibles. Hi Krzysztof, It's a bit confusing, because this compatible is used in both mtk_drm_drv.c and in mtk_dpi.c Albeit it's entirely the same thing regarding the mtk_drm_drv module, it's pretty different regarding the mtk_dpi module. Thx, Guillaume. > >Best regards, >Krzysztof >
On 27/09/2022 15:04, Guillaume Ranquet wrote: > On Thu, 22 Sep 2022 09:20, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> On 19/09/2022 18:56, Guillaume Ranquet wrote: >>> Add dpi support to enable the HDMI path. >>> >>> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> >>> >>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>> index 72049a530ae1..27f029ca760b 100644 >>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>> @@ -820,6 +820,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = { >>> .data = (void *)MTK_DPI }, >>> { .compatible = "mediatek,mt8192-dpi", >>> .data = (void *)MTK_DPI }, >>> + { .compatible = "mediatek,mt8195-dpi", >>> + .data = (void *)MTK_DPI }, >> >> It's compatible with the others. You don't need more compatibles. > > Hi Krzysztof, > > It's a bit confusing, because this compatible is used in both > mtk_drm_drv.c and in mtk_dpi.c > > Albeit it's entirely the same thing regarding the mtk_drm_drv module, > it's pretty different > regarding the mtk_dpi module. Sure, but this does not explain why do you need these entries here in mtk_drm_drv. Best regards, Krzysztof
On Tue, 27 Sep 2022 16:28, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >On 27/09/2022 15:04, Guillaume Ranquet wrote: >> On Thu, 22 Sep 2022 09:20, Krzysztof Kozlowski >> <krzysztof.kozlowski@linaro.org> wrote: >>> On 19/09/2022 18:56, Guillaume Ranquet wrote: >>>> Add dpi support to enable the HDMI path. >>>> >>>> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> >>>> >>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>>> index 72049a530ae1..27f029ca760b 100644 >>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>>> @@ -820,6 +820,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = { >>>> .data = (void *)MTK_DPI }, >>>> { .compatible = "mediatek,mt8192-dpi", >>>> .data = (void *)MTK_DPI }, >>>> + { .compatible = "mediatek,mt8195-dpi", >>>> + .data = (void *)MTK_DPI }, >>> >>> It's compatible with the others. You don't need more compatibles. >> >> Hi Krzysztof, >> >> It's a bit confusing, because this compatible is used in both >> mtk_drm_drv.c and in mtk_dpi.c >> >> Albeit it's entirely the same thing regarding the mtk_drm_drv module, >> it's pretty different >> regarding the mtk_dpi module. > >Sure, but this does not explain why do you need these entries here in >mtk_drm_drv. > >Best regards, >Krzysztof > Hi Krzysztof, Sorry for the late answer. The mtk_drm_drv is the component master of the full mediatek drm stack. it "binds" all of the crtc/dpi/ovl/mutex/merge... components of the stack. That mtk_ddp_comp_dt_ids array is iterated over to find all of the components from the device tree. Hope this clarifies things? Thx, Guillaume.
On 03/10/2022 17:29, Guillaume Ranquet wrote: > On Tue, 27 Sep 2022 16:28, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> On 27/09/2022 15:04, Guillaume Ranquet wrote: >>> On Thu, 22 Sep 2022 09:20, Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> On 19/09/2022 18:56, Guillaume Ranquet wrote: >>>>> Add dpi support to enable the HDMI path. >>>>> >>>>> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> >>>>> >>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>>>> index 72049a530ae1..27f029ca760b 100644 >>>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>>>> @@ -820,6 +820,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = { >>>>> .data = (void *)MTK_DPI }, >>>>> { .compatible = "mediatek,mt8192-dpi", >>>>> .data = (void *)MTK_DPI }, >>>>> + { .compatible = "mediatek,mt8195-dpi", >>>>> + .data = (void *)MTK_DPI }, >>>> >>>> It's compatible with the others. You don't need more compatibles. >>> >>> Hi Krzysztof, >>> >>> It's a bit confusing, because this compatible is used in both >>> mtk_drm_drv.c and in mtk_dpi.c >>> >>> Albeit it's entirely the same thing regarding the mtk_drm_drv module, >>> it's pretty different >>> regarding the mtk_dpi module. >> >> Sure, but this does not explain why do you need these entries here in >> mtk_drm_drv. >> >> Best regards, >> Krzysztof >> > > Hi Krzysztof, > > Sorry for the late answer. > The mtk_drm_drv is the component master of the full mediatek drm stack. > > it "binds" all of the crtc/dpi/ovl/mutex/merge... components of the stack. > > That mtk_ddp_comp_dt_ids array is iterated over to find all of the components > from the device tree. No. You said what the code is doing. I think I understand this. You still do not need more compatibles. Your sentence did not clarify it because it did not answer at all to question "why". Why do you need it? Sorry, the change looks not correct. Best regards, Krzysztof
On Tue, 04 Oct 2022 12:49, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >On 03/10/2022 17:29, Guillaume Ranquet wrote: >> On Tue, 27 Sep 2022 16:28, Krzysztof Kozlowski >> <krzysztof.kozlowski@linaro.org> wrote: >>> On 27/09/2022 15:04, Guillaume Ranquet wrote: >>>> On Thu, 22 Sep 2022 09:20, Krzysztof Kozlowski >>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>> On 19/09/2022 18:56, Guillaume Ranquet wrote: >>>>>> Add dpi support to enable the HDMI path. >>>>>> >>>>>> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> >>>>>> >>>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>>>>> index 72049a530ae1..27f029ca760b 100644 >>>>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>>>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>>>>> @@ -820,6 +820,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = { >>>>>> .data = (void *)MTK_DPI }, >>>>>> { .compatible = "mediatek,mt8192-dpi", >>>>>> .data = (void *)MTK_DPI }, >>>>>> + { .compatible = "mediatek,mt8195-dpi", >>>>>> + .data = (void *)MTK_DPI }, >>>>> >>>>> It's compatible with the others. You don't need more compatibles. >>>> >>>> Hi Krzysztof, >>>> >>>> It's a bit confusing, because this compatible is used in both >>>> mtk_drm_drv.c and in mtk_dpi.c >>>> >>>> Albeit it's entirely the same thing regarding the mtk_drm_drv module, >>>> it's pretty different >>>> regarding the mtk_dpi module. >>> >>> Sure, but this does not explain why do you need these entries here in >>> mtk_drm_drv. >>> >>> Best regards, >>> Krzysztof >>> >> >> Hi Krzysztof, >> >> Sorry for the late answer. >> The mtk_drm_drv is the component master of the full mediatek drm stack. >> >> it "binds" all of the crtc/dpi/ovl/mutex/merge... components of the stack. >> >> That mtk_ddp_comp_dt_ids array is iterated over to find all of the components >> from the device tree. > >No. You said what the code is doing. I think I understand this. You >still do not need more compatibles. Your sentence did not clarify it >because it did not answer at all to question "why". Why do you need it? > >Sorry, the change looks not correct. > >Best regards, >Krzysztof > I need a new compatible to adress the specifics of mt8195 in the mtk_dpi driver, the change is in this series with: [PATCH v1 16/17] drm/mediatek: dpi: Add mt8195 hdmi to DPI driver [1] I then need to add that compatible to the "list" here in mtk_drm_drv. I don't see a way around this unless I rewrite the way mtk_drm_drv works? Maybe if I declare a new compatible that is generic to all mediatek dpi variants? and have all the dts specify the node with both the generic dpi and the specific compatible? dpi@xxx { compatible = "mediatek,dpi", "mediatek,mt8195-dpi"; ... } Then I can "collapse" all the dpi related nodes in mtk_drm_drv under "mediatek,dpi" ? I guess would have to do the change for all other components that are needed in mtk_drm_drv (mmsys, aal, ccor, color, dither, dsc, gamma, mutex...). That's the only trivial way I can think of implementing this with the current status of the mtk_drm stack. Do you have any other ideas in mind? Thx, Guillaume. [1] : https://lore.kernel.org/all/20220919-v1-16-4844816c9808@baylibre.com/
On 04/10/2022 13:55, Guillaume Ranquet wrote: >> No. You said what the code is doing. I think I understand this. You >> still do not need more compatibles. Your sentence did not clarify it >> because it did not answer at all to question "why". Why do you need it? >> >> Sorry, the change looks not correct. >> >> Best regards, >> Krzysztof >> > > I need a new compatible to adress the specifics of mt8195 in the mtk_dpi driver, > the change is in this series with: > [PATCH v1 16/17] drm/mediatek: dpi: Add mt8195 hdmi to DPI driver [1] But you do not have specifics of mt8195. I wrote it in the beginning. > > I then need to add that compatible to the "list" here in mtk_drm_drv. No, you do not... I checked the driver and there is no single need... or convince me you need. > I don't see a way around this unless I rewrite the way mtk_drm_drv works? Why rewrite? You have all compatibles in place. > > Maybe if I declare a new compatible that is generic to all mediatek > dpi variants? You were asked to use fallback. Don't create some fake fallbacks. Use existing ones. > and have all the dts specify the node with both the generic dpi and > the specific compatible? > > dpi@xxx { > compatible = "mediatek,dpi", "mediatek,mt8195-dpi"; I don't know what's this but certainly looks odd. Some wild-card compatible in front (not fallback) and none are documented. > ... > } > > Then I can "collapse" all the dpi related nodes in mtk_drm_drv under > "mediatek,dpi" ? > > I guess would have to do the change for all other components that are needed in > mtk_drm_drv (mmsys, aal, ccor, color, dither, dsc, gamma, mutex...). > > That's the only trivial way I can think of implementing this with the > current status > of the mtk_drm stack. > > Do you have any other ideas in mind? Use fallback of compatible device. That's the common pattern. Everywhere, Mediatek as well. Best regards, Krzysztof
On Tue, 04 Oct 2022 17:05, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >On 04/10/2022 13:55, Guillaume Ranquet wrote: >>> No. You said what the code is doing. I think I understand this. You >>> still do not need more compatibles. Your sentence did not clarify it >>> because it did not answer at all to question "why". Why do you need it? >>> >>> Sorry, the change looks not correct. >>> >>> Best regards, >>> Krzysztof >>> >> >> I need a new compatible to adress the specifics of mt8195 in the mtk_dpi driver, >> the change is in this series with: >> [PATCH v1 16/17] drm/mediatek: dpi: Add mt8195 hdmi to DPI driver [1] > >But you do not have specifics of mt8195. I wrote it in the beginning. > >> >> I then need to add that compatible to the "list" here in mtk_drm_drv. > >No, you do not... I checked the driver and there is no single need... or >convince me you need. > >> I don't see a way around this unless I rewrite the way mtk_drm_drv works? > >Why rewrite? You have all compatibles in place. > >> >> Maybe if I declare a new compatible that is generic to all mediatek >> dpi variants? > >You were asked to use fallback. Don't create some fake fallbacks. Use >existing ones. > >> and have all the dts specify the node with both the generic dpi and >> the specific compatible? >> >> dpi@xxx { >> compatible = "mediatek,dpi", "mediatek,mt8195-dpi"; > >I don't know what's this but certainly looks odd. Some wild-card >compatible in front (not fallback) and none are documented. > >> ... >> } >> >> Then I can "collapse" all the dpi related nodes in mtk_drm_drv under >> "mediatek,dpi" ? >> >> I guess would have to do the change for all other components that are needed in >> mtk_drm_drv (mmsys, aal, ccor, color, dither, dsc, gamma, mutex...). >> >> That's the only trivial way I can think of implementing this with the >> current status >> of the mtk_drm stack. >> >> Do you have any other ideas in mind? > >Use fallback of compatible device. That's the common pattern. >Everywhere, Mediatek as well. > I'm unsure about what a "fallback of compatible device" is. But from what I can gather, you'd have me write the dts as: dpi@xxx { compatible = "mediatek,mt8195-dpi", "mediatek,mt2701-dpi"; ... } so that the mtk_dpi driver will use the specific mt8195 dpi config and the mtk_drm driver will fallback to mt2701 to find the compatible it needs. Would you like me to use this pattern for all the other compatibles declared in the mtk_ddp_comp_ids array in separate patches? Thx, Guillaume. >Best regards, >Krzysztof >
On 05/10/2022 11:34, Guillaume Ranquet wrote: > On Tue, 04 Oct 2022 17:05, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> On 04/10/2022 13:55, Guillaume Ranquet wrote: >>>> No. You said what the code is doing. I think I understand this. You >>>> still do not need more compatibles. Your sentence did not clarify it >>>> because it did not answer at all to question "why". Why do you need it? >>>> >>>> Sorry, the change looks not correct. >>>> >>>> Best regards, >>>> Krzysztof >>>> >>> >>> I need a new compatible to adress the specifics of mt8195 in the mtk_dpi driver, >>> the change is in this series with: >>> [PATCH v1 16/17] drm/mediatek: dpi: Add mt8195 hdmi to DPI driver [1] >> >> But you do not have specifics of mt8195. I wrote it in the beginning. >> >>> >>> I then need to add that compatible to the "list" here in mtk_drm_drv. >> >> No, you do not... I checked the driver and there is no single need... or >> convince me you need. >> >>> I don't see a way around this unless I rewrite the way mtk_drm_drv works? >> >> Why rewrite? You have all compatibles in place. >> >>> >>> Maybe if I declare a new compatible that is generic to all mediatek >>> dpi variants? >> >> You were asked to use fallback. Don't create some fake fallbacks. Use >> existing ones. >> >>> and have all the dts specify the node with both the generic dpi and >>> the specific compatible? >>> >>> dpi@xxx { >>> compatible = "mediatek,dpi", "mediatek,mt8195-dpi"; >> >> I don't know what's this but certainly looks odd. Some wild-card >> compatible in front (not fallback) and none are documented. >> >>> ... >>> } >>> >>> Then I can "collapse" all the dpi related nodes in mtk_drm_drv under >>> "mediatek,dpi" ? >>> >>> I guess would have to do the change for all other components that are needed in >>> mtk_drm_drv (mmsys, aal, ccor, color, dither, dsc, gamma, mutex...). >>> >>> That's the only trivial way I can think of implementing this with the >>> current status >>> of the mtk_drm stack. >>> >>> Do you have any other ideas in mind? >> >> Use fallback of compatible device. That's the common pattern. >> Everywhere, Mediatek as well. >> > > I'm unsure about what a "fallback of compatible device" is. > But from what I can gather, you'd have me write the dts as: > > dpi@xxx { > compatible = "mediatek,mt8195-dpi", "mediatek,mt2701-dpi"; > ... > } Sounds reasonable. Just someone should check whether devices are actually compatible, because driver is just a hint. > > so that the mtk_dpi driver will use the specific mt8195 dpi config and > the mtk_drm driver will fallback to mt2701 to find the compatible it needs. > > Would you like me to use this pattern for all the other compatibles declared > in the mtk_ddp_comp_ids array in separate patches? Could be for consistency, but it is up to you. Best regards, Krzysztof
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index 72049a530ae1..27f029ca760b 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c @@ -820,6 +820,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = { .data = (void *)MTK_DPI }, { .compatible = "mediatek,mt8192-dpi", .data = (void *)MTK_DPI }, + { .compatible = "mediatek,mt8195-dpi", + .data = (void *)MTK_DPI }, { .compatible = "mediatek,mt8195-dp-intf", .data = (void *)MTK_DP_INTF }, { .compatible = "mediatek,mt2701-dsi",
Add dpi support to enable the HDMI path. Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>