diff mbox series

drm/msm/hdmi: mark interlace_allowed as true

Message ID 20241019-msm-hdmi-interlaced-v1-1-03bf85133445@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm/msm/hdmi: mark interlace_allowed as true | expand

Commit Message

Dmitry Baryshkov Oct. 18, 2024, 9:10 p.m. UTC 
The MSM HDMI driver supports interlaced modes. Set the corresponding
flag to allow interlaced modes on the corresponding connectors.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 1 +
 1 file changed, 1 insertion(+)


---
base-commit: c4f364c621d0d509190d673d80a9b23250607b4a
change-id: 20241019-msm-hdmi-interlaced-1508c1dc9bb9

Best regards,

Comments

Abhinav Kumar Nov. 1, 2024, 9:41 p.m. UTC | #1 
On 10/18/2024 2:10 PM, Dmitry Baryshkov wrote:
> The MSM HDMI driver supports interlaced modes. Set the corresponding
> flag to allow interlaced modes on the corresponding connectors.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 4a5b5112227f..643c152e6380 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -336,6 +336,7 @@ int msm_hdmi_bridge_init(struct hdmi *hdmi)
>   	bridge->funcs = &msm_hdmi_bridge_funcs;
>   	bridge->ddc = hdmi->i2c;
>   	bridge->type = DRM_MODE_CONNECTOR_HDMIA;
> +	bridge->interlace_allowed = true;
>   	bridge->ops = DRM_BRIDGE_OP_HPD |
>   		DRM_BRIDGE_OP_DETECT |
>   		DRM_BRIDGE_OP_EDID;
> 

I had quite a bit of discussion on this internally because this spans 
quite a few generations of chipsets.

On very old hardware, even before msm8996, there was dedicated hardware 
de-interlacer. But even on msm8996 or other HDMI supported chipsets 
where the handling of if (mode->flags & DRM_MODE_FLAG_INTERLACE) is 
present, these were because its carry forward of older interface code.

The way we handle interlaced formats today, is software needs to handle 
the part of dividing height / 2 and width * 2 and adjust the source crop 
if necessary. This part has moved to userspace for recent chips.

Othwerise, we will need to add this part in the dpu driver to adjust 
this. I am not seeing this part there yet. So may I know how you 
validated this change? Something similar to :

https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/LE.UM.1.3.r3.25/drivers/gpu/drm/msm/sde/sde_plane.c#L1340

If we add this part first to dpu code, then we can mark interlace_allowed.

> ---
> base-commit: c4f364c621d0d509190d673d80a9b23250607b4a
> change-id: 20241019-msm-hdmi-interlaced-1508c1dc9bb9
> 
> Best regards,
Dmitry Baryshkov Nov. 1, 2024, 10:26 p.m. UTC | #2 
On Fri, 1 Nov 2024 at 23:41, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 10/18/2024 2:10 PM, Dmitry Baryshkov wrote:
> > The MSM HDMI driver supports interlaced modes. Set the corresponding
> > flag to allow interlaced modes on the corresponding connectors.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > index 4a5b5112227f..643c152e6380 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > @@ -336,6 +336,7 @@ int msm_hdmi_bridge_init(struct hdmi *hdmi)
> >       bridge->funcs = &msm_hdmi_bridge_funcs;
> >       bridge->ddc = hdmi->i2c;
> >       bridge->type = DRM_MODE_CONNECTOR_HDMIA;
> > +     bridge->interlace_allowed = true;
> >       bridge->ops = DRM_BRIDGE_OP_HPD |
> >               DRM_BRIDGE_OP_DETECT |
> >               DRM_BRIDGE_OP_EDID;
> >
>
> I had quite a bit of discussion on this internally because this spans
> quite a few generations of chipsets.
>
> On very old hardware, even before msm8996, there was dedicated hardware
> de-interlacer. But even on msm8996 or other HDMI supported chipsets
> where the handling of if (mode->flags & DRM_MODE_FLAG_INTERLACE) is
> present, these were because its carry forward of older interface code.
>
> The way we handle interlaced formats today, is software needs to handle
> the part of dividing height / 2 and width * 2 and adjust the source crop
> if necessary. This part has moved to userspace for recent chips.
>
> Othwerise, we will need to add this part in the dpu driver to adjust
> this. I am not seeing this part there yet. So may I know how you
> validated this change? Something similar to :
>
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/LE.UM.1.3.r3.25/drivers/gpu/drm/msm/sde/sde_plane.c#L1340
>
> If we add this part first to dpu code, then we can mark interlace_allowed.

I think you are mixing the interlaced formats and interlaced output.
The code that you have pointed to is related to hardware deinterlacing
- in other words taking the interlaced framebuffer and outputting it
to the progressive display.

The interlace_allowed flag controls a different feature - filtering of
the internalced modes (aka 576i, 1080i, etc). In this case we are
using progressive frames, but the HDMI outputs a picture as two
separate fields. I have validated this by outputting image (modetest)
to the external HDMI display on IFC6410 and on DB820c boards.
Abhinav Kumar Nov. 2, 2024, 12:40 a.m. UTC | #3 
On 11/1/2024 3:26 PM, Dmitry Baryshkov wrote:
> On Fri, 1 Nov 2024 at 23:41, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 10/18/2024 2:10 PM, Dmitry Baryshkov wrote:
>>> The MSM HDMI driver supports interlaced modes. Set the corresponding
>>> flag to allow interlaced modes on the corresponding connectors.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>>> index 4a5b5112227f..643c152e6380 100644
>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>>> @@ -336,6 +336,7 @@ int msm_hdmi_bridge_init(struct hdmi *hdmi)
>>>        bridge->funcs = &msm_hdmi_bridge_funcs;
>>>        bridge->ddc = hdmi->i2c;
>>>        bridge->type = DRM_MODE_CONNECTOR_HDMIA;
>>> +     bridge->interlace_allowed = true;
>>>        bridge->ops = DRM_BRIDGE_OP_HPD |
>>>                DRM_BRIDGE_OP_DETECT |
>>>                DRM_BRIDGE_OP_EDID;
>>>
>>
>> I had quite a bit of discussion on this internally because this spans
>> quite a few generations of chipsets.
>>
>> On very old hardware, even before msm8996, there was dedicated hardware
>> de-interlacer. But even on msm8996 or other HDMI supported chipsets
>> where the handling of if (mode->flags & DRM_MODE_FLAG_INTERLACE) is
>> present, these were because its carry forward of older interface code.
>>
>> The way we handle interlaced formats today, is software needs to handle
>> the part of dividing height / 2 and width * 2 and adjust the source crop
>> if necessary. This part has moved to userspace for recent chips.
>>
>> Othwerise, we will need to add this part in the dpu driver to adjust
>> this. I am not seeing this part there yet. So may I know how you
>> validated this change? Something similar to :
>>
>> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/LE.UM.1.3.r3.25/drivers/gpu/drm/msm/sde/sde_plane.c#L1340
>>
>> If we add this part first to dpu code, then we can mark interlace_allowed.
> 
> I think you are mixing the interlaced formats and interlaced output.
> The code that you have pointed to is related to hardware deinterlacing
> - in other words taking the interlaced framebuffer and outputting it
> to the progressive display.
> 
> The interlace_allowed flag controls a different feature - filtering of
> the internalced modes (aka 576i, 1080i, etc). In this case we are
> using progressive frames, but the HDMI outputs a picture as two
> separate fields. I have validated this by outputting image (modetest)
> to the external HDMI display on IFC6410 and on DB820c boards.
> 

Yes I did think that this was to show interlaced content but that being 
said, I traced through the HDMI code a bit, it does have support for 
changing the HDMI timing but without the support of dpu, progressive 
content really cannot be converted to interlaced. So I think the HDMI 
pieces there were supposed to go along with the rest of the dpu pipeline 
that is the entire pipeline shows out interlaced content. But dpu 
support for giving out interlaced content is not there, so this hdmi 
piece by itself is not complete enough to mark interlace_allowed as true.
Dmitry Baryshkov Nov. 2, 2024, 1:04 a.m. UTC | #4 
On Fri, Nov 01, 2024 at 05:40:46PM -0700, Abhinav Kumar wrote:
> 
> 
> On 11/1/2024 3:26 PM, Dmitry Baryshkov wrote:
> > On Fri, 1 Nov 2024 at 23:41, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > > 
> > > 
> > > 
> > > On 10/18/2024 2:10 PM, Dmitry Baryshkov wrote:
> > > > The MSM HDMI driver supports interlaced modes. Set the corresponding
> > > > flag to allow interlaced modes on the corresponding connectors.
> > > > 
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >    drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 1 +
> > > >    1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > > > index 4a5b5112227f..643c152e6380 100644
> > > > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > > > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > > > @@ -336,6 +336,7 @@ int msm_hdmi_bridge_init(struct hdmi *hdmi)
> > > >        bridge->funcs = &msm_hdmi_bridge_funcs;
> > > >        bridge->ddc = hdmi->i2c;
> > > >        bridge->type = DRM_MODE_CONNECTOR_HDMIA;
> > > > +     bridge->interlace_allowed = true;
> > > >        bridge->ops = DRM_BRIDGE_OP_HPD |
> > > >                DRM_BRIDGE_OP_DETECT |
> > > >                DRM_BRIDGE_OP_EDID;
> > > > 
> > > 
> > > I had quite a bit of discussion on this internally because this spans
> > > quite a few generations of chipsets.
> > > 
> > > On very old hardware, even before msm8996, there was dedicated hardware
> > > de-interlacer. But even on msm8996 or other HDMI supported chipsets
> > > where the handling of if (mode->flags & DRM_MODE_FLAG_INTERLACE) is
> > > present, these were because its carry forward of older interface code.
> > > 
> > > The way we handle interlaced formats today, is software needs to handle
> > > the part of dividing height / 2 and width * 2 and adjust the source crop
> > > if necessary. This part has moved to userspace for recent chips.
> > > 
> > > Othwerise, we will need to add this part in the dpu driver to adjust
> > > this. I am not seeing this part there yet. So may I know how you
> > > validated this change? Something similar to :
> > > 
> > > https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/LE.UM.1.3.r3.25/drivers/gpu/drm/msm/sde/sde_plane.c#L1340
> > > 
> > > If we add this part first to dpu code, then we can mark interlace_allowed.
> > 
> > I think you are mixing the interlaced formats and interlaced output.
> > The code that you have pointed to is related to hardware deinterlacing
> > - in other words taking the interlaced framebuffer and outputting it
> > to the progressive display.
> > 
> > The interlace_allowed flag controls a different feature - filtering of
> > the internalced modes (aka 576i, 1080i, etc). In this case we are
> > using progressive frames, but the HDMI outputs a picture as two
> > separate fields. I have validated this by outputting image (modetest)
> > to the external HDMI display on IFC6410 and on DB820c boards.
> > 
> 
> Yes I did think that this was to show interlaced content but that being
> said, I traced through the HDMI code a bit, it does have support for
> changing the HDMI timing but without the support of dpu, progressive content
> really cannot be converted to interlaced. So I think the HDMI pieces there
> were supposed to go along with the rest of the dpu pipeline that is the
> entire pipeline shows out interlaced content. But dpu support for giving out
> interlaced content is not there, so this hdmi piece by itself is not
> complete enough to mark interlace_allowed as true.

I could not find corresponding bits in the original fbdev or SDE
drivers. My quick tests showed the correct context, but most likely I
need to revertify that. Unfortunately next week I won't be able to run
the tests, so this gets into the 6.14 area.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 4a5b5112227f..643c152e6380 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -336,6 +336,7 @@  int msm_hdmi_bridge_init(struct hdmi *hdmi)
 	bridge->funcs = &msm_hdmi_bridge_funcs;
 	bridge->ddc = hdmi->i2c;
 	bridge->type = DRM_MODE_CONNECTOR_HDMIA;
+	bridge->interlace_allowed = true;
 	bridge->ops = DRM_BRIDGE_OP_HPD |
 		DRM_BRIDGE_OP_DETECT |
 		DRM_BRIDGE_OP_EDID;