| Message ID | 20200206191834.6125-3-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | drm/bridge: dw-hdmi: implement bus-format negotiation and YUV420 support | expand |
Hi! Dne četrtek, 06. februar 2020 ob 20:18:25 CET je Neil Armstrong napisal(a): > From: Jonas Karlman <jonas@kwiboo.se> > > Add the max_bpc property to the dw-hdmi connector to prepare support > for 10, 12 & 16bit output support. > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > 9e0927d22db6..051001f77dd4 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -2406,6 +2406,10 @@ static int dw_hdmi_bridge_attach(struct drm_bridge > *bridge) DRM_MODE_CONNECTOR_HDMIA, > hdmi->ddc); > > + drm_atomic_helper_connector_reset(connector); Why is this reset needed? Best regards, Jernej > + > + drm_connector_attach_max_bpc_property(connector, 8, 16); > + > if (hdmi->version >= 0x200a && hdmi->plat_data->use_drm_infoframe) > drm_object_attach_property(&connector->base, > connector->dev- >mode_config.hdr_output_metadata_property, 0);
Hi Jernej, On 17/02/2020 07:38, Jernej Škrabec wrote: > Hi! > > Dne četrtek, 06. februar 2020 ob 20:18:25 CET je Neil Armstrong napisal(a): >> From: Jonas Karlman <jonas@kwiboo.se> >> >> Add the max_bpc property to the dw-hdmi connector to prepare support >> for 10, 12 & 16bit output support. >> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index >> 9e0927d22db6..051001f77dd4 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> @@ -2406,6 +2406,10 @@ static int dw_hdmi_bridge_attach(struct drm_bridge >> *bridge) DRM_MODE_CONNECTOR_HDMIA, >> hdmi->ddc); >> >> + drm_atomic_helper_connector_reset(connector); > > Why is this reset needed? I assume it's to allocate a new connector state to attach a the bpc propery. But indeed, this helper is never used here, but only as callback to the drm_connector_funcs->reset. But, amdgpu calls : /* * Some of the properties below require access to state, like bpc. * Allocate some default initial connector state with our reset helper. */ if (aconnector->base.funcs->reset) aconnector->base.funcs->reset(&aconnector->base); which is the same. Neil > > Best regards, > Jernej > >> + >> + drm_connector_attach_max_bpc_property(connector, 8, 16); >> + >> if (hdmi->version >= 0x200a && hdmi->plat_data->use_drm_infoframe) >> drm_object_attach_property(&connector->base, >> connector->dev- >> mode_config.hdr_output_metadata_property, 0); > > > >
Hi Neil and Jonas, (CC'ing Daniel for a framework question) Thank you for the patch. On Fri, Feb 21, 2020 at 09:50:18AM +0100, Neil Armstrong wrote: > On 17/02/2020 07:38, Jernej Škrabec wrote: > > Dne četrtek, 06. februar 2020 ob 20:18:25 CET je Neil Armstrong napisal(a): > >> From: Jonas Karlman <jonas@kwiboo.se> > >> > >> Add the max_bpc property to the dw-hdmi connector to prepare support > >> for 10, 12 & 16bit output support. > >> > >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > >> --- > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > >> 9e0927d22db6..051001f77dd4 100644 > >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >> @@ -2406,6 +2406,10 @@ static int dw_hdmi_bridge_attach(struct drm_bridge > >> *bridge) DRM_MODE_CONNECTOR_HDMIA, > >> hdmi->ddc); > >> > >> + drm_atomic_helper_connector_reset(connector); > > > > Why is this reset needed? > > I assume it's to allocate a new connector state to attach a the bpc propery. > > But indeed, this helper is never used here, but only as callback to the drm_connector_funcs->reset. > > But, amdgpu calls : > /* > * Some of the properties below require access to state, like bpc. > * Allocate some default initial connector state with our reset helper. > */ > if (aconnector->base.funcs->reset) > aconnector->base.funcs->reset(&aconnector->base); > > which is the same. A comment would be useful: /* * drm_connector_attach_max_bpc_property() requires the * connector to have a state. */ drm_atomic_helper_connector_reset(connector); drm_connector_attach_max_bpc_property(connector, 8, 16); I don't like this much though, it feels like the initial reset performed by drm_mode_config_reset() should set default values for all state members that are related to properties. Daniel, what's the rationale behind the current implementation ? This is a DRM core issue that shouldn't block this patch though, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + > >> + drm_connector_attach_max_bpc_property(connector, 8, 16); > >> + > >> if (hdmi->version >= 0x200a && hdmi->plat_data->use_drm_infoframe) > >> drm_object_attach_property(&connector->base, > >> connector->dev- > >> mode_config.hdr_output_metadata_property, 0);
On 02/03/2020 10:18, Laurent Pinchart wrote: > Hi Neil and Jonas, > > (CC'ing Daniel for a framework question) > > Thank you for the patch. > > On Fri, Feb 21, 2020 at 09:50:18AM +0100, Neil Armstrong wrote: >> On 17/02/2020 07:38, Jernej Škrabec wrote: >>> Dne četrtek, 06. februar 2020 ob 20:18:25 CET je Neil Armstrong napisal(a): >>>> From: Jonas Karlman <jonas@kwiboo.se> >>>> >>>> Add the max_bpc property to the dw-hdmi connector to prepare support >>>> for 10, 12 & 16bit output support. >>>> >>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> >>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>>> --- >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index >>>> 9e0927d22db6..051001f77dd4 100644 >>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>> @@ -2406,6 +2406,10 @@ static int dw_hdmi_bridge_attach(struct drm_bridge >>>> *bridge) DRM_MODE_CONNECTOR_HDMIA, >>>> hdmi->ddc); >>>> >>>> + drm_atomic_helper_connector_reset(connector); >>> >>> Why is this reset needed? >> >> I assume it's to allocate a new connector state to attach a the bpc propery. >> >> But indeed, this helper is never used here, but only as callback to the drm_connector_funcs->reset. >> >> But, amdgpu calls : >> /* >> * Some of the properties below require access to state, like bpc. >> * Allocate some default initial connector state with our reset helper. >> */ >> if (aconnector->base.funcs->reset) >> aconnector->base.funcs->reset(&aconnector->base); >> >> which is the same. > > A comment would be useful: > > /* > * drm_connector_attach_max_bpc_property() requires the > * connector to have a state. > */ > drm_atomic_helper_connector_reset(connector); > > drm_connector_attach_max_bpc_property(connector, 8, 16); > Done > I don't like this much though, it feels like the initial reset performed > by drm_mode_config_reset() should set default values for all state > members that are related to properties. Daniel, what's the rationale > behind the current implementation ? > > This is a DRM core issue that shouldn't block this patch though, so I'll investigate why, but I haven't found out how the intel driver got the connector state initialized since they don't use the atomic helpers.... > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> + >>>> + drm_connector_attach_max_bpc_property(connector, 8, 16); >>>> + >>>> if (hdmi->version >= 0x200a && hdmi->plat_data->use_drm_infoframe) >>>> drm_object_attach_property(&connector->base, >>>> connector->dev- >>>> mode_config.hdr_output_metadata_property, 0); >
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 9e0927d22db6..051001f77dd4 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2406,6 +2406,10 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) DRM_MODE_CONNECTOR_HDMIA, hdmi->ddc); + drm_atomic_helper_connector_reset(connector); + + drm_connector_attach_max_bpc_property(connector, 8, 16); + if (hdmi->version >= 0x200a && hdmi->plat_data->use_drm_infoframe) drm_object_attach_property(&connector->base, connector->dev->mode_config.hdr_output_metadata_property, 0);