diff mbox series

[2/3] drm/dp_mst: Expose build_mst_prop_path()

Message ID 1555977388-14203-2-git-send-email-sunpeng.li@amd.com (mailing list archive)
State New, archived
Headers show
Series [1/3] drm/dp: Use non-cyclic idr | expand

Commit Message

Leo Li April 22, 2019, 11:56 p.m. UTC
From: Leo Li <sunpeng.li@amd.com>

To give identifiable attributes to MST DP aux devices, we can use the
MST relative address. Expose this function for later use.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
 include/drm/drm_dp_mst_helper.h       | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Ville Syrjälä April 23, 2019, 2:52 p.m. UTC | #1
On Mon, Apr 22, 2019 at 07:56:27PM -0400, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> To give identifiable attributes to MST DP aux devices, we can use the
> MST relative address. Expose this function for later use.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
>  include/drm/drm_dp_mst_helper.h       | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 2ab16c9..86ff8e2 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, u8 *guid)
>  	}
>  }
>  
> -static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>  				int pnum,
>  				char *proppath,
>  				size_t proppath_size)

I was wondering if we need to export this but looks like both 
drm_dp_mst_topology.o and drm_dp_aux_dev.o both end up in
drm_kms_helper.ko, so the answer is no.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> @@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
>  	if (created && !port->input) {
>  		char proppath[255];
>  
> -		build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
> +		drm_dp_build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
>  		port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr, port, proppath);
>  		if (!port->connector) {
>  			/* remove it from the port list */
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 371cc28..81c8d79 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>  int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>  			   int pbn);
>  
> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> +				   int pnum,
> +				   char *proppath,
> +				   size_t proppath_size);
>  
>  int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);
>  
> -- 
> 2.7.4
Lyude Paul April 24, 2019, 5:26 p.m. UTC | #2
Closer, but are we sure we want to use the MST prop path for this? Why not add
a sysfs attribute with the corresponding DRM connector name instead since the
connector itself will have a path property. That way we can associate aux
devices for eDP and DP devices with their corresponding connectors as well

On Mon, 2019-04-22 at 19:56 -0400, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> To give identifiable attributes to MST DP aux devices, we can use the
> MST relative address. Expose this function for later use.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
>  include/drm/drm_dp_mst_helper.h       | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 2ab16c9..86ff8e2 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct
> drm_dp_mst_branch *mstb, u8 *guid)
>  	}
>  }
>  
> -static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>  				int pnum,
>  				char *proppath,
>  				size_t proppath_size)
> @@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
> *mstb,
>  	if (created && !port->input) {
>  		char proppath[255];
>  
> -		build_mst_prop_path(mstb, port->port_num, proppath,
> sizeof(proppath));
> +		drm_dp_build_mst_prop_path(mstb, port->port_num, proppath,
> sizeof(proppath));
>  		port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr,
> port, proppath);
>  		if (!port->connector) {
>  			/* remove it from the port list */
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index 371cc28..81c8d79 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct
> drm_dp_mst_topology_mgr *mgr,
>  int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>  			   int pbn);
>  
> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
> +				   int pnum,
> +				   char *proppath,
> +				   size_t proppath_size);
>  
>  int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);
>
Leo Li April 24, 2019, 8:40 p.m. UTC | #3
On 2019-04-24 1:26 p.m., Lyude Paul wrote:
> Closer, but are we sure we want to use the MST prop path for this? Why not add
> a sysfs attribute with the corresponding DRM connector name instead since the
> connector itself will have a path property. That way we can associate aux
> devices for eDP and DP devices with their corresponding connectors as well

I thought about that as well, but I hit a wall when trying to get the
SST connector from the aux device. Perhaps there's a simpler way that
I'm overlooking?

It's easier for MST, since the mst_port can be obtained via container_of
dp_aux. port->connector would then give what we want.

For SST though, each driver calls drm_aux_register() with an aux struct
that they've initialized. I'm not sure how I can reliably get the
drm_connector from that.

Maybe drm_dp_aux should have a back reference to the connector? FWICT
all drivers using drm_aux_register() should be able to provide the
associated connector when calling it.

Leo


> 
> On Mon, 2019-04-22 at 19:56 -0400, sunpeng.li@amd.com wrote:
>> From: Leo Li <sunpeng.li@amd.com>
>>
>> To give identifiable attributes to MST DP aux devices, we can use the
>> MST relative address. Expose this function for later use.
>>
>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>> ---
>>   drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
>>   include/drm/drm_dp_mst_helper.h       | 4 ++++
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index 2ab16c9..86ff8e2 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct
>> drm_dp_mst_branch *mstb, u8 *guid)
>>   	}
>>   }
>>   
>> -static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>>   				int pnum,
>>   				char *proppath,
>>   				size_t proppath_size)
>> @@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
>> *mstb,
>>   	if (created && !port->input) {
>>   		char proppath[255];
>>   
>> -		build_mst_prop_path(mstb, port->port_num, proppath,
>> sizeof(proppath));
>> +		drm_dp_build_mst_prop_path(mstb, port->port_num, proppath,
>> sizeof(proppath));
>>   		port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr,
>> port, proppath);
>>   		if (!port->connector) {
>>   			/* remove it from the port list */
>> diff --git a/include/drm/drm_dp_mst_helper.h
>> b/include/drm/drm_dp_mst_helper.h
>> index 371cc28..81c8d79 100644
>> --- a/include/drm/drm_dp_mst_helper.h
>> +++ b/include/drm/drm_dp_mst_helper.h
>> @@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct
>> drm_dp_mst_topology_mgr *mgr,
>>   int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>>   			   int pbn);
>>   
>> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>> +				   int pnum,
>> +				   char *proppath,
>> +				   size_t proppath_size);
>>   
>>   int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);
>>
Ville Syrjälä April 24, 2019, 8:52 p.m. UTC | #4
On Wed, Apr 24, 2019 at 08:40:30PM +0000, Li, Sun peng (Leo) wrote:
> 
> 
> 
> On 2019-04-24 1:26 p.m., Lyude Paul wrote:
> > Closer, but are we sure we want to use the MST prop path for this? Why not add
> > a sysfs attribute with the corresponding DRM connector name instead since the
> > connector itself will have a path property. That way we can associate aux
> > devices for eDP and DP devices with their corresponding connectors as well
> 
> I thought about that as well, but I hit a wall when trying to get the
> SST connector from the aux device. Perhaps there's a simpler way that
> I'm overlooking?
> 
> It's easier for MST, since the mst_port can be obtained via container_of
> dp_aux. port->connector would then give what we want.
> 
> For SST though, each driver calls drm_aux_register() with an aux struct
> that they've initialized. I'm not sure how I can reliably get the
> drm_connector from that.

On i915 the aux is a child of the connector, so no extra
attributes/links needed. Maybe other drivers should/could
follow  that apporach as well?
Lyude Paul April 24, 2019, 8:55 p.m. UTC | #5
On Wed, 2019-04-24 at 23:52 +0300, Ville Syrjälä wrote:
> On Wed, Apr 24, 2019 at 08:40:30PM +0000, Li, Sun peng (Leo) wrote:
> > 
> > 
> > On 2019-04-24 1:26 p.m., Lyude Paul wrote:
> > > Closer, but are we sure we want to use the MST prop path for this? Why
> > > not add
> > > a sysfs attribute with the corresponding DRM connector name instead
> > > since the
> > > connector itself will have a path property. That way we can associate
> > > aux
> > > devices for eDP and DP devices with their corresponding connectors as
> > > well
> > 
> > I thought about that as well, but I hit a wall when trying to get the
> > SST connector from the aux device. Perhaps there's a simpler way that
> > I'm overlooking?
> > 
> > It's easier for MST, since the mst_port can be obtained via container_of
> > dp_aux. port->connector would then give what we want.
> > 
> > For SST though, each driver calls drm_aux_register() with an aux struct
> > that they've initialized. I'm not sure how I can reliably get the
> > drm_connector from that.
> 
> On i915 the aux is a child of the connector, so no extra
> attributes/links needed. Maybe other drivers should/could
> follow  that apporach as well?

ooo, good point. Yeah that seems like it would be worth a shot since it'd be a
little nicer then just adding more sysfs attributes. But otherwise if that
doesn't work, adding a connector parameter to drm_dp_aux_register() should be
fine.

>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 2ab16c9..86ff8e2 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1120,7 +1120,7 @@  static void drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, u8 *guid)
 	}
 }
 
-static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
+void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
 				int pnum,
 				char *proppath,
 				size_t proppath_size)
@@ -1202,7 +1202,7 @@  static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
 	if (created && !port->input) {
 		char proppath[255];
 
-		build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
+		drm_dp_build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
 		port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr, port, proppath);
 		if (!port->connector) {
 			/* remove it from the port list */
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 371cc28..81c8d79 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -602,6 +602,10 @@  void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
 			   int pbn);
 
+void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
+				   int pnum,
+				   char *proppath,
+				   size_t proppath_size);
 
 int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);