| Message ID | 20250126-drm-hdmi-connector-cec-v3-1-5b5b2d4956da@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/display: generic HDMI CEC helpers | expand |
On Sun, Jan 26, 2025 at 03:29:06PM +0200, Dmitry Baryshkov wrote: > As a preparation to adding HDMI CEC helper code, add CEC-related fields > to the struct drm_connector. Include both cec_adapter and cec_notifier, > allowing drivers to select which one to use. The unregister callback > is provided to let drivers unregister CEC-related data in a generic way > without polluting drm_connector.c with dependencies on the CEC > functions. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/drm_connector.c | 13 ++++++++++++ > include/drm/drm_connector.h | 44 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 57 insertions(+) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 1383fa9fff9bcf31488453e209a36c6fe97be2f1..fffb718b09eaaac200e6abc7524bbfe98c4741f4 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -279,6 +279,7 @@ static int drm_connector_init_only(struct drm_device *dev, > INIT_LIST_HEAD(&connector->probed_modes); > INIT_LIST_HEAD(&connector->modes); > mutex_init(&connector->mutex); > + mutex_init(&connector->cec.mutex); > mutex_init(&connector->eld_mutex); > mutex_init(&connector->edid_override_mutex); > mutex_init(&connector->hdmi.infoframes.lock); > @@ -698,6 +699,16 @@ static void drm_mode_remove(struct drm_connector *connector, > drm_mode_destroy(connector->dev, mode); > } > > +static void drm_connector_cec_unregister(struct drm_connector *connector) > +{ > + mutex_lock(&connector->cec.mutex); > + > + if (connector->cec.funcs->unregister) > + connector->cec.funcs->unregister(connector); > + > + mutex_unlock(&connector->cec.mutex); > +} > + > /** > * drm_connector_cleanup - cleans up an initialised connector > * @connector: connector to cleanup > @@ -718,6 +729,8 @@ void drm_connector_cleanup(struct drm_connector *connector) > > platform_device_unregister(connector->hdmi_audio.codec_pdev); > > + drm_connector_cec_unregister(connector); > + This should either be in a separate patch, or mentioned in the commit title/log > if (connector->privacy_screen) { > drm_privacy_screen_put(connector->privacy_screen); > connector->privacy_screen = NULL; > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index f13d597370a30dc1b14c630ee00145256052ba56..6da840673b1209c84bbc396643c6033679a7ec74 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -46,6 +46,7 @@ struct drm_property_blob; > struct drm_printer; > struct drm_privacy_screen; > struct drm_edid; > +struct cec_adapter; > struct edid; > struct hdmi_codec_daifmt; > struct hdmi_codec_params; > @@ -1191,6 +1192,19 @@ struct drm_connector_hdmi_audio_funcs { > bool enable, int direction); > }; > > +/** > + * struct drm_connector_cec_funcs - drm_hdmi_connector control functions > + */ > +struct drm_connector_cec_funcs { > + /** > + * @adap_unregister: unregister CEC adapter / notifier. > + * > + * The callback to unregister CEC adapter or notifier, so that the core > + * DRM layer doesn't depend on the CEC_CORE. > + */ > + void (*unregister)(struct drm_connector *connector); > +}; > + > /** > * struct drm_connector_hdmi_funcs - drm_hdmi_connector control functions > */ > @@ -1832,6 +1846,31 @@ struct drm_connector_hdmi { > } infoframes; > }; > > +/** > + * struct drm_connector_cec - DRM Connector CEC-related structure > + */ > +struct drm_connector_cec { > + /** > + * @mutex: protects all CEC-related fields > + */ All fields? Which fields require to be protected by a specific mutex here? Maxime
On Tue, Jan 28, 2025 at 11:33:05AM +0100, Maxime Ripard wrote: > On Sun, Jan 26, 2025 at 03:29:06PM +0200, Dmitry Baryshkov wrote: > > As a preparation to adding HDMI CEC helper code, add CEC-related fields > > to the struct drm_connector. Include both cec_adapter and cec_notifier, > > allowing drivers to select which one to use. The unregister callback > > is provided to let drivers unregister CEC-related data in a generic way > > without polluting drm_connector.c with dependencies on the CEC > > functions. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/drm_connector.c | 13 ++++++++++++ > > include/drm/drm_connector.h | 44 +++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 57 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > index 1383fa9fff9bcf31488453e209a36c6fe97be2f1..fffb718b09eaaac200e6abc7524bbfe98c4741f4 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -279,6 +279,7 @@ static int drm_connector_init_only(struct drm_device *dev, > > INIT_LIST_HEAD(&connector->probed_modes); > > INIT_LIST_HEAD(&connector->modes); > > mutex_init(&connector->mutex); > > + mutex_init(&connector->cec.mutex); > > mutex_init(&connector->eld_mutex); > > mutex_init(&connector->edid_override_mutex); > > mutex_init(&connector->hdmi.infoframes.lock); > > @@ -698,6 +699,16 @@ static void drm_mode_remove(struct drm_connector *connector, > > drm_mode_destroy(connector->dev, mode); > > } > > > > +static void drm_connector_cec_unregister(struct drm_connector *connector) > > +{ > > + mutex_lock(&connector->cec.mutex); > > + > > + if (connector->cec.funcs->unregister) > > + connector->cec.funcs->unregister(connector); > > + > > + mutex_unlock(&connector->cec.mutex); > > +} > > + > > /** > > * drm_connector_cleanup - cleans up an initialised connector > > * @connector: connector to cleanup > > @@ -718,6 +729,8 @@ void drm_connector_cleanup(struct drm_connector *connector) > > > > platform_device_unregister(connector->hdmi_audio.codec_pdev); > > > > + drm_connector_cec_unregister(connector); > > + > > This should either be in a separate patch, or mentioned in the commit title/log I'll mention it in the commit message. > > > if (connector->privacy_screen) { > > drm_privacy_screen_put(connector->privacy_screen); > > connector->privacy_screen = NULL; > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > index f13d597370a30dc1b14c630ee00145256052ba56..6da840673b1209c84bbc396643c6033679a7ec74 100644 > > --- a/include/drm/drm_connector.h > > +++ b/include/drm/drm_connector.h > > @@ -46,6 +46,7 @@ struct drm_property_blob; > > struct drm_printer; > > struct drm_privacy_screen; > > struct drm_edid; > > +struct cec_adapter; > > struct edid; > > struct hdmi_codec_daifmt; > > struct hdmi_codec_params; > > @@ -1191,6 +1192,19 @@ struct drm_connector_hdmi_audio_funcs { > > bool enable, int direction); > > }; > > > > +/** > > + * struct drm_connector_cec_funcs - drm_hdmi_connector control functions > > + */ > > +struct drm_connector_cec_funcs { > > + /** > > + * @adap_unregister: unregister CEC adapter / notifier. > > + * > > + * The callback to unregister CEC adapter or notifier, so that the core > > + * DRM layer doesn't depend on the CEC_CORE. > > + */ > > + void (*unregister)(struct drm_connector *connector); > > +}; > > + > > /** > > * struct drm_connector_hdmi_funcs - drm_hdmi_connector control functions > > */ > > @@ -1832,6 +1846,31 @@ struct drm_connector_hdmi { > > } infoframes; > > }; > > > > +/** > > + * struct drm_connector_cec - DRM Connector CEC-related structure > > + */ > > +struct drm_connector_cec { > > + /** > > + * @mutex: protects all CEC-related fields > > + */ > > All fields? Which fields require to be protected by a specific mutex > here? Yes, all the fields. adapter, notifier and funcs are all protected by the mutex. See the drm_connector_cec_unregister() implementation (and corresponding unregister() callbacks implementations.
On Tue, Jan 28, 2025 at 03:12:41PM +0200, Dmitry Baryshkov wrote: > On Tue, Jan 28, 2025 at 11:33:05AM +0100, Maxime Ripard wrote: > > On Sun, Jan 26, 2025 at 03:29:06PM +0200, Dmitry Baryshkov wrote: > > > As a preparation to adding HDMI CEC helper code, add CEC-related fields > > > to the struct drm_connector. Include both cec_adapter and cec_notifier, > > > allowing drivers to select which one to use. The unregister callback > > > is provided to let drivers unregister CEC-related data in a generic way > > > without polluting drm_connector.c with dependencies on the CEC > > > functions. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/gpu/drm/drm_connector.c | 13 ++++++++++++ > > > include/drm/drm_connector.h | 44 +++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 57 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > > index 1383fa9fff9bcf31488453e209a36c6fe97be2f1..fffb718b09eaaac200e6abc7524bbfe98c4741f4 100644 > > > --- a/drivers/gpu/drm/drm_connector.c > > > +++ b/drivers/gpu/drm/drm_connector.c > > > @@ -279,6 +279,7 @@ static int drm_connector_init_only(struct drm_device *dev, > > > INIT_LIST_HEAD(&connector->probed_modes); > > > INIT_LIST_HEAD(&connector->modes); > > > mutex_init(&connector->mutex); > > > + mutex_init(&connector->cec.mutex); > > > mutex_init(&connector->eld_mutex); > > > mutex_init(&connector->edid_override_mutex); > > > mutex_init(&connector->hdmi.infoframes.lock); > > > @@ -698,6 +699,16 @@ static void drm_mode_remove(struct drm_connector *connector, > > > drm_mode_destroy(connector->dev, mode); > > > } > > > > > > +static void drm_connector_cec_unregister(struct drm_connector *connector) > > > +{ > > > + mutex_lock(&connector->cec.mutex); > > > + > > > + if (connector->cec.funcs->unregister) > > > + connector->cec.funcs->unregister(connector); > > > + > > > + mutex_unlock(&connector->cec.mutex); > > > +} > > > + > > > /** > > > * drm_connector_cleanup - cleans up an initialised connector > > > * @connector: connector to cleanup > > > @@ -718,6 +729,8 @@ void drm_connector_cleanup(struct drm_connector *connector) > > > > > > platform_device_unregister(connector->hdmi_audio.codec_pdev); > > > > > > + drm_connector_cec_unregister(connector); > > > + > > > > This should either be in a separate patch, or mentioned in the commit title/log > > I'll mention it in the commit message. > > > > > > if (connector->privacy_screen) { > > > drm_privacy_screen_put(connector->privacy_screen); > > > connector->privacy_screen = NULL; > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > > index f13d597370a30dc1b14c630ee00145256052ba56..6da840673b1209c84bbc396643c6033679a7ec74 100644 > > > --- a/include/drm/drm_connector.h > > > +++ b/include/drm/drm_connector.h > > > @@ -46,6 +46,7 @@ struct drm_property_blob; > > > struct drm_printer; > > > struct drm_privacy_screen; > > > struct drm_edid; > > > +struct cec_adapter; > > > struct edid; > > > struct hdmi_codec_daifmt; > > > struct hdmi_codec_params; > > > @@ -1191,6 +1192,19 @@ struct drm_connector_hdmi_audio_funcs { > > > bool enable, int direction); > > > }; > > > > > > +/** > > > + * struct drm_connector_cec_funcs - drm_hdmi_connector control functions > > > + */ > > > +struct drm_connector_cec_funcs { > > > + /** > > > + * @adap_unregister: unregister CEC adapter / notifier. > > > + * > > > + * The callback to unregister CEC adapter or notifier, so that the core > > > + * DRM layer doesn't depend on the CEC_CORE. > > > + */ > > > + void (*unregister)(struct drm_connector *connector); > > > +}; > > > + > > > /** > > > * struct drm_connector_hdmi_funcs - drm_hdmi_connector control functions > > > */ > > > @@ -1832,6 +1846,31 @@ struct drm_connector_hdmi { > > > } infoframes; > > > }; > > > > > > +/** > > > + * struct drm_connector_cec - DRM Connector CEC-related structure > > > + */ > > > +struct drm_connector_cec { > > > + /** > > > + * @mutex: protects all CEC-related fields > > > + */ > > > > All fields? Which fields require to be protected by a specific mutex > > here? > > Yes, all the fields. adapter, notifier and funcs are all protected by > the mutex. See the drm_connector_cec_unregister() implementation (and > corresponding unregister() callbacks implementations. That's still surprising to me. Like, what concurrency source / code path will need to make sure funcs is updated properly? or the adapter and notifier? Maxime
On Wed, 5 Feb 2025 at 16:28, Maxime Ripard <mripard@kernel.org> wrote: > > On Tue, Jan 28, 2025 at 03:12:41PM +0200, Dmitry Baryshkov wrote: > > On Tue, Jan 28, 2025 at 11:33:05AM +0100, Maxime Ripard wrote: > > > On Sun, Jan 26, 2025 at 03:29:06PM +0200, Dmitry Baryshkov wrote: > > > > As a preparation to adding HDMI CEC helper code, add CEC-related fields > > > > to the struct drm_connector. Include both cec_adapter and cec_notifier, > > > > allowing drivers to select which one to use. The unregister callback > > > > is provided to let drivers unregister CEC-related data in a generic way > > > > without polluting drm_connector.c with dependencies on the CEC > > > > functions. > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > --- > > > > drivers/gpu/drm/drm_connector.c | 13 ++++++++++++ > > > > include/drm/drm_connector.h | 44 +++++++++++++++++++++++++++++++++++++++++ > > > > 2 files changed, 57 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > > > > index 1383fa9fff9bcf31488453e209a36c6fe97be2f1..fffb718b09eaaac200e6abc7524bbfe98c4741f4 100644 > > > > --- a/drivers/gpu/drm/drm_connector.c > > > > +++ b/drivers/gpu/drm/drm_connector.c > > > > @@ -279,6 +279,7 @@ static int drm_connector_init_only(struct drm_device *dev, > > > > INIT_LIST_HEAD(&connector->probed_modes); > > > > INIT_LIST_HEAD(&connector->modes); > > > > mutex_init(&connector->mutex); > > > > + mutex_init(&connector->cec.mutex); > > > > mutex_init(&connector->eld_mutex); > > > > mutex_init(&connector->edid_override_mutex); > > > > mutex_init(&connector->hdmi.infoframes.lock); > > > > @@ -698,6 +699,16 @@ static void drm_mode_remove(struct drm_connector *connector, > > > > drm_mode_destroy(connector->dev, mode); > > > > } > > > > > > > > +static void drm_connector_cec_unregister(struct drm_connector *connector) > > > > +{ > > > > + mutex_lock(&connector->cec.mutex); > > > > + > > > > + if (connector->cec.funcs->unregister) > > > > + connector->cec.funcs->unregister(connector); > > > > + > > > > + mutex_unlock(&connector->cec.mutex); > > > > +} > > > > + > > > > /** > > > > * drm_connector_cleanup - cleans up an initialised connector > > > > * @connector: connector to cleanup > > > > @@ -718,6 +729,8 @@ void drm_connector_cleanup(struct drm_connector *connector) > > > > > > > > platform_device_unregister(connector->hdmi_audio.codec_pdev); > > > > > > > > + drm_connector_cec_unregister(connector); > > > > + > > > > > > This should either be in a separate patch, or mentioned in the commit title/log > > > > I'll mention it in the commit message. > > > > > > > > > if (connector->privacy_screen) { > > > > drm_privacy_screen_put(connector->privacy_screen); > > > > connector->privacy_screen = NULL; > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > > > index f13d597370a30dc1b14c630ee00145256052ba56..6da840673b1209c84bbc396643c6033679a7ec74 100644 > > > > --- a/include/drm/drm_connector.h > > > > +++ b/include/drm/drm_connector.h > > > > @@ -46,6 +46,7 @@ struct drm_property_blob; > > > > struct drm_printer; > > > > struct drm_privacy_screen; > > > > struct drm_edid; > > > > +struct cec_adapter; > > > > struct edid; > > > > struct hdmi_codec_daifmt; > > > > struct hdmi_codec_params; > > > > @@ -1191,6 +1192,19 @@ struct drm_connector_hdmi_audio_funcs { > > > > bool enable, int direction); > > > > }; > > > > > > > > +/** > > > > + * struct drm_connector_cec_funcs - drm_hdmi_connector control functions > > > > + */ > > > > +struct drm_connector_cec_funcs { > > > > + /** > > > > + * @adap_unregister: unregister CEC adapter / notifier. > > > > + * > > > > + * The callback to unregister CEC adapter or notifier, so that the core > > > > + * DRM layer doesn't depend on the CEC_CORE. > > > > + */ > > > > + void (*unregister)(struct drm_connector *connector); > > > > +}; > > > > + > > > > /** > > > > * struct drm_connector_hdmi_funcs - drm_hdmi_connector control functions > > > > */ > > > > @@ -1832,6 +1846,31 @@ struct drm_connector_hdmi { > > > > } infoframes; > > > > }; > > > > > > > > +/** > > > > + * struct drm_connector_cec - DRM Connector CEC-related structure > > > > + */ > > > > +struct drm_connector_cec { > > > > + /** > > > > + * @mutex: protects all CEC-related fields > > > > + */ > > > > > > All fields? Which fields require to be protected by a specific mutex > > > here? > > > > Yes, all the fields. adapter, notifier and funcs are all protected by > > the mutex. See the drm_connector_cec_unregister() implementation (and > > corresponding unregister() callbacks implementations. > > That's still surprising to me. Like, what concurrency source / code path > will need to make sure funcs is updated properly? or the adapter and > notifier? I'll have to check, maybe we can lift that and really protect only the adapter and the notifier. I was using ->funcs != NULL as a way to check that CEC is registered at all (and thus unsetting it on CEC unregistration). But maybe that's not that necessary. > > Maxime
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 1383fa9fff9bcf31488453e209a36c6fe97be2f1..fffb718b09eaaac200e6abc7524bbfe98c4741f4 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -279,6 +279,7 @@ static int drm_connector_init_only(struct drm_device *dev, INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); mutex_init(&connector->mutex); + mutex_init(&connector->cec.mutex); mutex_init(&connector->eld_mutex); mutex_init(&connector->edid_override_mutex); mutex_init(&connector->hdmi.infoframes.lock); @@ -698,6 +699,16 @@ static void drm_mode_remove(struct drm_connector *connector, drm_mode_destroy(connector->dev, mode); } +static void drm_connector_cec_unregister(struct drm_connector *connector) +{ + mutex_lock(&connector->cec.mutex); + + if (connector->cec.funcs->unregister) + connector->cec.funcs->unregister(connector); + + mutex_unlock(&connector->cec.mutex); +} + /** * drm_connector_cleanup - cleans up an initialised connector * @connector: connector to cleanup @@ -718,6 +729,8 @@ void drm_connector_cleanup(struct drm_connector *connector) platform_device_unregister(connector->hdmi_audio.codec_pdev); + drm_connector_cec_unregister(connector); + if (connector->privacy_screen) { drm_privacy_screen_put(connector->privacy_screen); connector->privacy_screen = NULL; diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index f13d597370a30dc1b14c630ee00145256052ba56..6da840673b1209c84bbc396643c6033679a7ec74 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -46,6 +46,7 @@ struct drm_property_blob; struct drm_printer; struct drm_privacy_screen; struct drm_edid; +struct cec_adapter; struct edid; struct hdmi_codec_daifmt; struct hdmi_codec_params; @@ -1191,6 +1192,19 @@ struct drm_connector_hdmi_audio_funcs { bool enable, int direction); }; +/** + * struct drm_connector_cec_funcs - drm_hdmi_connector control functions + */ +struct drm_connector_cec_funcs { + /** + * @adap_unregister: unregister CEC adapter / notifier. + * + * The callback to unregister CEC adapter or notifier, so that the core + * DRM layer doesn't depend on the CEC_CORE. + */ + void (*unregister)(struct drm_connector *connector); +}; + /** * struct drm_connector_hdmi_funcs - drm_hdmi_connector control functions */ @@ -1832,6 +1846,31 @@ struct drm_connector_hdmi { } infoframes; }; +/** + * struct drm_connector_cec - DRM Connector CEC-related structure + */ +struct drm_connector_cec { + /** + * @mutex: protects all CEC-related fields + */ + struct mutex mutex; + + /** + * @adap: CEC adapter corresponding to the DRM connector. + */ + struct cec_adapter *adapter; + + /** + * @notifier: CEC notifier corresponding to the DRM connector. + */ + struct cec_notifier *notifier; + + /** + * @funcs: CEC Control Functions + */ + const struct drm_connector_cec_funcs *funcs; +}; + /** * struct drm_connector - central DRM connector control structure * @@ -2253,6 +2292,11 @@ struct drm_connector { * @hdmi_audio: HDMI codec properties and non-DRM state. */ struct drm_connector_hdmi_audio hdmi_audio; + + /** + * @cec: CEC-related data. + */ + struct drm_connector_cec cec; }; #define obj_to_connector(x) container_of(x, struct drm_connector, base)
As a preparation to adding HDMI CEC helper code, add CEC-related fields to the struct drm_connector. Include both cec_adapter and cec_notifier, allowing drivers to select which one to use. The unregister callback is provided to let drivers unregister CEC-related data in a generic way without polluting drm_connector.c with dependencies on the CEC functions. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/drm_connector.c | 13 ++++++++++++ include/drm/drm_connector.h | 44 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+)