| Message ID | YVLYh/SgBritG/RJ@qwark.sigxcpu.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: mxsfb: Set proper default bus format when using a bridge | expand |
On 9/28/21 10:55 AM, Guido Günther wrote: > If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is > returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in > that case. > > This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. > > Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > --- > > I'll look at what needs to be done in nwl separately but this also > unbreaks other bridge seupts that don't to format negotiation yet. > > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > index af6c620adf6e..4ef94cf686b0 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, > drm_atomic_get_new_bridge_state(state, > mxsfb->bridge); > bus_format = bridge_state->input_bus_cfg.format; > + if (bus_format == MEDIA_BUS_FMT_FIXED) > + bus_format = MEDIA_BUS_FMT_RGB888_1X24; Shouldn't the NWL bridge return the correct format ?
Hi, On Tue, Sep 28, 2021 at 11:08:58AM +0200, Marek Vasut wrote: > On 9/28/21 10:55 AM, Guido Günther wrote: > > If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is > > returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in > > that case. > > > > This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. > > > > Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") > > > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > > --- > > > > I'll look at what needs to be done in nwl separately but this also > > unbreaks other bridge seupts that don't to format negotiation yet. > > > > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > index af6c620adf6e..4ef94cf686b0 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, > > drm_atomic_get_new_bridge_state(state, > > mxsfb->bridge); > > bus_format = bridge_state->input_bus_cfg.format; > > + if (bus_format == MEDIA_BUS_FMT_FIXED) > > + bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > Shouldn't the NWL bridge return the correct format ? Yes it should and I'll send a separate patch for that but we currently don't do anything meaningful at all if the bridge doesn't do format negotiation and then fail setup in mxsfb_set_formats(). I think we should at least preserve the status quo (as we do with the non bridge case in b776b0f00f24 too). We could have a warning to spot drivers that don't do that yet and hence the generic code returns MEDIA_BUS_FMT_FIXED. Cheers, -- Guido
On 9/28/21 11:19 AM, Guido Günther wrote: > Hi, > On Tue, Sep 28, 2021 at 11:08:58AM +0200, Marek Vasut wrote: >> On 9/28/21 10:55 AM, Guido Günther wrote: >>> If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is >>> returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in >>> that case. >>> >>> This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. >>> >>> Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") >>> >>> Signed-off-by: Guido Günther <agx@sigxcpu.org> >>> --- >>> >>> I'll look at what needs to be done in nwl separately but this also >>> unbreaks other bridge seupts that don't to format negotiation yet. >>> >>> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c >>> index af6c620adf6e..4ef94cf686b0 100644 >>> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c >>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c >>> @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, >>> drm_atomic_get_new_bridge_state(state, >>> mxsfb->bridge); >>> bus_format = bridge_state->input_bus_cfg.format; >>> + if (bus_format == MEDIA_BUS_FMT_FIXED) >>> + bus_format = MEDIA_BUS_FMT_RGB888_1X24; >> >> Shouldn't the NWL bridge return the correct format ? > > Yes it should and I'll send a separate patch for that but we currently > don't do anything meaningful at all if the bridge doesn't do format > negotiation and then fail setup in mxsfb_set_formats(). > > I think we should at least preserve the status quo (as we do with the > non bridge case in b776b0f00f24 too). > > We could have a warning to spot drivers that don't do that yet and hence > the generic code returns MEDIA_BUS_FMT_FIXED. I am not gonna push back against this patch, I think you need feedback from the drm people on this. A warning would indeed be nice. Preserving the old behavior in stable releases would be good (if there are any releases which contain the mxsfb patch this fixes).
Am Dienstag, dem 28.09.2021 um 11:19 +0200 schrieb Guido Günther: > Hi, > On Tue, Sep 28, 2021 at 11:08:58AM +0200, Marek Vasut wrote: > > On 9/28/21 10:55 AM, Guido Günther wrote: > > > If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is > > > returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in > > > that case. > > > > > > This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. > > > > > > Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") > > > > > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > > > --- > > > > > > I'll look at what needs to be done in nwl separately but this also > > > unbreaks other bridge seupts that don't to format negotiation yet. > > > > > > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > index af6c620adf6e..4ef94cf686b0 100644 > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, > > > drm_atomic_get_new_bridge_state(state, > > > mxsfb->bridge); > > > bus_format = bridge_state->input_bus_cfg.format; > > > + if (bus_format == MEDIA_BUS_FMT_FIXED) > > > + bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > > > Shouldn't the NWL bridge return the correct format ? > > Yes it should and I'll send a separate patch for that but we currently > don't do anything meaningful at all if the bridge doesn't do format > negotiation and then fail setup in mxsfb_set_formats(). > > I think we should at least preserve the status quo (as we do with the > non bridge case in b776b0f00f24 too). > > We could have a warning to spot drivers that don't do that yet and hence > the generic code returns MEDIA_BUS_FMT_FIXED. > That sounds sensible. Using a default format if we don't know what to do is going to be a unpleasant surprise for those with a display pipeline that doesn't work with the default format. So please add a dev_warn when we are doing this fallback. Also I would argue that the NWL fix is the patch that should go in the stable tree. This one should only be a additional safety net, so I would drop the Fixes tag. Regards, Lucas
On 9/28/21 11:27 AM, Lucas Stach wrote: > Am Dienstag, dem 28.09.2021 um 11:19 +0200 schrieb Guido Günther: >> Hi, >> On Tue, Sep 28, 2021 at 11:08:58AM +0200, Marek Vasut wrote: >>> On 9/28/21 10:55 AM, Guido Günther wrote: >>>> If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is >>>> returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in >>>> that case. >>>> >>>> This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. >>>> >>>> Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") >>>> >>>> Signed-off-by: Guido Günther <agx@sigxcpu.org> >>>> --- >>>> >>>> I'll look at what needs to be done in nwl separately but this also >>>> unbreaks other bridge seupts that don't to format negotiation yet. >>>> >>>> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c >>>> index af6c620adf6e..4ef94cf686b0 100644 >>>> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c >>>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c >>>> @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, >>>> drm_atomic_get_new_bridge_state(state, >>>> mxsfb->bridge); >>>> bus_format = bridge_state->input_bus_cfg.format; >>>> + if (bus_format == MEDIA_BUS_FMT_FIXED) >>>> + bus_format = MEDIA_BUS_FMT_RGB888_1X24; >>> >>> Shouldn't the NWL bridge return the correct format ? >> >> Yes it should and I'll send a separate patch for that but we currently >> don't do anything meaningful at all if the bridge doesn't do format >> negotiation and then fail setup in mxsfb_set_formats(). >> >> I think we should at least preserve the status quo (as we do with the >> non bridge case in b776b0f00f24 too). >> >> We could have a warning to spot drivers that don't do that yet and hence >> the generic code returns MEDIA_BUS_FMT_FIXED. >> > That sounds sensible. Using a default format if we don't know what to > do is going to be a unpleasant surprise for those with a display > pipeline that doesn't work with the default format. So please add a > dev_warn when we are doing this fallback. > > Also I would argue that the NWL fix is the patch that should go in the > stable tree. This one should only be a additional safety net, so I > would drop the Fixes tag. Indeed
Hi, On Tue, Sep 28, 2021 at 11:27:49AM +0200, Lucas Stach wrote: > Am Dienstag, dem 28.09.2021 um 11:19 +0200 schrieb Guido Günther: > > Hi, > > On Tue, Sep 28, 2021 at 11:08:58AM +0200, Marek Vasut wrote: > > > On 9/28/21 10:55 AM, Guido Günther wrote: > > > > If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is > > > > returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in > > > > that case. > > > > > > > > This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. > > > > > > > > Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") > > > > > > > > Signed-off-by: Guido Günther <agx@sigxcpu.org> > > > > --- > > > > > > > > I'll look at what needs to be done in nwl separately but this also > > > > unbreaks other bridge seupts that don't to format negotiation yet. > > > > > > > > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > > index af6c620adf6e..4ef94cf686b0 100644 > > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > > @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, > > > > drm_atomic_get_new_bridge_state(state, > > > > mxsfb->bridge); > > > > bus_format = bridge_state->input_bus_cfg.format; > > > > + if (bus_format == MEDIA_BUS_FMT_FIXED) > > > > + bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > > > > > Shouldn't the NWL bridge return the correct format ? > > > > Yes it should and I'll send a separate patch for that but we currently > > don't do anything meaningful at all if the bridge doesn't do format > > negotiation and then fail setup in mxsfb_set_formats(). > > > > I think we should at least preserve the status quo (as we do with the > > non bridge case in b776b0f00f24 too). > > > > We could have a warning to spot drivers that don't do that yet and hence > > the generic code returns MEDIA_BUS_FMT_FIXED. > > > That sounds sensible. Using a default format if we don't know what to > do is going to be a unpleasant surprise for those with a display > pipeline that doesn't work with the default format. So please add a > dev_warn when we are doing this fallback. > > Also I would argue that the NWL fix is the patch that should go in the > stable tree. This one should only be a additional safety net, so I > would drop the Fixes tag. Dropped and folded into the small series with other fixes needed to get get the bus format propagated up from the panel. Cheers, -- Guido > > Regards, > Lucas >
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index af6c620adf6e..4ef94cf686b0 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, drm_atomic_get_new_bridge_state(state, mxsfb->bridge); bus_format = bridge_state->input_bus_cfg.format; + if (bus_format == MEDIA_BUS_FMT_FIXED) + bus_format = MEDIA_BUS_FMT_RGB888_1X24; } /* If there is no bridge, use bus format from connector */
If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in that case. This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") Signed-off-by: Guido Günther <agx@sigxcpu.org> --- I'll look at what needs to be done in nwl separately but this also unbreaks other bridge seupts that don't to format negotiation yet. drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ 1 file changed, 2 insertions(+)